On 11/2/23 19:02, Ihar Hrachyshka wrote: > On Wed, Nov 1, 2023 at 5:42 PM Ilya Maximets <i.maxim...@ovn.org > <mailto: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).
OK. It makes some sense. Let;s keep the option. Though, we should probably rename it to something else. The current output is already a hex dump, so the 'hexified' is not a very intuitive name. Maybe call it '--oneline' or something like that? What do you think? I also thought about averloading the '--raw', but it usually means binary output, so doesn't fit well. Best regards, Ilya Maximets. > > * - 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 > <mailto: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 <http://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 <http://dpif-netdev.at> > b/tests/dpif-netdev.at <http://dpif-netdev.at> > > index 85119fb81..2551f3366 100644 > > --- a/tests/dpif-netdev.at <http://dpif-netdev.at> > > +++ b/tests/dpif-netdev.at <http://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 > <http://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
