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

[Ben Pfaff]

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.

So I'd rather check in the individual places.

Thanks,

Ben.