On Tue, Oct 10, 2023 at 08:44:17PM +0000, Ihar Hrachyshka wrote:
> The command receives a flow string and an optional additional payload
> and produces a hex-string representation of the corresponding frame.
>
> The command is datapath-independent and may be useful in tests, where we
> may need to produce hex frame payloads to compare observed frames
> against.
>
> Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
Thanks Ihar,
this looks quite nice to me.
Please find one minor review item inline.
> ---
> ofproto/ofproto-dpif-trace.c | 70 ++++++++++++++++++++++++++++++++++++
> ofproto/ofproto-unixctl.man | 36 ++++++++++++++++++-
> tests/ofproto-dpif.at | 45 +++++++++++++++++++++++
> 3 files changed, 150 insertions(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> index 527e2f17e..2e1931a1f 100644
> --- a/ofproto/ofproto-dpif-trace.c
> +++ b/ofproto/ofproto-dpif-trace.c
> @@ -469,6 +469,73 @@ free_ct_states(struct ovs_list *ct_states)
> }
> }
>
> +static void
> +ofproto_unixctl_hexify(struct unixctl_conn *conn, int argc,
> + const char *argv[], void *aux OVS_UNUSED)
> +{
> + ovs_assert(argc == 2 || argc == 3);
> +
> + char *error = NULL;
> + struct dp_packet *packet = NULL;
> +
> + uint8_t *l7 = NULL;
> + size_t l7_len = 0;
> +
> + struct ofpbuf odp_key = { 0 };
> + struct ofpbuf odp_mask = { 0 };
> + ofpbuf_init(&odp_key, 0);
> + ofpbuf_init(&odp_mask, 0);
nit: I think that ofpbuf_init does a full (enough) initialisation,
so it is unnecessary to first zero odp_key and odp_mask .
> +
> + /* Extract additional payload, if passed */
> + if (argc == 3) {
> + struct dp_packet payload;
> + memset(&payload, 0, sizeof payload);
> + dp_packet_init(&payload, 0);
> + if (dp_packet_put_hex(&payload, argv[2], NULL)[0] != '\0') {
> + dp_packet_uninit(&payload);
> + error = xstrdup("Trailing garbage in payload data");
> + goto out;
> + }
> + l7_len = dp_packet_size(&payload);
> + l7 = dp_packet_steal_data(&payload);
> + }
> +
> + /* Flow string to flow. */
> + /* `hexify` command is backer agnostic, hence port_names=NULL */
> + if (odp_flow_from_string(argv[1], NULL, &odp_key, &odp_mask, &error)) {
> + goto out;
> + }
> + struct flow flow;
> + if (odp_flow_key_to_flow(odp_key.data, odp_key.size, &flow, &error)
> + == ODP_FIT_ERROR) {
> + goto out;
> + }
> +
> + /* Flow to binary. */
> + packet = dp_packet_new(0);
> + flow_compose(packet, &flow, l7, l7_len);
> +
> + /* Binary to hex string. */
> + struct ds result;
> + ds_init(&result);
> + for (int i = 0; i < dp_packet_size(packet); i++) {
> + uint8_t val = ((uint8_t *) dp_packet_data(packet))[i];
> + ds_put_format(&result, "%02"PRIx8, val);
> + }
> +
> + unixctl_command_reply(conn, result.string);
> + ds_destroy(&result);
> +out:
> + if (error) {
> + unixctl_command_reply_error(conn, error);
> + free(error);
> + }
> + dp_packet_delete(packet);
> + free(l7);
> + ofpbuf_uninit(&odp_mask);
> + ofpbuf_uninit(&odp_key);
> +}
...
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
