On 11/14/23 12:39, Ilya Maximets wrote: > On 11/2/23 19:02, Ihar Hrachyshka wrote: >> On Wed, Nov 1, 2023 at 5:42 PM Ilya Maximets <[email protected] >> <mailto:[email protected]>> 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.
Also, please, add 'ovs-ofctl: ' to the patch as a subject prefix, since it is a primary target of the change. > > 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 <[email protected] >> <mailto:[email protected]>> >> > --- >> > 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
