Well, we can do better, anyway, by adjusting test-odp.c and odp.at.
Update appended
On Tue, May 15, 2012 at 11:19:04AM -0700, Ethan Jackson wrote:
> Just out of curiosity, is there any way we can build assert, or unit
> test these values? That seems like the most future proof way to
> ensure they're correct, though I'm not sure what such a check would
> look like.
>
> Ethan
>
> On Tue, May 15, 2012 at 10:42 AM, Justin Pettit <jpet...@nicira.com> wrote:
> > On May 15, 2012, at 10:29 AM, Ben Pfaff wrote:
> >
> >>>>> Do you think it's worth having the total be a #define, and then do a
> >>>>> build-time assertion that the total value is less than
> >>>>> ODPUTIL_FLOW_KEY_BYTES?
> >>>>
> >>>> Can you explain how that would be different?
> >>>
> >>> Right now, we have a chart that maps out the total number of bytes
> >>> needed to represent the largest flow. When updating that chart it's
> >>> pretty obvious that the "total" field at the bottom needs to be
> >>> updated. I feel like we've had issues in the past where "total" was
> >>> updated, but ODPUTIL_FLOW_KEY_BYTES was not. This would serve as a
> >>> reminder. It's pretty unlikely to occur, but I was thinking it would
> >>> be a good sanity-check. We could also mandate a certain amount a
> >>> slack. However, if you think it's unnecessary, I'm fine not adding
> >>> one, too.
> >>
> >> Mostly I don't understand what you envision the code or comment looking
> >> like. Can you show me?
> >
> >
> > I was thinking of something like the following:
> >
> > -=-=-=-=-=-=-=-=-=-=-
> > * struct pad nl hdr total
> > * ------ --- ------ -----
> > * OVS_KEY_ATTR_PRIORITY 4 -- 4 8
> > ...
> > * OVS_KEY_ATTR_ND 28 -- 4 32
> > * -------------------------------------------------*/
> > #define ODP_FLOW_KEY_MAX_TOTAL 156
> >
> > /* We include some slack space in case the calculation isn't quite right or
> > we
> > * add another field and forget to adjust this value.
> > */
> > #define ODPUTIL_FLOW_KEY_BYTES 200
> >
> > BUILD_ASSERT_DECL(ODP_FLOW_KEY_MAX_TOTAL < ODPUTIL_FLOW_KEY_BYTES)
> > -=-=-=-=-=-=-=-=-=-=-
> >
> > However, ODPUTIL_FLOW_KEY_BYTES is defined pretty close to "total", so it's
> > probably pointless. I'll drop this, unless you've suddenly fallen in love
> > with the idea. :-)
--8<--------------------------cut here-------------------------->8--
From: Ben Pfaff <b...@nicira.com>
Date: Tue, 15 May 2012 12:50:57 -0700
Subject: [PATCH] odp-util: Update ODPUTIL_FLOW_KEY_BYTES for current kernel
flow format.
Before we submitted the kernel module upstream, we updated the flow format
by adding two fields to the description of packets with VLAN headers, but
we forgot to update ODPUTIL_FLOW_KEY_BYTES to reflect these changes. The
result was that a maximum-length flow did not fit in the given space.
This fixes a crash processing IPv6 neighbor discovery packets with VLAN
headers received in a tunnel configured with key=flow or in_key=flow.
This updates some comments to better describe the implications of
ODPUTIL_FLOW_KEY_BYTES (suggested by Justin).
This also updates test-odp.c so that it would have caught this problem, and
updates odp.at to demonstrate that a full 156 bytes are necessary. (To see
that, revert the change to ODPUTIL_FLOW_KEY_BYTES and run the test.)
Reported-by: Dan Wendlandt <d...@nicira.com>
Signed-off-by: Ben Pfaff <b...@nicira.com>
---
lib/odp-util.c | 7 +++++--
lib/odp-util.h | 25 +++++++++++++++++++------
tests/odp.at | 6 ++++++
tests/test-odp.c | 11 +++++++++--
4 files changed, 39 insertions(+), 10 deletions(-)
diff --git a/lib/odp-util.c b/lib/odp-util.c
index aae5cc7..7e5c12f 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2009, 2010, 2011 Nicira Networks.
+ * Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -1155,7 +1155,10 @@ ovs_to_odp_frag(uint8_t ovs_frag)
: OVS_FRAG_TYPE_NONE);
}
-/* Appends a representation of 'flow' as OVS_KEY_ATTR_* attributes to 'buf'. */
+/* Appends a representation of 'flow' as OVS_KEY_ATTR_* attributes to 'buf'.
+ *
+ * 'buf' must have at least ODPUTIL_FLOW_KEY_BYTES bytes of space, or be
+ * capable of being expanded to allow for that much space. */
void
odp_flow_key_from_flow(struct ofpbuf *buf, const struct flow *flow)
{
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 7c9b588..45d7d3f 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2009, 2010, 2011 Nicira Networks.
+ * Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -65,8 +65,16 @@ void format_odp_actions(struct ds *, const struct nlattr
*odp_actions,
int odp_actions_from_string(const char *, const struct shash *port_names,
struct ofpbuf *odp_actions);
-/* Upper bound on the length of a nlattr-formatted flow key. The longest
- * nlattr-formatted flow key would be:
+/* The maximum number of bytes that odp_flow_key_from_flow() appends to a
+ * buffer. This is the upper bound on the length of a nlattr-formatted flow
+ * key that ovs-vswitchd fully understands.
+ *
+ * OVS doesn't insist that ovs-vswitchd and the datapath have exactly the same
+ * idea of a flow, so therefore this value isn't necessarily an upper bound on
+ * the length of a flow key that the datapath can pass to ovs-vswitchd.
+ *
+ * The longest nlattr-formatted flow key appended by odp_flow_key_from_flow()
+ * would be:
*
* struct pad nl hdr total
* ------ --- ------ -----
@@ -74,15 +82,20 @@ int odp_actions_from_string(const char *, const struct
shash *port_names,
* OVS_KEY_ATTR_TUN_ID 8 -- 4 12
* OVS_KEY_ATTR_IN_PORT 4 -- 4 8
* OVS_KEY_ATTR_ETHERNET 12 -- 4 16
+ * OVS_KEY_ATTR_ETHERTYPE 2 2 4 8 (outer VLAN ethertype)
* OVS_KEY_ATTR_8021Q 4 -- 4 8
- * OVS_KEY_ATTR_ETHERTYPE 2 2 4 8
+ * OVS_KEY_ATTR_ENCAP 0 -- 4 4 (VLAN encapsulation)
+ * OVS_KEY_ATTR_ETHERTYPE 2 2 4 8 (inner VLAN ethertype)
* OVS_KEY_ATTR_IPV6 40 -- 4 44
* OVS_KEY_ATTR_ICMPV6 2 2 4 8
* OVS_KEY_ATTR_ND 28 -- 4 32
* -------------------------------------------------
- * total 144
+ * total 156
+ *
+ * We include some slack space in case the calculation isn't quite right or we
+ * add another field and forget to adjust this value.
*/
-#define ODPUTIL_FLOW_KEY_BYTES 144
+#define ODPUTIL_FLOW_KEY_BYTES 200
/* A buffer with sufficient size and alignment to hold an nlattr-formatted flow
* key. An array of "struct nlattr" might not, in theory, be sufficiently
diff --git a/tests/odp.at b/tests/odp.at
index 90a1192..3e7a8ad 100644
--- a/tests/odp.at
+++ b/tests/odp.at
@@ -48,6 +48,12 @@
s/\(eth([[^)]]*)\),*/\1,eth_type(0x8100),vlan(vid=99,pcp=7),encap(/
s/$/)/' odp-base.txt
echo
+ echo '# Valid forms with QOS priority, tun_id, and VLAN headers.'
+ sed 's/^/priority(1234),tun_id(0xfedcba9876543210),/
+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
diff --git a/tests/test-odp.c b/tests/test-odp.c
index 9ae897c..cbf6004 100644
--- a/tests/test-odp.c
+++ b/tests/test-odp.c
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2011 Nicira Networks.
+ * Copyright (c) 2011, 2012 Nicira Networks.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -27,6 +27,7 @@
int
main(void)
{
+ int exit_code = 0;
struct ds in;
ds_init(&in);
@@ -85,6 +86,12 @@ main(void)
ofpbuf_init(&odp_key, 0);
odp_flow_key_from_flow(&odp_key, &flow);
+ if (odp_key.size > ODPUTIL_FLOW_KEY_BYTES) {
+ printf ("too long: %zu > %d\n",
+ odp_key.size, ODPUTIL_FLOW_KEY_BYTES);
+ exit_code = 1;
+ }
+
/* Convert odp_key to string. */
ds_init(&out);
odp_flow_key_format(odp_key.data, odp_key.size, &out);
@@ -96,5 +103,5 @@ main(void)
}
ds_destroy(&in);
- return 0;
+ return exit_code;
}
--
1.7.2.5
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
