On Jan 4, 2012, at 10:05 AM, Ben Pfaff wrote:
> On Tue, Jan 03, 2012 at 05:13:46PM -0800, Justin Pettit wrote:
>> Both mirroring and "normal" processing make use of the input bundle to
>> perform various sanity checks. Controller-generated traffic typically
>> uses an ingress port of OFPP_NONE, which doesn't have a corresponding
>> input bundle. This commit fakes one up well enough that mirroring and
>> "normal" processing succeed.
>>
>> We looked at creating an actual bundle based on the "real" OFPP_NONE.
>> This was even uglier, since there were even more special-cases that
>> needed to be handled, including having to hide it from port queries.
>>
>> Reported-by: Jesse Gross <je...@nicira.com>
>> Signed-off-by: Justin Pettit <jpet...@nicira.com>
>
> ofpp_none_bundle should be "static".
Thanks.
> CodingStyle says not to use C99 designated initializers:
>
> * Don't use designated initializers (e.g. don't write "struct foo
> foo = {.a = 1};" or "int a[] = {[2] = 5};").
>
> However, I've wanted to use them in many places and C99 is now about 13
> years old, so maybe we should remove this restriction. "git grep '^
> *\..*='" shows that designated initializers have already crept in in a
> few non-Linux-specific places already, such as lib/ovsdb-types.h. What
> do you think?
Well, some hardware vendors' compilers seem to even pre-date C73. However, I
did a quick poll around here, and it appears that their former
employers/existing contacts believe this shouldn't be a problem. As you point
out, anyone who's built the IDL since 2010 would have already run into it as a
problem.
> The only part of this that gives me much pause is input_vid_is_valid().
> This function refers to in_bundle->ofproto->up.name, but only in cases
> that we can't hit. It still makes me a bit nervous, though.
Yeah, it seems a bit fragile for future code. I put in a special case that
returns "true" if the bundle is ofpp_none_bundle.
> The test checks the mirroring case, but I think that "normal" was broken
> too. Can we add a test for that too?
Damn. I noticed that before I sent it off, but was hoping it'd slip past you.
No such luck. For that test, I need your "ofproto-dpif: Fix nondeterministic
flow revalidation behavior." patch. Since you don't want to push that into
branch-1.4 in order to give it more time to bake, I'll send out the test
separately and only for master.
Thanks for the review. I pushed this to master and branch-1.4 (with a small
change to make the test work).
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
