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

[Joe Stringer]

On 13 July 2012 16:01, Ben Pfaff <blp at nicira.com> wrote:
> On Fri, Jul 13, 2012 at 02:48:49PM +1200, Joe Stringer wrote:
>> On 13 July 2012 12:02, Ben Pfaff <blp at nicira.com> wrote:
>> > Suppose you fix that; it should not be hard (just remove an "else"
>> > keyword, I think).  Then, second, the semantics implemented here will
>> > surprise users, because if ofpacts are translated to OF1.0 then each
>> > OFPIT_WRITE_METADATA will be executed one at a time at the point where
>> > it appears in the actions, but if they are translated to OF1.1 then only
>> > a single OFPIT_WRITE_METADATA will be processed, after all other
>> > actions.  I'd rather avoid that.  One idea: report an error if
>> > "write_metadata" is specified more than once or if it is not specified
>> > as the last action.
>>
>> Do we want to just limit a maximum of one ofpact_metadata per ofpacts?
>>
>> So, with the above patch:-
>> When OFPIT_WRITE_METADATA is converted to ofpacts, an error is thrown
>> if there is more than one. The conversion is done last, so we don't
>> care where the OFPIT_WRITE_METADATA is placed within the message; it
>> is parsed last, and added to the end of ofpacts. When the ofpacts are
>> converted back to OF1.1, we ignore the NXAST translation case and
>> always write OFPIT_WRITE_METADATA last. If we only have a maximum of
>> one ofpact_metadata in our ofpacts, then this is all fine.
>
> Yes, all of that makes sense.
>
>> The trouble is with NXAST_WRITE_METADATA; Parsing to ofpacts may
>> currently cause multiple metadata_ofpacts to appear internally, but we
>> don't want this. If we can restrict this to one entry, then the
>> reverse process (ofpacts->NX), just needs to ensure that the
>> NXAST_WRITE_METADATA action is the last to appear in the actions list.
>
> Yes, I think that we should restrict parsing NXAST_WRITE_METADATA to one
> entry and only as the final entry.
>
>> Is this the preferred approach? Are there other examples which may
>> cause multiple instances of the ofpact_metadata to be stored in
>> ofpacts?
>
> 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.