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.sherw...@bigswitch.com> > Signed-off-by: Ben Pfaff <b...@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 > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
