Thank you. I applied this to master.
On Mon, Jul 30, 2012 at 02:16:37PM -0700, Mehak Mahajan wrote: > Hey Ben, > > Looks good to me. > > thanx! > mehak > > On Fri, Jul 27, 2012 at 1:23 PM, Ben Pfaff <b...@nicira.com> wrote: > > > 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 > > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
