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?
>
> 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]>
> ---
> 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
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
