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

Fri Aug 17 08:42:53 UTC 2012

On 16 August 2012 06:43, Ben Pfaff <blp at nicira.com> wrote:
> On Wed, Aug 15, 2012 at 12:28:47AM +1200, Joe Stringer wrote:
>> In OpenFlow 1.1, we add support for OFPIT_WRITE_METADATA. This allows us to
>> write to the metadata field. Internally it is represented using ofpact_metadata.
>>
>> We introduce NXAST_WRITE_METADATA to handle writing to the metadata field in
>> OpenFlow 1.0+. This structure reflects OFPIT_WRITE_METADATA.
>>
>> When writing out the structure to OpenFlow 1.1, it uses the OFPIT_WRITE_METADATA
>> instruction only, and not the new NXAST action (which would be redundant).
>>
>> Signed-off-by: Joe Stringer <joe at wand.net.nz>
>
> Isaku's comment seems reasonable to me.

I put host byte-order here just because the rest of OFPACT_* appeared
to use host byte-order, but if we're not worried about keeping that
consistent then I'll make the change.

> I've been trying to insist for newly added actions that any padding
> bytes must be zero.  So I'd rename this "zeros", add a comment "Must
> be zero." and check for that in metadata_from_openflow(), returning
> OFPERR_NXBRC_MUST_BE_ZERO if there is a nonzero byte.

Sure, I'll do this.

> Doesn't ofpact_put_WRITE_METADATA() return the correct type?  (Why
> not?)

It does. This was my mixup. Will be fixed in the next revision.

> Please add a space before the (.

Done.

> It looks like the other log messages in this file typically do not
> begin with a capital letter or end with a period.
>
> I'd forgotten that there was an "unsupported order" error, but it is
> certainly appropriate here.  Good choice.

Fixed. Thanks.

>> @@ -1524,6 +1599,14 @@ ofpacts_put_openflow11_instructions(const struct ofpact ofpacts[],
>>              in_instruction = true;
>>              ofs = openflow->size;
>>              instruction_put_OFPIT11_WRITE_ACTIONS(openflow);
>> +        } else if (a->type == OFPACT_WRITE_METADATA) {
>> +            const struct ofpact_metadata *om;
>> +            struct ofp11_instruction_write_metadata *oiwm;
>> +            om = ofpact_get_WRITE_METADATA(a);
>> +            oiwm = instruction_put_OFPIT11_WRITE_METADATA(openflow);
>> +            oiwm->metadata = htonll(om->metadata);
>> +            oiwm->metadata_mask = htonll(om->mask);
>> +            memset(oiwm->pad, 0, sizeof oiwm->pad);
>
> instruction_put_*() zeros what it adds, so I don't think the memset is
> necessary here.

Fixed.

>> +dnl OpenFlow 1.1 uses OFPIT_WRITE_METADATA to express the NXAST_WRITE_METADATA
>> +dnl action instead, so parse-ofp11-actions will recognise and drop this action.
>> +# instructions=write_metadata:0xfedcba9876543210
>> +#  0: ff -> (none)
>> +#  1: ff -> (none)
>> +#  2: 00 -> (none)
>> +#  3: 20 -> (none)
>> +#  4: 00 -> (none)
>> +#  5: 00 -> (none)
>> +#  6: 23 -> (none)
>> +#  7: 20 -> (none)
>> +#  8: 00 -> (none)
>> +#  9: 15 -> (none)
>> +# 10: 00 -> (none)
>> +# 11: 00 -> (none)
>> +# 12: 00 -> (none)
>> +# 13: 00 -> (none)
>> +# 14: 00 -> (none)
>> +# 15: 00 -> (none)
>> +# 16: fe -> (none)
>> +# 17: dc -> (none)
>> +# 18: ba -> (none)
>> +# 19: 98 -> (none)
>> +# 20: 76 -> (none)
>> +# 21: 54 -> (none)
>> +# 22: 32 -> (none)
>> +# 23: 10 -> (none)
>> +# 24: ff -> (none)
>> +# 25: ff -> (none)
>> +# 26: ff -> (none)
>> +# 27: ff -> (none)
>> +# 28: ff -> (none)
>> +# 29: ff -> (none)
>> +# 30: ff -> (none)
>> +# 31: ff -> (none)
>> +ffff 0020 00002320 0015 000000000000 fedcba9876543210 ffffffffffffffff
>
> I'm not sure I understand the above.  It seems counterintuitive at
> first glance.  I need to take another look when I've got a few more
> minutes.

Here's my understanding of the above behaviour:-
ovs-ofctl parse-ofp11-actions reads in hex, and converts that to the
internal OFPACTS structure. The write_metadata is recognised, hence
the first line:-

# instructions=write_metadata:0xfedcba9876543210

But then parse-ofp11-actions only outputs actions. When write_metadata
is outputted to OpenFlow 1.1, it is ONLY outputted as an official
OF1.1 write-metadata instruction. The actions part will skip over the
ofpact, to prevent serializing the same metadata twice. So, the diff
shows the entire packet being dropped.

>> diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
>> index 22fc530..2254fdb 100644
>> --- a/utilities/ovs-ofctl.8.in
>> +++ b/utilities/ovs-ofctl.8.in
>> @@ -614,6 +614,14 @@ When this field is specified in \fBadd-flow\fR, \fBadd-flows\fR,
>>  extension to OpenFlow, which as of this writing is only known to be
>>  implemented by Open vSwitch.
>>  .
>> +.IP \fBmetadata=\fIvalue\fR[\fB/\fImask\fR]
>> +Matches \fIvalue\fR either exactly or with optional \fImask\fR in the metadata
>> +field. \fIvalue\fR and \fImask\fR are 64-bit integers, by default in decimal
>> +(use a \fB0x\fR prefix to specify hexadecimal). Arbitrary \fImask\fR values
>> +are allowed: a 1-bit in \fImask\fR indicates that the corresponding bit in
>> +\fIvalue\fR must match exactly, and a 0-bit wildcards that bit. Matching on
>> +metadata was added in Open vSwitch 1.8.
>> +.
>
> It looks like the above hunk is really a documentation bug fix for
> what's already on master.  If so, would you mind submitting it as a
> separate patch so I can apply it immediately?

Done.

I also realise the above patch was sneaking in extra keywords for
actions tests:-

-AT_KEYWORDS([OF1.0])
+AT_KEYWORDS([ofp-actions OF1.0])

I found this useful as I tested, and will leave them as part of this
patch unless you have a specific opinion otherwise.

With my next revision, I'll add missing tests (ensuring specific fail
behaviour). Could be atleast a few days though, things are quite busy
at the moment.

Cheers,
Joe