[ovs-dev] [PATCH v2 14/16] ofp-actions: Centralize all OpenFlow action code for maintainability.

Ben Pfaff blp at nicira.com
Mon Aug 11 19:57:28 UTC 2014


Thanks for the review!

On Mon, Aug 11, 2014 at 11:34:58AM -0700, Jarno Rajahalme wrote:
> Nice cleanup, especially like the way wire formats are now hidden
> inside lib/ofp-actions.c.
> 
> Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
> 
> On Aug 7, 2014, at 4:14 PM, Ben Pfaff <blp at nicira.com> wrote:
> > +    /* XXX
> > +     *
> > +     * If write_metadata is specified as an action AND an instruction, ofpacts
> > +     * could be invalid. */
> 
> Could this be resolved by not allowing the action for OpenFlow
> versions which support the instruction?

Thanks for pointing out this comment.  This comment is obsolete now
because we now (as of the previous commit) only allow write_metadata
actions in OF1.0 in contexts where write_metadata instructions are
allowed in OF1.1+.

I deleted the comment.

> > +    ds_destroy(&actions);
> > +    if (error) {
> > +        ofpbuf_uninit(&ofpacts);
> > +        return error;
> > +    }
> >     bucket->ofpacts = ofpbuf_data(&ofpacts);
> >     bucket->ofpacts_len = ofpbuf_size(&ofpacts);
> > 
> 
> Should bucket parsing go to ofp-actions.c as well?

That's an interesting idea.  I'll look at that for a future change.

> (snip)
> 
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 7bf53a4..e17377f 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -1246,7 +1246,6 @@ add_internal_flows(struct ofproto_dpif *ofproto)
> >      * (priority=2), recirc=0, actions=resubmit(, 0)
> >      */
> >     resubmit = ofpact_put_RESUBMIT(&ofpacts);
> > -    resubmit->ofpact.compat = 0;
> 
> Doesn?t this leave the renamed ?raw? member as ?-1?? Does this matter?

It doesn't matter.

I'll push this in a minute, after rerunning the tests.



More information about the dev mailing list