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

[Ben Pfaff]

On Fri, Aug 17, 2012 at 08:42:53PM +1200, Joe Stringer wrote:
> 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'm not sure that the existing OFPACT_* actually made the right choice.
It was pointed out as an odd choice in review.  I might change it to use
network byte order.

> >> +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.

Thanks for explaining.  Makes sense.

> 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.

That's fine.

> 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.

Thanks.