[ovs-dev] [PATCH] tests: Test that ofp10_match bytes that should be ignored really are.
Ben Pfaff
blp at nicira.com
Fri Jul 27 20:23:15 UTC 2012
Would someone review this please? It should not be hard.
Thanks,
Ben.
On Sat, Jul 21, 2012 at 09:56:28AM -0700, Ben Pfaff wrote:
> Rob Sherwood reported a bug in OVS treatment of ofp10_match bytes that
> should be ignored some time ago:
>
> > In any case, the pktact.SingleWildcardMatch and
> > pktact.AllExceptOneWildcardMatch tests were failing because it looks
> > like OVS (v1.4 release) was not matching vlan tagged packets when the
> > match wildcarded vlan but the dl_vlan value (which should be ignored,
> > because it is wildcarded) was non-zero. We've worked around this in
> > OFTest by making sure that the dl_vlan value is zero when vlan is
> > wildcarded and now the test passes.
> >
> > In other words:
> >
> > if (ofp_match->wildcards&OFPFW_DL_VLAN) is true, then the match should
> > match both tagged and untagged packets, independent of the value of
> > ofp_match->dl_vlan. OVS (seemingly) only matches tagged packets if
> > ofp_match->dl_vlan == 0.
>
> I wasn't able to spot the problem at the time, and I still don't see a
> problem (perhaps it has been fixed since then), but this commit should
> prevent any regression for this specific problem and for anything like it.
>
> It would be natural to modify the parse-ofp11-match test in the same way,
> but this commit doesn't do it.
>
> Rob's original bug report is at:
> https://mailman.stanford.edu/pipermail/openflow-discuss/2012-March/003107.html
>
> Reported-by: Rob Sherwood <rob.sherwood at bigswitch.com>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
> tests/ovs-ofctl.at | 136 ++++++++++++++++++++++++------------------------
> utilities/ovs-ofctl.c | 40 +++++++++++++--
> 2 files changed, 104 insertions(+), 72 deletions(-)
>
> diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
> index 08026ec..9f47f78 100644
> --- a/tests/ovs-ofctl.at
> +++ b/tests/ovs-ofctl.at
> @@ -730,55 +730,55 @@ AT_SETUP([ovs-ofctl parse-ofp10-match])
> AT_KEYWORDS([OF1.0])
> AT_DATA([test-data], [dnl
> # in_port=65534
> -003820fe fffe 000000000000 000000000000 0000 00 00 0000 00 00 0000 dnl
> -00000000 00000000 0000 0000
> +003820fe fffe xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx xxxx xx xx xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> # dl_src=00:01:02:03:04:05
> -003820fb 0000 000102030405 000000000000 0000 00 00 0000 00 00 0000 dnl
> -00000000 00000000 0000 0000
> +003820fb xxxx 000102030405 xxxxxxxxxxxx xxxx xx xx xxxx xx xx xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> # dl_dst=10:20:30:40:50:60
> -003820f7 0000 000000000000 102030405060 0000 00 00 0000 00 00 0000 dnl
> -00000000 00000000 0000 0000
> +003820f7 xxxx xxxxxxxxxxxx 102030405060 xxxx xx xx xxxx xx xx xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> # dl_vlan=291
> -003820fd 0000 000000000000 000000000000 0123 00 00 0000 00 00 0000 dnl
> -00000000 00000000 0000 0000
> +003820fd xxxx xxxxxxxxxxxx xxxxxxxxxxxx 0123 xx xx xxxx xx xx xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> # dl_vlan_pcp=5
> -002820ff 0000 000000000000 000000000000 0000 05 00 0000 00 00 0000 dnl
> -00000000 00000000 0000 0000
> +002820ff xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx 05 xx xxxx xx xx xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> # dl_vlan=291,dl_vlan_pcp=4
> -002820fd 0000 000000000000 000000000000 0123 04 00 0000 00 00 0000 dnl
> -00000000 00000000 0000 0000
> +002820fd xxxx xxxxxxxxxxxx xxxxxxxxxxxx 0123 04 xx xxxx xx xx xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> # vlan_tci=0x0000
> -003820fd 0000 000000000000 000000000000 ffff 00 00 0000 00 00 0000 dnl
> -00000000 00000000 0000 0000
> +003820fd xxxx xxxxxxxxxxxx xxxxxxxxxxxx ffff xx xx xxxx xx xx xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> dnl dl_vlan_pcp doesn't make sense when dl_vlan is "none", so
> dnl OVS ignores it and drops it on output.
> # vlan_tci=0x0000
> # 1: 28 -> 38
> # 20: 05 -> 00
> -002820fd 0000 000000000000 000000000000 ffff 05 00 0000 00 00 0000 dnl
> -00000000 00000000 0000 0000
> +002820fd xxxx xxxxxxxxxxxx xxxxxxxxxxxx ffff 05 xx xxxx xx xx xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> dnl Invalid VID and PCP discards out-of-range bits:
> # dl_vlan=256,dl_vlan_pcp=7
> # 18: f1 -> 01
> # 20: ff -> 07
> -002820fd 0000 000000000000 000000000000 f100 ff 00 0000 00 00 0000 dnl
> -00000000 00000000 0000 0000
> +002820fd xxxx xxxxxxxxxxxx xxxxxxxxxxxx f100 ff xx xxxx xx xx xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> # dl_type=0x1234
> -003820ef 0000 000000000000 000000000000 0000 00 00 1234 00 00 0000 dnl
> -00000000 00000000 0000 0000
> +003820ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 1234 xx xx xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> # ip,nw_proto=5
> -003820cf 0000 000000000000 000000000000 0000 00 00 0800 00 05 0000 dnl
> -00000000 00000000 0000 0000
> +003820cf xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 xx 05 xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> dnl Ignore nw_proto if not IP or ARP:
> # dl_type=0x1234,nw_proto=5
> @@ -787,12 +787,12 @@ dnl Ignore nw_proto if not IP or ARP:
> & ofp_util|INFO|normalization changed ofp_match, details:
> & ofp_util|INFO| pre: dl_type=0x1234,nw_proto=5
> & ofp_util|INFO|post: dl_type=0x1234
> -003820cf 0000 000000000000 000000000000 0000 00 00 1234 00 05 0000 dnl
> -00000000 00000000 0000 0000
> +003820cf xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 1234 xx 05 xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> # ip,nw_tos=252
> -001820ef 0000 000000000000 000000000000 0000 00 00 0800 fc 00 0000 dnl
> -00000000 00000000 0000 0000
> +001820ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 fc xx xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> dnl Ignore nw_tos if not IP:
> # arp,nw_tos=4
> @@ -802,54 +802,54 @@ dnl Ignore nw_tos if not IP:
> & ofp_util|INFO|normalization changed ofp_match, details:
> & ofp_util|INFO| pre: arp,nw_tos=4
> & ofp_util|INFO|post: arp
> -001820ef 0000 000000000000 000000000000 0000 00 00 0806 05 00 0000 dnl
> -00000000 00000000 0000 0000
> +001820ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0806 05 xx xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> dnl Low 2 bits of invalid TOS are forced to 0:
> # ip,nw_tos=48
> # 24: 31 -> 30
> -001820ef 0000 000000000000 000000000000 0000 00 00 0800 31 00 0000 dnl
> -00000000 00000000 0000 0000
> +001820ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 31 xx xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> # arp,arp_op=2
> -003820cf 0000 000000000000 000000000000 0000 00 00 0806 00 02 0000 dnl
> -00000000 00000000 0000 0000
> +003820cf xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0806 xx 02 xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx xxxx
>
> # ip,nw_src=192.168.128.85
> -003800ef 0000 000000000000 000000000000 0000 00 00 0800 00 00 0000 dnl
> -c0a88055 00000000 0000 0000
> +003800ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 xx xx xxxx dnl
> +c0a88055 xxxxxxxx xxxx xxxx
>
> # ip,nw_src=192.168.128.0/24
> # 31: 55 -> 00
> -003808ef 0000 000000000000 000000000000 0000 00 00 0800 00 00 0000 dnl
> -c0a88055 00000000 0000 0000
> +003808ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 xx xx xxxx dnl
> +c0a88055 xxxxxxxx xxxx xxxx
>
> # ip,nw_dst=192.168.128.85
> -003020ef 0000 000000000000 000000000000 0000 00 00 0800 00 00 0000 dnl
> -00000000 c0a88055 0000 0000
> +003020ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 xx xx xxxx dnl
> +xxxxxxxx c0a88055 xxxx xxxx
>
> # ip,nw_dst=192.168.128.0/24
> # 35: 55 -> 00
> -003220ef 0000 000000000000 000000000000 0000 00 00 0800 00 00 0000 dnl
> -00000000 c0a88055 0000 0000
> +003220ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 xx xx xxxx dnl
> +xxxxxxxx c0a88055 xxxx xxxx
>
> # arp,nw_src=192.168.128.85
> -003800ef 0000 000000000000 000000000000 0000 00 00 0806 00 00 0000 dnl
> -c0a88055 00000000 0000 0000
> +003800ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0806 xx xx xxxx dnl
> +c0a88055 xxxxxxxx xxxx xxxx
>
> # arp,nw_src=192.168.128.0/24
> # 31: 55 -> 00
> -003808ef 0000 000000000000 000000000000 0000 00 00 0806 00 00 0000 dnl
> -c0a88055 00000000 0000 0000
> +003808ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0806 xx xx xxxx dnl
> +c0a88055 xxxxxxxx xxxx xxxx
>
> # arp,nw_dst=192.168.128.85
> -003020ef 0000 000000000000 000000000000 0000 00 00 0806 00 00 0000 dnl
> -00000000 c0a88055 0000 0000
> +003020ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0806 xx xx xxxx dnl
> +xxxxxxxx c0a88055 xxxx xxxx
>
> # arp,nw_dst=192.168.128.0/24
> # 35: 55 -> 00
> -003220ef 0000 000000000000 000000000000 0000 00 00 0806 00 00 0000 dnl
> -00000000 c0a88055 0000 0000
> +003220ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0806 xx xx xxxx dnl
> +xxxxxxxx c0a88055 xxxx xxxx
>
> dnl Ignore nw_src if not IP or ARP:
> # dl_type=0x1234,nw_src=192.168.128.0/24
> @@ -861,8 +861,8 @@ dnl Ignore nw_src if not IP or ARP:
> & ofp_util|INFO|normalization changed ofp_match, details:
> & ofp_util|INFO| pre: dl_type=0x1234,nw_src=192.168.128.0/24
> & ofp_util|INFO|post: dl_type=0x1234
> -003808ef 0000 000000000000 000000000000 0000 00 00 1234 00 00 0000 dnl
> -c0a88055 00000000 0000 0000
> +003808ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 1234 xx xx xxxx dnl
> +c0a88055 xxxxxxxx xxxx xxxx
>
> dnl Ignore nw_dst if not IP or ARP:
> # dl_type=0x1234,nw_dst=192.168.128.0/24
> @@ -874,32 +874,32 @@ dnl Ignore nw_dst if not IP or ARP:
> & ofp_util|INFO|normalization changed ofp_match, details:
> & ofp_util|INFO| pre: dl_type=0x1234,nw_dst=192.168.128.0/24
> & ofp_util|INFO|post: dl_type=0x1234
> -003220ef 0000 000000000000 000000000000 0000 00 00 1234 00 00 0000 dnl
> -00000000 c0a88055 0000 0000
> +003220ef xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 1234 xx xx xxxx dnl
> +xxxxxxxx c0a88055 xxxx xxxx
>
> # tcp,tp_src=443
> -0038208f 0000 000000000000 000000000000 0000 00 00 0800 00 06 0000 dnl
> -00000000 00000000 01bb 0000
> +0038208f xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 xx 06 xxxx dnl
> +xxxxxxxx xxxxxxxx 01bb xxxx
>
> # tcp,tp_dst=443
> -0038204f 0000 000000000000 000000000000 0000 00 00 0800 00 06 0000 dnl
> -00000000 00000000 0000 01bb
> +0038204f xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 xx 06 xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx 01bb
>
> # udp,tp_src=443
> -0038208f 0000 000000000000 000000000000 0000 00 00 0800 00 11 0000 dnl
> -00000000 00000000 01bb 0000
> +0038208f xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 xx 11 xxxx dnl
> +xxxxxxxx xxxxxxxx 01bb xxxx
>
> # udp,tp_dst=443
> -0038204f 0000 000000000000 000000000000 0000 00 00 0800 00 11 0000 dnl
> -00000000 00000000 0000 01bb
> +0038204f xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 xx 11 xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx 01bb
>
> # icmp,icmp_type=5
> -0038208f 0000 000000000000 000000000000 0000 00 00 0800 00 01 0000 dnl
> -00000000 00000000 0005 0000
> +0038208f xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 xx 01 xxxx dnl
> +xxxxxxxx xxxxxxxx 0005 xxxx
>
> # icmp,icmp_code=8
> -0038204f 0000 000000000000 000000000000 0000 00 00 0800 00 01 0000 dnl
> -00000000 00000000 0000 0008
> +0038204f xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 xx 01 xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx 0008
>
> dnl Ignore tp_src if not TCP or UDP:
> # ip,nw_proto=21,tp_src=443
> @@ -909,8 +909,8 @@ dnl Ignore tp_src if not TCP or UDP:
> & ofp_util|INFO|normalization changed ofp_match, details:
> & ofp_util|INFO| pre: ip,nw_proto=21,tp_src=443
> & ofp_util|INFO|post: ip,nw_proto=21
> -0038208f 0000 000000000000 000000000000 0000 00 00 0800 00 15 0000 dnl
> -00000000 00000000 01bb 0000
> +0038208f xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 xx 15 xxxx dnl
> +xxxxxxxx xxxxxxxx 01bb xxxx
>
> dnl Ignore tp_dst if not TCP or UDP:
> # ip,nw_proto=21,tp_dst=443
> @@ -918,8 +918,8 @@ dnl Ignore tp_dst if not TCP or UDP:
> # normal: 38: 01 -> 00
> # normal: 39: bb -> 00
> dnl The normalization details are suppressed here due to rate-limiting.
> -0038204f 0000 000000000000 000000000000 0000 00 00 0800 00 15 0000 dnl
> -00000000 00000000 0000 01bb
> +0038204f xxxx xxxxxxxxxxxx xxxxxxxxxxxx xxxx xx xx 0800 xx 15 xxxx dnl
> +xxxxxxxx xxxxxxxx xxxx 01bb
>
> ])
> sed '/^[[#&]]/d' < test-data > input.txt
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index e5c5255..78983f2 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -2315,20 +2315,50 @@ ofctl_parse_ofp10_actions(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> /* "parse-ofp10-match": reads a series of ofp10_match specifications as hex
> * bytes from stdin, converts them to cls_rules, prints them as strings on
> * stdout, and then converts them back to hex bytes and prints any differences
> - * from the input. */
> + * from the input.
> + *
> + * The input hex bytes may contain "x"s to represent "don't-cares", bytes whose
> + * values are ignored in the input and will be set to zero when OVS converts
> + * them back to hex bytes. ovs-ofctl actually sets "x"s to random bits when
> + * it does the conversion to hex, to ensure that in fact they are ignored. */
> static void
> ofctl_parse_ofp10_match(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> {
> + struct ds expout;
> struct ds in;
>
> ds_init(&in);
> + ds_init(&expout);
> while (!ds_get_preprocessed_line(&in, stdin)) {
> - struct ofpbuf match_in;
> + struct ofpbuf match_in, match_expout;
> struct ofp10_match match_out;
> struct ofp10_match match_normal;
> struct cls_rule rule;
> + char *p;
> +
> + /* Parse hex bytes to use for expected output. */
> + ds_clear(&expout);
> + ds_put_cstr(&expout, ds_cstr(&in));
> + for (p = ds_cstr(&expout); *p; p++) {
> + if (*p == 'x') {
> + *p = '0';
> + }
> + }
> + ofpbuf_init(&match_expout, 0);
> + if (ofpbuf_put_hex(&match_expout, ds_cstr(&expout), NULL)[0] != '\0') {
> + ovs_fatal(0, "Trailing garbage in hex data");
> + }
> + if (match_expout.size != sizeof(struct ofp10_match)) {
> + ovs_fatal(0, "Input is %zu bytes, expected %zu",
> + match_expout.size, sizeof(struct ofp10_match));
> + }
>
> - /* Parse hex bytes. */
> + /* Parse hex bytes for input. */
> + for (p = ds_cstr(&in); *p; p++) {
> + if (*p == 'x') {
> + *p = "0123456789abcdef"[random_uint32() & 0xf];
> + }
> + }
> ofpbuf_init(&match_in, 0);
> if (ofpbuf_put_hex(&match_in, ds_cstr(&in), NULL)[0] != '\0') {
> ovs_fatal(0, "Trailing garbage in hex data");
> @@ -2345,7 +2375,7 @@ ofctl_parse_ofp10_match(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
>
> /* Convert back to ofp10_match and print differences from input. */
> ofputil_cls_rule_to_ofp10_match(&rule, &match_out);
> - print_differences("", match_in.data, match_in.size,
> + print_differences("", match_expout.data, match_expout.size,
> &match_out, sizeof match_out);
>
> /* Normalize, then convert and compare again. */
> @@ -2356,8 +2386,10 @@ ofctl_parse_ofp10_match(int argc OVS_UNUSED, char *argv[] OVS_UNUSED)
> putchar('\n');
>
> ofpbuf_uninit(&match_in);
> + ofpbuf_uninit(&match_expout);
> }
> ds_destroy(&in);
> + ds_destroy(&expout);
> }
>
> /* "parse-ofp11-match": reads a series of ofp11_match specifications as hex
> --
> 1.7.2.5
>
More information about the dev
mailing list