[ovs-dev] [ECNv2 2/5] ofproto-dpif: Make initial packet value handling generic.

Ben Pfaff blp at nicira.com
Tue Mar 5 23:44:19 UTC 2013


On Tue, Mar 05, 2013 at 03:40:13PM -0800, Justin Pettit wrote:
> On Mar 5, 2013, at 9:51 AM, Ben Pfaff <blp at 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.



More information about the dev mailing list