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 <az...@nicira.com>
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 <az...@nicira.com>
[b...@nicira.com added Jesse's text to the commit message]
Signed-off-by: Ben Pfaff <b...@nicira.com>
---
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
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
