On Mon, Nov 26, 2012 at 06:50:26PM +0200, Jarno Rajahalme wrote: > > Initial OpenFlow 1.3 support with new include/openflow/openflow-1.3.h. > Most of the messages that differ from 1.2 are implemented. OFPT_SET_ASYNC > is implemented via NX_SET_ASYNC_CONFIG, other new message types are > yet to be implemented. Stats replies that add duration fields are > implemented at > encode/decode level only. Test cases for implemented features are included. > > Remaining FIXME:s should not cause runtime aborts. make check comes out clean.
Thanks for the second revision! I have a few more comments. I think that I would be inclined to put all of the OFPXMT* and related definitions in openflow-1.2, perhaps with a comment above the ones that were introduced in OF1.3. One of the goals of the OXM flow format is that it should be extensible; that is, that it should be possible to introduce new fields without disrupting implementations (switches or controllers) that only understand the existing fields. Thus, I feel like keeping all of the definitions together (to make them easy to see together) is a better choice than to separate them by version (to make it easy to tell which came from which version). As a practical matter, I've found that code is easier to read and to write if, when a newer version of OpenFlow redefines a data structure only by replacing padding in-place by a real structure member, we simply modify the existing data structure and add a comment in or above it that says that the member was previously padding, rather than defining a completely new data structure. Thus, instead of defining a new struct ofp13_switch_features, I would just change the existing struct ofp_switch_features. Similarly, for ofp13_flow_stats, I would just modify ofp11_flow_stats. I wouldn't bother with defining new struct and enums for multipart. We can use and might as well use the existing stats structs and enums, since they have exactly the same values and meanings.. I'd be inclined to define ofp13_port_stats as a wrapper around ofp11_port_stats, to simplify code, and similarly for ofp13_queue_stats, ofp13_group_stats, and ofp13_packet_in. I'm not sure there is any value in ofp13_experimenter_multipart_header. It's very much like ofp11_vendor_stats_msg and the extra 'exp_type' member isn't good for much as far as I can tell. In ofp_print_switch_features(), it would be better, as usual, to avoid unnecessary dependencies on the protocol version in use. The underlying ofputil_decode_switch_features() function already takes care of that, in this case, so I would simply check for nonzero features.auxiliary_id instead of explicitly for the OpenFlow version. In ofputil_port_stats_to_ofp13(), the hardcoded 4.000000002 second duration is rather odd. I'd suggest all-one-bits for the moment since that means "unknown" elsewhere in the protocol. Ditto for ofputil_queue_stats_to_ofp13(). Thanks, Ben. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
