On 16 August 2012 06:43, Ben Pfaff <b...@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 <j...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
