[ovs-dev] [PATCH] tests: Test that ofp10_match bytes that should be ignored really are.
Ben Pfaff
blp at nicira.com
Tue Aug 7 22:37:13 UTC 2012
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 <blp at 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.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
> > >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
> >
More information about the dev
mailing list