From 54bb0348d2588aab6447784bb0ca8ff0b794df15 Mon Sep 17 00:00:00 2001 From: Andy Zhou 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 [blp@nicira.com added Jesse's text to the commit message] Signed-off-by: Ben Pfaff --- 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 cc83fa567..78d5a1b44 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 b0aca6c68..469e120dd 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 2c14af6fb..46e1deaa7 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 -- 2.43.0