[ovs-dev] [PATCH OF12+ 1/4 v2] OpenFlow 1.2 fixes and initial OpenFlow 1.3 support

[Ben Pfaff]

On Fri, Nov 23, 2012 at 01:33:55PM +0200, Jarno Rajahalme wrote:
> On Nov 23, 2012, at 2:14 , ext Ben Pfaff wrote:
> > On Thu, Nov 22, 2012 at 05:24:06PM +0200, Jarno Rajahalme wrote:
> >>    OpenFlow 1.2 protocol fixes: Add OFPP_ANY to include/openflow/openflow-1.1.h,
> >>    and allow it as a port in queue stats request. Make ovs_ofctl use OFPP_ANY
> >>    instead of OFPP_ALL for queue stats requests on OF 1.1+.
> >>    Do not check out_group on flow_mod unless the command is DELETE*.
> > 
> > Thank you for this patch (and the rest)!
> > 
> > The change to ofputil_decode_flow_mod() looks good, except that you
> > should break the line so that it does not exceed 79 columns wide.
> > 
> > The purpose of ofputil_queue_stats_request and the functions that encode
> > and decode it is to insulate other code from having to know the
> > differences between the various OpenFlow versions.  Therefore, instead
> > of modifying handle_queue_stats_request() and ofctl_queue_stats() to
> > support the various versions, I would modify
> > ofputil_encode_queue_stats_request() and
> > ofputil_decode_queue_stats_request() so that they do not have to know
> > the difference.  For example, ofputil_encode_queue_stats_request() could
> > translate OFPP_ANY to the appropriate value in whatever version it is
> > encoding, and ofputil_decode_queue_stats_request() could translate the
> > particular version's value to OFPP_ANY.  The clients would then use
> > OFPP_ANY regardless of version.
> 
> Below is an updated patch to do the above.
> 
> OF 1.1+ uses OFPP_ANY also for flow_mod, flow_stats, and port_stats
> requests. ofp-util.c now uses OFPP_NONE for that purpose, which is the
> same and thus works fine. Semantically, however, OFPP_ANY would be a
> better fit for the function for selecting all ports than
> OFPP_NONE. Therefore it might be good to update ofp_util.c to use
> OFPP_ANY instead of OFPP_NONE for selecting all ports. As it relates
> to this patch that adds the definition of OFPP_ANY, it might be a good
> idea to do that change while we are at it. thoughts?

That would be fine with me.  Will you send a new version?

The Open vSwitch C style requires {} around the conditional statement
here:
+        if (oqsr->port_no == OFPP_ALL)
+            oqsr->port_no = OFPP_ANY;