On Wed, Nov 1, 2023 at 5:42 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
> On 10/24/23 19:04, Ihar Hrachyshka wrote:
> > With --hexified, it will produce a bare hexified payload with no spaces
> > or offset indicators inserted, which is useful in tests to produce
> > frames to pass to e.g. `receive`.
>
> Hmm. I though we agreed on just allowing ovs-pcap to read from stdin,
> so something like this is possible:
>
> ovs-ofctl compose-packet --pcap ${flow_expected} | ovs-pcap -
>
> instead of adding an extra argument to the ovs-ofctl. Or is there a
> reason to not do that?
>
>
The primary reason is that it's unclear to me why we have to 1) generate a
pcap; 2) initialize a python interpreter* to parse it back into binary ...
for something that we could do in-place since we already have the packet
and already "print" it (albeit in "hex_dump" format that is hard to
machine-read).
* - which takes a second of cpu time at times.
Let me know what you think though.
> >
> > With --bad-csum, it will produce a frame that has an invalid IP checksum
> > (applicable to IPv4 only because IPv6 doesn't have checksums.)
>
> This is fine.
>
> >
> > The command is now more useful in tests, where we may need to produce
> > hex frame payloads to compare observed frames against.
> >
> > As an example of the tool use, a single test case is converted to it.
> > The test uses both normal --hexified and --bad-csum behaviors of the
> > command, confirming they work as advertised.
> >
> > Signed-off-by: Ihar Hrachyshka <ihrac...@redhat.com>
> > ---
> > v1: - initial version.
> > v2: - convert from appctl to ovstest.
> > - add --bad-csum support.
> > - drop separate test case and man for the tool.
> > v3: - fix build warnings on restricted ovs_be16 ++.
> > v4: - migrate to compose-packet.
> > ---
> > lib/flow.c | 16 ++++++++++--
> > lib/flow.h | 2 +-
> > lib/netdev-dummy.c | 4 +--
> > ofproto/ofproto-dpif-trace.c | 2 +-
> > ofproto/ofproto-dpif.c | 4 +--
> > tests/dpif-netdev.at | 45 ++++++++++++++++------------------
> > utilities/ovs-ofctl.c | 47 ++++++++++++++++++++++++++++++------
> > 7 files changed, 80 insertions(+), 40 deletions(-)
> >
> > diff --git a/lib/flow.c b/lib/flow.c
> > index fe226cf0f..cd7832710 100644
> > --- a/lib/flow.c
> > +++ b/lib/flow.c
> > @@ -3306,6 +3306,8 @@ packet_expand(struct dp_packet *p, const struct
> flow *flow, size_t size)
> > * (This is useful only for testing, obviously, and the packet isn't
> really
> > * valid. Lots of fields are just zeroed.)
> > *
> > + * If 'bad_csum' is true, the final IP checksum is invalid.
> > + *
> > * For packets whose protocols can encapsulate arbitrary L7 payloads,
> 'l7' and
> > * 'l7_len' determine that payload:
> > *
> > @@ -3318,7 +3320,7 @@ packet_expand(struct dp_packet *p, const struct
> flow *flow, size_t size)
> > * from 'l7'. */
> > void
> > flow_compose(struct dp_packet *p, const struct flow *flow,
> > - const void *l7, size_t l7_len)
> > + const void *l7, size_t l7_len, bool bad_csum)
> > {
> > /* Add code to this function (or its callees) for emitting new
> fields or
> > * protocols. (This isn't essential, so it can be skipped for
> initial
> > @@ -3370,7 +3372,17 @@ flow_compose(struct dp_packet *p, const struct
> flow *flow,
> > /* Checksum has already been zeroed by put_zeros call. */
> > ip->ip_csum = csum(ip, sizeof *ip);
> >
> > - dp_packet_ol_set_ip_csum_good(p);
> > + if (bad_csum) {
> > + /* Internet checksum is a sum complement to zero, so any
> other
> > + * value will result in an invalid checksum. Here, we flip
> one
> > + * bit.
> > + */
>
> Nit: I'd suggest to either put both the '/*' and '*/' on the lines with
> a text, or put both on their own empty lines.
>
> > + ip->ip_csum ^= (OVS_FORCE ovs_be16) 0x1;
> > + dp_packet_ip_checksum_bad(p);
> > + } else {
> > + dp_packet_ol_set_ip_csum_good(p);
> > + }
> > +
> > pseudo_hdr_csum = packet_csum_pseudoheader(ip);
> > flow_compose_l4_csum(p, flow, pseudo_hdr_csum);
> > } else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
> > diff --git a/lib/flow.h b/lib/flow.h
> > index a9d026e1c..75a9be3c1 100644
> > --- a/lib/flow.h
> > +++ b/lib/flow.h
> > @@ -127,7 +127,7 @@ void flow_set_mpls_bos(struct flow *, int idx,
> uint8_t stack);
> > void flow_set_mpls_lse(struct flow *, int idx, ovs_be32 lse);
> >
> > void flow_compose(struct dp_packet *, const struct flow *,
> > - const void *l7, size_t l7_len);
> > + const void *l7, size_t l7_len, bool bad_csum);
> > void packet_expand(struct dp_packet *, const struct flow *, size_t
> size);
> >
> > bool parse_ipv6_ext_hdrs(const void **datap, size_t *sizep, uint8_t
> *nw_proto,
> > diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> > index 1a54add87..9ead449e1 100644
> > --- a/lib/netdev-dummy.c
> > +++ b/lib/netdev-dummy.c
> > @@ -1758,7 +1758,7 @@ eth_from_flow_str(const char *s, size_t
> packet_size,
> >
> > packet = dp_packet_new(0);
> > if (packet_size) {
> > - flow_compose(packet, flow, NULL, 0);
> > + flow_compose(packet, flow, NULL, 0, false);
> > if (dp_packet_size(packet) < packet_size) {
> > packet_expand(packet, flow, packet_size);
> > } else if (dp_packet_size(packet) > packet_size){
> > @@ -1766,7 +1766,7 @@ eth_from_flow_str(const char *s, size_t
> packet_size,
> > packet = NULL;
> > }
> > } else {
> > - flow_compose(packet, flow, NULL, 64);
> > + flow_compose(packet, flow, NULL, 64, false);
> > }
> >
> > ofpbuf_uninit(&odp_key);
> > diff --git a/ofproto/ofproto-dpif-trace.c b/ofproto/ofproto-dpif-trace.c
> > index 527e2f17e..b86e7fe07 100644
> > --- a/ofproto/ofproto-dpif-trace.c
> > +++ b/ofproto/ofproto-dpif-trace.c
> > @@ -440,7 +440,7 @@ parse_flow_and_packet(int argc, const char *argv[],
> > if (generate_packet) {
> > /* Generate a packet, as requested. */
> > packet = dp_packet_new(0);
> > - flow_compose(packet, flow, l7, l7_len);
> > + flow_compose(packet, flow, l7, l7_len, false);
> > } else if (packet) {
> > /* Use the metadata from the flow and the packet argument to
> > * reconstruct the flow. */
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index ba5706f6a..9e8faf829 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -1255,7 +1255,7 @@ check_ct_eventmask(struct dpif_backer *backer)
> >
> > /* Compose a dummy UDP packet. */
> > dp_packet_init(&packet, 0);
> > - flow_compose(&packet, &flow, NULL, 64);
> > + flow_compose(&packet, &flow, NULL, 64, false);
> >
> > /* Execute the actions. On older datapaths this fails with EINVAL,
> on
> > * newer datapaths it succeeds. */
> > @@ -1348,7 +1348,7 @@ check_ct_timeout_policy(struct dpif_backer *backer)
> >
> > /* Compose a dummy UDP packet. */
> > dp_packet_init(&packet, 0);
> > - flow_compose(&packet, &flow, NULL, 64);
> > + flow_compose(&packet, &flow, NULL, 64, false);
> >
> > /* Execute the actions. On older datapaths this fails with EINVAL,
> on
> > * newer datapaths it succeeds. */
> > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> > index 85119fb81..2551f3366 100644
> > --- a/tests/dpif-netdev.at
> > +++ b/tests/dpif-netdev.at
> > @@ -746,19 +746,25 @@ OVS_VSWITCHD_START(
> > # Modify the ip_dst addr to force changing the IP csum.
> > AT_CHECK([ovs-ofctl add-flow br1
> in_port=p1,actions=mod_nw_dst:192.168.1.1,output:p2])
> >
> > +flow_s="\
> > +eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x0800,\
> >
> +nw_src=192.168.123.2,nw_dst=192.168.123.1,nw_proto=6,nw_ttl=64,nw_frag=no,\
> > +tp_src=54392,tp_dst=5201,tcp_flags=ack"
>
> OpenFlow parser allows extra spaces, so there is no need to start these
> lines right from the beginning of the line. Some small indent may be
> useful.
>
> > +
> > +good_frame=$(ovs-ofctl compose-packet --hexified ${flow_s})
> > +
> > # Check if no offload remains ok.
> > AT_CHECK([ovs-vsctl set Interface p2 options:tx_pcap=p2.pcap])
> > AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=false])
> > AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=false])
> > -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
> >
> -0a8f394fe0738abf7e2f058408004500003433e0400040068f8fc0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd00000101080a2524d2345c7fe1c4
> > -])
> > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${good_frame}])
> >
> > # Checksum should change to 0x990 with ip_dst changed to 192.168.1.1
> > # by the datapath while processing the packet.
> > +flow_expected=$(echo ${flow_s} | sed 's/192.168.123.1/192.168.1.1/g')
> > +good_expected=$(ovs-ofctl compose-packet --hexified ${flow_expected})
> > AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> > -AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> >
> -0a8f394fe0738abf7e2f058408004500003433e0400040060990c0a87b02c0a80101d4781451a962ad5417ed297b801000e5c1fd00000101080a2524d2345c7fe1c4
> > +AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${good_expected}
> > ])
> >
> > # Check if packets entering the datapath with csum offloading
> > @@ -766,12 +772,9 @@ AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> > # in the datapath and not by the netdev.
> > AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=false])
> > AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=true])
> > -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
> >
> -0a8f394fe0738abf7e2f058408004500003433e0400040068f8fc0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd00000101080a2524d2345c7fe1c4
> > -])
> > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${good_frame}])
> > AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> > -AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> >
> -0a8f394fe0738abf7e2f058408004500003433e0400040060990c0a87b02c0a80101d4781451a962ad5417ed297b801000e5c1fd00000101080a2524d2345c7fe1c4
> > +AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${good_expected}
> > ])
> >
> > # Check if packets entering the datapath with csum offloading
> > @@ -779,36 +782,30 @@ AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> > # by the datapath.
> > AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=true])
> > AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=true])
> > -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
> >
> -0a8f394fe0738abf7e2f058408004500003433e0400040068f8fc0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd00000101080a2524d2345c7fe1c4
> > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${good_frame}
> > ])
> > AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> > -AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> >
> -0a8f394fe0738abf7e2f058408004500003433e0400040060990c0a87b02c0a80101d4781451a962ad5417ed297b801000e5c1fd00000101080a2524d2345c7fe1c4
> > +AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${good_expected}
> > ])
> >
> > # Push a packet with bad csum and offloading disabled to check
> > # if the datapath updates the csum, but does not fix the issue.
> > +bad_frame=$(ovs-ofctl compose-packet --hexified --bad-csum ${flow_s})
> > AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=false])
> > AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=false])
> > -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
> >
> -0a8f394fe0738abf7e2f058408004500003433e0400040068f03c0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd00000101080a2524d2345c7fe1c4
> > -])
> > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${bad_frame}])
> > AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> > -AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> >
> -0a8f394fe0738abf7e2f058408004500003433e0400040060904c0a87b02c0a80101d4781451a962ad5417ed297b801000e5c1fd00000101080a2524d2345c7fe1c4
> > +bad_expected=$(ovs-ofctl compose-packet --hexified --bad-csum
> ${flow_expected})
> > +AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${bad_expected}
> > ])
> >
> > # Push a packet with bad csum and offloading enabled to check
> > # if the driver updates and fixes the csum.
> > AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=true])
> > AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=true])
> > -AT_CHECK([ovs-appctl netdev-dummy/receive p1 \
> >
> -0a8f394fe0738abf7e2f058408004500003433e0400040068f03c0a87b02c0a87b01d4781451a962ad5417ed297b801000e547fd00000101080a2524d2345c7fe1c4
> > -])
> > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${bad_frame}])
> > AT_CHECK([ovs-pcap p2.pcap > p2.pcap.txt 2>&1])
> > -AT_CHECK([tail -n 1 p2.pcap.txt], [0], [dnl
> >
> -0a8f394fe0738abf7e2f058408004500003433e0400040060990c0a87b02c0a80101d4781451a962ad5417ed297b801000e5c1fd00000101080a2524d2345c7fe1c4
> > +AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0], [${good_expected}
> > ])
> > OVS_VSWITCHD_STOP
> > AT_CLEANUP
> > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> > index 24d0941cf..1292beaff 100644
> > --- a/utilities/ovs-ofctl.c
> > +++ b/utilities/ovs-ofctl.c
> > @@ -154,6 +154,12 @@ static int show_stats = 1;
> > /* --pcap: Makes "compose-packet" print a pcap on stdout. */
> > static int print_pcap = 0;
> >
> > +/* --hexified: Makes "compose-packet" print a bare hexified payload. */
> > +static int print_hexified = 0;
> > +
> > +/* -bad-csum: Makes "compose-packet" generate an invalid checksum. */
> > +static int bad_csum = 0;
> > +
> > /* --raw: Makes "ofp-print" read binary data from stdin. */
> > static int raw = 0;
> >
> > @@ -243,6 +249,8 @@ parse_options(int argc, char *argv[])
> > {"color", optional_argument, NULL, OPT_COLOR},
> > {"may-create", no_argument, NULL, OPT_MAY_CREATE},
> > {"pcap", no_argument, &print_pcap, 1},
> > + {"hexified", no_argument, &print_hexified, 1},
> > + {"bad-csum", no_argument, &bad_csum, 1},
> > {"raw", no_argument, &raw, 1},
> > {"read-only", no_argument, NULL, OPT_READ_ONLY},
> > DAEMON_LONG_OPTIONS,
> > @@ -4948,20 +4956,33 @@ ofctl_parse_key_value(struct ovs_cmdl_context
> *ctx)
> > }
> > }
> >
> > -/* "compose-packet [--pcap] FLOW [L7]": Converts the OpenFlow flow
> > - * specification FLOW to a packet with flow_compose() and prints the
> hex bytes
> > - * in the packet on stdout. Also verifies that the flow extracted from
> that
> > - * packet matches the original FLOW.
> > +/* "compose-packet [--pcap] [--hexified] [--bad-csum] FLOW [L7]":
> Converts the
> > + * OpenFlow flow specification FLOW to a packet with flow_compose() and
> prints
> > + * the hex bytes of the packet, with offsets, to stdout.
> > + *
> > + * With --pcap, prints the packet in pcap format, so that you can do
> something
> > + * like "ovs-ofctl --pcap compose-packet udp | tcpdump -vvvv -r-" to use
> > + * another tool to dump the packet contents.
> > + *
> > + * With --hexified, prints the packet as a single bare hex string with
> no
> > + * spaces or offsets, so that you can pass the result directly to e.g.
> > + * "ovs-appctl netdev-dummy/receive vif $(ovs-ofctl compose-packet
> --hexified
> > + * FLOW)"
> > + *
> > + * With --bad-csum, produces a packet with an invalid IP checksum. (For
> IPv4.)
> > *
> > - * With --pcap, prints the packet to stdout instead as a pcap file, so
> that you
> > - * can do something like "ovs-ofctl --pcap compose-packet udp | tcpdump
> -vvvv
> > - * -r-" to use another tool to dump the packet contents.
> > + * Regardless of the mode, the command also verifies that the flow
> extracted
> > + * from that packet matches the original FLOW.
> > *
> > * If L7 is specified, draws the L7 payload data from it, otherwise
> defaults to
> > * 64 bytes of payload. */
> > static void
> > ofctl_compose_packet(struct ovs_cmdl_context *ctx)
> > {
> > + if (print_pcap && print_hexified) {
> > + ovs_fatal(1, "--hexified and --pcap are mutually exclusive");
> > + }
> > +
> > if (print_pcap && isatty(STDOUT_FILENO)) {
> > ovs_fatal(1, "not writing pcap data to stdout; redirect to a
> file "
> > "or pipe to tcpdump instead");
> > @@ -4989,7 +5010,7 @@ ofctl_compose_packet(struct ovs_cmdl_context *ctx)
> > l7_len = dp_packet_size(&payload);
> > l7 = dp_packet_steal_data(&payload);
> > }
> > - flow_compose(&p, &flow1, l7, l7_len);
> > + flow_compose(&p, &flow1, l7, l7_len, bad_csum);
> > free(l7);
> >
> > if (print_pcap) {
> > @@ -4997,6 +5018,16 @@ ofctl_compose_packet(struct ovs_cmdl_context *ctx)
> > ovs_pcap_write_header(p_file);
> > ovs_pcap_write(p_file, &p);
> > ovs_pcap_close(p_file);
> > + } else if (print_hexified) {
> > + /* Binary to hex string. */
> > + for (int i = 0; i < dp_packet_size(&p); i++) {
> > + uint8_t val = ((uint8_t *) dp_packet_data(&p))[i];
> > + /* Don't use ds_put_hex because it adds 0x prefix as well as
> > + * it doesn't guarantee even number of payload characters,
> which is
> > + * important elsewhere (e.g. in `receive` command. */
>
> 'receive' is a little out of context here, because it's not an ofctl
> command.
>
> > + printf("%02" PRIx8, val);
> > + }
> > +
> > } else {
> > ovs_hex_dump(stdout, dp_packet_data(&p), dp_packet_size(&p), 0,
> false);
> > }
> > --
> > 2.41.0
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
