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 <[email protected]>
>
> On Aug 7, 2014, at 4:14 PM, Ben Pfaff <[email protected]> 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.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
