[ovs-dev] [PATCH v2] ofp-actions: Implement writing to metadata field

[Ben Pfaff]

On Mon, Jul 16, 2012 at 09:27:08AM -0700, Ben Pfaff wrote:
> On Mon, Jul 16, 2012 at 07:38:39PM +1200, Joe Stringer wrote:
> > On 13 July 2012 16:01, Ben Pfaff <blp at nicira.com> wrote:
> > > ...I think that we should restrict parsing NXAST_WRITE_METADATA to one
> > > entry and only as the final entry.
> ...
> > > There are only two important paths into ofpacts: parsing of binary
> > > instructions and actions, and parsing of text versions of instructions
> > > and actions.  We'd want to make sure that both of these paths enforce
> > > the constraint.
> > 
> > Would it just be enough to to ofpacts_check() to verify there's only
> > one ofpact_metadata, and it's at the end of 'ofpacts'?
> > 
> > This seems like the appropriate way -- keeping it simple, and applying
> > to all versions of the protocol.. and it follows the comments for
> > ofpacts_pull_openflow10(),11() which say that it is the caller's
> > responsibility to check that the ofpacts are valid. My only concern is
> > that the other code which uses these pull functions don't appear use
> > this method to verify the ofpacts are valid.
> 
> Hmm.  The comments don't express it, but the current intent is
> different.  Each of the two paths into ofpacts is supposed to check
> the actions to the limit of the extent that it can.  But some ofpacts
> require additional context to be fully checked (either the maximum
> port number or an associated flow).  That context isn't always
> available at the time of parsing (and sometimes it's not a single
> context, since a set of actions might be getting applied to a whole
> bunch of flows at once), so we reserve that checking for the
> ofpacts_check() function.
> 
> I don't really like that we need ofpacts_check(), and my tendency is
> to want to get rid of it somehow (not sure how), not to expand its
> scope.
> 
> We should clarify the comments to make all of the above clear.  I'll
> do that.

Patch available:
        http://openvswitch.org/pipermail/dev/2012-July/018998.html