Nifty, thanks.
Ethan
On Tue, May 15, 2012 at 12:51 PM, Ben Pfaff <b...@nicira.com> wrote:
> 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
