On Tue, Mar 05, 2013 at 03:40:13PM -0800, Justin Pettit wrote:
> On Mar 5, 2013, at 9:51 AM, Ben Pfaff <b...@nicira.com> wrote:
>
> > "git am" says:
> >
> > Applying: ofproto-dpif: Make initial packet value handling generic.
> > /home/blp/nicira/ovs/.git/rebase-apply/patch:140: trailing whitespace.
> > action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
> > warning: 1 line adds whitespace errors.
>
> Fixed.
>
> > The "*" in '*initial_vals->vlan_tci' is wrong I think:
>
> D'oh.
>
> > The use of 'one_subfacet' here worries me. 'one_subfacet' can
> > theoretically not be in use at any given time.
>
> I wonder if we should update the comment for 'one_subfacet':
>
> /* Storage for a single subfacet, to reduce malloc() time and space
> * overhead. (A facet always has at least one subfacet and in the common
> * case has exactly one subfacet.) */
> struct subfacet one_subfacet;
>
> My reading of that lead me to believe that it would always have a
> value. I'm happy to supply a patch, if you agree.
Clarifying the comment is a good idea.
Here's a case where one_subfacet would not be in use:
* Facet is initially created with one subfacet. (This uses
one_subfacet.)
* A second subfacet is created for the facet. (It would be
malloc()'d.)
* The first subfacet expires, but the second one doesn't.
> + struct subfacet *subfacet= CONTAINER_OF(list_front(&facet->subfacets),
> + struct subfacet, list_node);
There's a similarly unreadable expression in facet_account(), perhaps
a helper is warranted?
Thanks,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
