Those problems are different and not related to this change so it is confusing to have it here.
On Mon, Aug 5, 2013 at 12:42 PM, Andy Zhou <[email protected]> wrote: > Ben's original commit message gives more background to the netlink protocol > changes we have been making. It would be nice to keep them as well. > > > On Mon, Aug 5, 2013 at 12:34 PM, Ben Pfaff <[email protected]> wrote: >> >> On Mon, Aug 05, 2013 at 11:22:49AM -0700, Jesse Gross wrote: >> > This information is factually correct but it's not really related to >> > the change in this commit. >> > >> > The problem here is very simple: the current protocol allows a default >> > value of zero if either mark or priority is not specified (this >> > actually is part of the ABI). When userspace serializes either the >> > value or mask it looks at the value and omits the netlink attribute if >> > it is zero. This is a bug because an exact match on zero turns into a >> > wildcard of the field. >> > >> > These two fields (plus input port and EtherType) are special because >> > they can be omitted whereas most other values are required to be fully >> > specified. These protocol variations tend to cause bugs (as above) >> > when we evolve the protocol because an exception that makes sense in >> > one context might not be logical in another. Since the default value >> > for mark and priority are merely shorthands, we can push the protocol >> > in a more consistent direction by ignoring the shortcut and always >> > serializing the values. >> >> Thanks, like this? >> >> --8<--------------------------cut here-------------------------->8-- >> >> From: Andy Zhou <[email protected]> >> Date: Sat, 3 Aug 2013 12:23:15 -0700 >> Subject: [PATCH] odp-util: Always export the priority and skb_mark netlink >> attributes. >> >> The current Netlink protocol allows a default value of zero if either mark >> or priority is not specified (this is part of the ABI). Until now, when >> userspace serializes either the value or mask, it looked at the value and >> omitted the netlink attribute if it is zero. This is a bug because an >> exact match on zero turns into a wildcard of the field. >> >> These two fields (plus input port and EtherType) are special because they >> can be omitted whereas most other values are required to be fully >> specified. These protocol variations tend to cause bugs (as above) when >> we >> evolve the protocol because an exception that makes sense in one context >> might not be logical in another. Since the default value for mark and >> priority are merely shorthands, we can push the protocol in a more >> consistent direction by ignoring the shortcut and always serializing the >> values. This is what this commits does. >> >> Signed-off-by: Andy Zhou <[email protected]> >> [[email protected] added Jesse's text to the commit message] >> Signed-off-by: Ben Pfaff <[email protected]> >> --- >> lib/odp-util.c | 8 ++------ >> tests/odp.at | 33 +++++++++++++++++++-------------- >> tests/ofproto-dpif.at | 18 +++++++++--------- >> 3 files changed, 30 insertions(+), 29 deletions(-) >> >> diff --git a/lib/odp-util.c b/lib/odp-util.c >> index cc83fa5..78d5a1b 100644 >> --- a/lib/odp-util.c >> +++ b/lib/odp-util.c >> @@ -2355,17 +2355,13 @@ odp_flow_key_from_flow__(struct ofpbuf *buf, const >> struct flow *data, >> * treat 'data' as a mask. */ >> is_mask = (data != flow); >> >> - if (flow->skb_priority) { >> - nl_msg_put_u32(buf, OVS_KEY_ATTR_PRIORITY, data->skb_priority); >> - } >> + nl_msg_put_u32(buf, OVS_KEY_ATTR_PRIORITY, data->skb_priority); >> >> if (flow->tunnel.ip_dst || is_mask) { >> tun_key_to_attr(buf, &data->tunnel); >> } >> >> - if (flow->skb_mark) { >> - nl_msg_put_u32(buf, OVS_KEY_ATTR_SKB_MARK, data->skb_mark); >> - } >> + nl_msg_put_u32(buf, OVS_KEY_ATTR_SKB_MARK, data->skb_mark); >> >> /* Add an ingress port attribute if this is a mask or 'odp_in_port' >> * is not the magical value "ODPP_NONE". */ >> diff --git a/tests/odp.at b/tests/odp.at >> index b0aca6c..469e120 100644 >> --- a/tests/odp.at >> +++ b/tests/odp.at >> @@ -24,7 +24,7 @@ >> in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x86dd),ipv >> >> in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x86dd),ipv6(src=::1,dst=::2,label=0,proto=58,tclass=0,hlimit=128,frag=no),icmpv6(type=136,code=0),nd(target=::3,tll=00:0a:0b:0c:0d:0e) >> >> in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x86dd),ipv6(src=::1,dst=::2,label=0,proto=58,tclass=0,hlimit=128,frag=no),icmpv6(type=136,code=0),nd(target=::3,sll=00:05:06:07:08:09,tll=00:0a:0b:0c:0d:0e) >> >> in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x0806),arp(sip=1.2.3.4,tip=5.6.7.8,op=1,sha=00:0f:10:11:12:13,tha=00:14:15:16:17:18) >> >> -skb_mark(0x1234),in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x86dd),ipv6(src=::1,dst=::2,label=0,proto=58,tclass=0,hlimit=128,frag=no),icmpv6(type=136,code=0),nd(target=::3,sll=00:05:06:07:08:09,tll=00:0a:0b:0c:0d:0e) >> >> +in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x86dd),ipv6(src=::1,dst=::2,label=0,proto=58,tclass=0,hlimit=128,frag=no),icmpv6(type=136,code=0),nd(target=::3,sll=00:05:06:07:08:09,tll=00:0a:0b:0c:0d:0e) >> >> in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x8847),mpls(label=100,tc=3,ttl=64,bos=1) >> >> in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x8847),mpls(label=100,tc=7,ttl=100,bos=1) >> >> in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x8847),mpls(label=100,tc=7,ttl=100,bos=0) >> @@ -33,48 +33,53 @@ >> in_port(1),eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15),eth_type(0x8848),mpl >> ]) >> >> (echo '# Valid forms without tun_id or VLAN header.' >> - cat odp-base.txt >> + set 's/^/skb_priority(0),skb_mark(0),/' odp-base.txt >> + >> + set ' >> +s/^/skb_priority(0),skb_mark(0),/ >> +' odp-base.txt >> + >> >> echo >> echo '# Valid forms with tunnel header.' >> - sed >> 's/^/tunnel(tun_id=0x7f10354,src=10.10.10.10,dst=20.20.20.20,tos=0x0,ttl=64,flags(csum,key)),/' >> odp-base.txt >> + sed >> 's/^/skb_priority(0),tunnel(tun_id=0x7f10354,src=10.10.10.10,dst=20.20.20.20,tos=0x0,ttl=64,flags(csum,key)),skb_mark(0x1234),/' >> odp-base.txt >> >> echo >> echo '# Valid forms with VLAN header.' >> - sed 's/\(eth([[^)]]*)\),*/\1,eth_type(0x8100),vlan(vid=99,pcp=7),encap(/ >> + sed 's/^/skb_priority(0),skb_mark(0),/ >> +s/\(eth([[^)]]*)\),*/\1,eth_type(0x8100),vlan(vid=99,pcp=7),encap(/ >> s/$/)/' odp-base.txt >> >> echo >> echo '# Valid forms with MPLS header.' >> - sed >> 's/\(eth([[^)]]*),?\)/\1,eth_type(0x8847),mpls(label=100,tc=7,ttl=64,bos=1)/' >> odp-base.txt >> + sed 's/^/skb_priority(0),skb_mark(0),/ >> >> +s/\(eth([[^)]]*),?\)/\1,eth_type(0x8847),mpls(label=100,tc=7,ttl=64,bos=1)/' >> odp-base.txt >> >> echo >> echo '# Valid forms with MPLS multicast header.' >> - sed >> 's/\(eth([[^)]]*),?\)/\1,eth_type(0x8848),mpls(label=100,tc=7,ttl=64,bos=1)/' >> odp-base.txt >> - >> - echo >> - echo '# Valid forms with QoS priority.' >> - sed 's/^/skb_priority(0x1234),/' odp-base.txt >> + sed 's/^/skb_priority(0),skb_mark(0),/ >> >> +s/\(eth([[^)]]*),?\)/\1,eth_type(0x8848),mpls(label=100,tc=7,ttl=64,bos=1)/' >> odp-base.txt >> >> echo >> echo '# Valid forms with tunnel and VLAN headers.' >> - sed >> 's/^/tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,tos=0x8,ttl=128,flags(key)),/ >> + sed >> 's/^/skb_priority(0),tunnel(tun_id=0xfedcba9876543210,src=10.0.0.1,dst=10.0.0.2,tos=0x8,ttl=128,flags(key)),skb_mark(0),/ >> s/\(eth([[^)]]*)\),*/\1,eth_type(0x8100),vlan(vid=99,pcp=7),encap(/ >> s/$/)/' odp-base.txt >> >> echo >> echo '# Valid forms with QOS priority, tunnel, and VLAN headers.' >> - sed >> 's/^/skb_priority(0x1234),tunnel(tun_id=0xfedcba9876543210,src=10.10.10.10,dst=20.20.20.20,tos=0x8,ttl=64,flags(key)),/ >> + sed >> 's/^/skb_priority(0x1234),tunnel(tun_id=0xfedcba9876543210,src=10.10.10.10,dst=20.20.20.20,tos=0x8,ttl=64,flags(key)),skb_mark(0),/ >> s/\(eth([[^)]]*)\),*/\1,eth_type(0x8100),vlan(vid=99,pcp=7),encap(/ >> s/$/)/' odp-base.txt >> >> echo >> echo '# Valid forms with IP first fragment.' >> -sed -n 's/,frag=no),/,frag=first),/p' odp-base.txt >> +sed 's/^/skb_priority(0),skb_mark(0),/' odp-base.txt | sed -n >> 's/,frag=no),/,frag=first),/p' >> >> echo >> echo '# Valid forms with IP later fragment.' >> -sed -n 's/,frag=no),.*/,frag=later)/p' odp-base.txt) > odp.txt >> +sed 's/^/skb_priority(0),skb_mark(0),/' odp-base.txt | sed -n >> 's/,frag=no),.*/,frag=later)/p' >> +) > odp.txt >> AT_CAPTURE_FILE([odp.txt]) >> AT_CHECK_UNQUOTED([test-odp parse-keys < odp.txt], [0], [`cat odp.txt` >> ]) >> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at >> index 2c14af6..46e1dea 100644 >> --- a/tests/ofproto-dpif.at >> +++ b/tests/ofproto-dpif.at >> @@ -2088,12 +2088,12 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p2 >> 'in_port(2),eth(src=50:54:00:00:00: >> AT_CHECK([ovs-appctl netdev-dummy/receive p3 >> 'in_port(3),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) >> >> AT_CHECK([ovs-appctl dpif/dump-flows br0 | sort | STRIP_USED], [0], [dnl >> >> -in_port(1),eth_type(0x0800),ipv4(src=192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> >> -in_port(2),eth_type(0x0800),ipv4(src=192.168.0.2/0.0.0.0,dst=192.168.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> >> +skb_priority(0),in_port(1),eth_type(0x0800),ipv4(src=192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> >> +skb_priority(0),in_port(2),eth_type(0x0800),ipv4(src=192.168.0.2/0.0.0.0,dst=192.168.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> ]) >> >> AT_CHECK([ovs-appctl dpif/dump-flows br1 | sort | STRIP_USED], [0], [dnl >> >> -in_port(3),eth_type(0x0800),ipv4(src=10.0.0.2/0.0.0.0,dst=10.0.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> >> +skb_priority(0),in_port(3),eth_type(0x0800),ipv4(src=10.0.0.2/0.0.0.0,dst=10.0.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> ]) >> >> OVS_VSWITCHD_STOP >> @@ -2110,12 +2110,12 @@ AT_CHECK([ovs-appctl netdev-dummy/receive p2 >> 'in_port(2),eth(src=50:54:00:00:00: >> AT_CHECK([ovs-appctl netdev-dummy/receive p3 >> 'in_port(3),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)']) >> >> AT_CHECK([ovs-appctl dpif/dump-flows br0 | sort | STRIP_USED], [0], [dnl >> >> -in_port(1),eth_type(0x0800),ipv4(src=192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> >> -in_port(2),eth_type(0x0800),ipv4(src=192.168.0.2/0.0.0.0,dst=192.168.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> >> +skb_priority(0),in_port(1),eth_type(0x0800),ipv4(src=192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> >> +skb_priority(0),in_port(2),eth_type(0x0800),ipv4(src=192.168.0.2/0.0.0.0,dst=192.168.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> ]) >> >> AT_CHECK([ovs-appctl dpif/dump-flows br1 | sort | STRIP_USED], [0], [dnl >> >> -in_port(3),eth_type(0x0800),ipv4(src=10.0.0.2/0.0.0.0,dst=10.0.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> >> +skb_priority(0),in_port(3),eth_type(0x0800),ipv4(src=10.0.0.2/0.0.0.0,dst=10.0.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> ]) >> >> AT_CHECK([ovs-appctl dpif/del-flows br0]) >> @@ -2123,7 +2123,7 @@ AT_CHECK([ovs-appctl dpif/dump-flows br0 | sort | >> STRIP_USED], [0], [dnl >> ]) >> >> AT_CHECK([ovs-appctl dpif/dump-flows br1 | sort | STRIP_USED], [0], [dnl >> >> -in_port(3),eth_type(0x0800),ipv4(src=10.0.0.2/0.0.0.0,dst=10.0.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> >> +skb_priority(0),in_port(3),eth_type(0x0800),ipv4(src=10.0.0.2/0.0.0.0,dst=10.0.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:0, bytes:0, used:0.0s, >> actions:userspace(pid=0,slow_path(controller)) >> ]) >> >> OVS_VSWITCHD_STOP >> @@ -2170,10 +2170,10 @@ dummy@ovs-dummy: hit:13 missed:2 >> ]) >> >> AT_CHECK([ovs-appctl dpif/dump-flows br0 | STRIP_USED], [0], [dnl >> >> -in_port(100),eth_type(0x0800),ipv4(src=192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:9, bytes:540, used:0.0s, actions:101,3,2 >> >> +skb_priority(0),in_port(100),eth_type(0x0800),ipv4(src=192.168.0.1/0.0.0.0,dst=192.168.0.2/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:9, bytes:540, used:0.0s, actions:101,3,2 >> ]), >> AT_CHECK([ovs-appctl dpif/dump-flows br1 | STRIP_USED], [0], [dnl >> >> -in_port(101),eth_type(0x0800),ipv4(src=192.168.0.2/0.0.0.0,dst=192.168.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:4, bytes:240, used:0.0s, actions:100,2,3 >> >> +skb_priority(0),in_port(101),eth_type(0x0800),ipv4(src=192.168.0.2/0.0.0.0,dst=192.168.0.1/0.0.0.0,proto=1/0,tos=0/0,ttl=64/0,frag=no/0xff), >> packets:4, bytes:240, used:0.0s, actions:100,2,3 >> ]) >> >> AT_CHECK([ovs-ofctl dump-ports br0 pbr0], [0], [dnl >> -- >> 1.7.10.4 >> > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
