On 10/10/23 22:44, 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>
Hi, Ihar. Thanks for the patch!
This is an interesting and fairly useful functionality for testing.
But I'm not sure why it is part of ofproto-dpif-trace and why it
is an appctl command. It should be simple to just create a separate
test binary or add it into ovstest instead. This way we will avoid
unnecessary connection to the ovs-vswitchd daemon to perform the
operation that doesn't seem to belong there.
If it were a separate binary we could also add more interesting
options for packet construction that may only make sense for testing
purposes. More on that in the reply to the next patch (incoming).
Separate binary may be better than ovstest integration if we want
to use it for OVN as well, because OVN has its own copy of ovstest
that will overshadow OVS'es, IIRC.
We have a few such small programs like test-stream, for example.
I didn't review the code in full, but see a few comments inline.
Best regards, Ilya Maximets.
> ---
> 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);
> +
> + /* Extract additional payload, if passed */
Period at the end of a comment.
> + if (argc == 3) {
> + struct dp_packet payload;
An empty line.
> + 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 */
Period.
> + if (odp_flow_from_string(argv[1], NULL, &odp_key, &odp_mask, &error)) {
> + goto out;
> + }
> + struct flow flow;
Some blank lines would be nice. Might make sense to just move to the top.
> + 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);
May use DS_EMPTY_INITIALIZER.
> + 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);
> + }
There is a ds_put_hex.
> +
> + 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);
> +}
> +
> static void
> ofproto_unixctl_trace(struct unixctl_conn *conn, int argc, const char
> *argv[],
> void *aux OVS_UNUSED)
> @@ -876,6 +943,9 @@ ofproto_dpif_trace_init(void)
> "[-consistent] {[dp_name] odp_flow | bridge br_flow} [OPTIONS...] "
> "[-generate|packet] actions",
> 2, INT_MAX, ofproto_unixctl_trace_actions, NULL);
> + unixctl_command_register(
> + "ofproto/hexify", "odp_flow [payload]",
> + 1, 2, ofproto_unixctl_hexify, NULL);
> }
>
> void
> diff --git a/ofproto/ofproto-unixctl.man b/ofproto/ofproto-unixctl.man
> index 095afd57c..8dc68a38d 100644
> --- a/ofproto/ofproto-unixctl.man
> +++ b/ofproto/ofproto-unixctl.man
> @@ -4,7 +4,7 @@ These commands manage the core OpenFlow switch implementation
> (called
> .
> .IP "\fBofproto/list\fR"
> Lists the names of the running ofproto instances. These are the names
> -that may be used on \fBofproto/trace\fR.
> +that may be used on \fBofproto/trace\fR and \fBofproto/hexify\fR.
> .
> .IP "\fBofproto/trace\fR [\fIoptions\fR] [\fIdpname\fR] \fIodp_flow\fR
> [\fIpacket\fR]
> .IQ "\fBofproto/trace\fR [\fIoptions\fR] \fIbridge\fR \fIbr_flow\fR
> [\fIpacket\fR]]
> @@ -230,3 +230,37 @@ ofproto/trace br in_port=1,arp,arp_op=2
> .fi
> .RE
> .RE
> +
> +.IQ "\fBofproto/hexify\fR \fIodp_flow\fR [\fIpayload\fR]"
> +Generates a hex digits representation of a frame matching
> +\fIodp_flow\fR. When \fIpayload\fR is passed, the additional payload
> +is attached to the end of the generated frame representation.
> +.
> +.RS
> +.IP \(bu
> +\fIodp_flow\fR is a flow in the form printed by \fBovs\-dpctl\fR(8)'s
> +\fBdump\-flows\fR command.
> +.
> +.IP \(bu
> +The command is agnostic to the datapath, that's why it doesn't support
> +some standard \fIodp_flow\fR features like using port names.
> +.RE
> +.
> +.IP "Usage examples:"
> +.RS 4
> +.PP
> +\fBGenerate a unicast IPv4 TCP ACK packet\fR
> +.RS 4
> +.nf
> +ofproto/hexify eth(src=50:54:00:00:00:05,dst=50:54:00:00:01:00),
> +eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=6),
> +tcp(src=8,dst=9),tcp_flags(ack)
> +.RE
> +.fi
> +.PP
> +\fBGenerate a unicast IPv4 TCP ACK packet with additional payload\fR
> +.RS 4
> +.nf
> +ofproto/hexify eth(src=50:54:00:00:00:05,dst=50:54:00:00:01:00),
> +eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=6),
> +tcp(src=8,dst=9),tcp_flags(ack) 010203abcdef
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index a39d0d3ae..e51e0f412 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -12041,3 +12041,48 @@ AT_CHECK([test 1 = `ovs-ofctl parse-pcap p2-tx.pcap
> | wc -l`])
>
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - ofproto/hexify])
> +OVS_VSWITCHD_START
> +
> +# check that invalid syntax generates an error: ethernet() instead of eth()
> +flow_s="\
> +ethernet(src=50:54:00:00:00:05,dst=50:54:00:00:01:00),eth_type(0x0800),\
> +ipv4(src=10.0.0.2,dst=10.0.0.1,proto=6,tos=0,ttl=64,frag=no),\
> +tcp(src=8,dst=9),tcp_flags(ack)"
> +AT_CHECK([ovs-appctl ofproto/hexify ${flow_s}], [2], [stdout], [stderr])
> +AT_CHECK_UNQUOTED([tail -2 stderr], [0], [syntax error at ${flow_s}
> +ovs-appctl: ovs-vswitchd: server returned an error
> +])
> +
> +# check that a simpler flow string is successfully hexified
> +flow_s="eth(src=50:54:00:00:00:05,dst=50:54:00:00:01:00),eth_type(0x0800)"
> +expected=505400000100505400000005080045000014000000000000baeb0000000000000000
> +actual=$(ovs-appctl ofproto/hexify ${flow_s})
> +AT_CHECK([test ${expected} = ${actual}])
> +
> +# check that a longer flow string is successfully hexified
> +flow_s="\
> +eth(src=50:54:00:00:00:05,dst=50:54:00:00:01:00),eth_type(0x0800),\
> +ipv4(src=10.0.0.2,dst=10.0.0.1,proto=6,tos=0,ttl=64,frag=no),\
> +tcp(src=8,dst=9),tcp_flags(ack)"
> +expected=50540000010050540000000508004500002800000000400666ce0a0000020a000001000800090000000000000000501000009bc10000
> +actual=$(ovs-appctl ofproto/hexify ${flow_s})
> +AT_CHECK([test ${expected} = ${actual}])
> +
> +# check that incomplete payload fails the command
> +l7payload=$(printf 'd%.0s' {1..13})
> +AT_CHECK([ovs-appctl ofproto/hexify ${flow_s} ${l7payload}], [2], [stdout],
> [stderr])
> +AT_CHECK([tail -2 stderr], [0], [dnl
> +Trailing garbage in payload data
> +ovs-appctl: ovs-vswitchd: server returned an error
> +])
> +
> +# check that a properly sized payload is successfully appended
> +l7payload=$(printf 'dead%.0s' {1..10})
> +expected=50540000010050540000000508004500003c00000000400666ba0a0000020a00000100080009000000000000000050100000e8e20000deaddeaddeaddeaddeaddeaddeaddeaddeaddead
> +actual=$(ovs-appctl ofproto/hexify ${flow_s} ${l7payload})
> +AT_CHECK([test ${expected} = ${actual}])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
