[ovs-dev] [PATCH 3/4] Add some missing Open Flow 1.1 definitions

[Simon Horman]

On Wed, Mar 28, 2012 at 12:21:18PM -0700, Ben Pfaff wrote:
> On Wed, Mar 28, 2012 at 09:44:16AM +0900, Simon Horman wrote:
> > Signed-off-by: Simon Horman <horms at verge.net.au>
> 
> The comment here is wrong, since it talks about OXM but OXM was
> introduced in OpenFlow 1.2:
> > +/* The match type indicates the match structure (set of fields that compose the
> > + * match) in use. The match type is placed in the type field at the beginning
> > + * of all match structures. The "OpenFlow Extensible Match" type corresponds
> > + * to OXM TLV format described below and must be supported by all OpenFlow
> > + * switches. Extensions that define other match types may be published on the
> > + * ONF wiki. Support for extensions is optional.
> > + */
> > +enum ofp11_match_type {
> > +    OFPMT11_STANDARD = 0,       /* The match fields defined in the ofp11_match
> > +                                   structure apply */
> > +};
> 
> I'm actually thinking that maybe we should just have "enum
> ofp_match_type" in openflow-common.h, with all the possible values (so
> far only two).  Sure, enum ofp_match_type was introduced in OpenFlow
> 1.1, but the intention, at least, is to maintain the meaning of each
> value over time.
> 
> I think I feel the same way about ofp_packet_in_reason and
> specifically about OFPR_INVALID_TTL.  No values have been removed or
> had their meanings changed from OF1.0 to OF1.2, and in fact OVS
> supports OFPR_INVALID_TTL value as a Nicira extension to OF1.0, so
> there's not much value in breaking it apart across header files.
> 
> Also for ofp_flow_removed_reason.

That sounds good to me, its probably easier all around to have
the definitions centralised where possible.

> For stats messages with fixed length, we've found over time that it's
> more convenient to include the ofp_header and stats message header in
> the structure, rather than having separate headers and bodies.  For
> example, look at struct ofp_flow_stats_request in openflow-1.0.h.  I
> think that we should retain this pattern in openflow-1.1.h where it is
> applicable, that is, for ofp11_flow_stats_request,
> ofp11_queue_stats_request, and ofp11_group_stats_request, I think.

Thanks, I was a little confused looking over the existing code, but I
understand now. I'll update the ofp11 structures as you suggest.

> I'd remove ofp11_aggregate_stats_request since it is identical to
> ofp11_flow_stats_request, just updating the comment to note that.

Sure, will do.