On Fri, Aug 17, 2012 at 08:42:53PM +1200, Joe Stringer wrote: > 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'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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
