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

Jarno Rajahalme jarno.rajahalme at nsn.com
Fri Nov 23 11:33:55 UTC 2012


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?

diff --git a/include/openflow/openflow-1.1.h b/include/openflow/openflow-1.1.h
index 9785db4..c4a5aba 100644
--- a/include/openflow/openflow-1.1.h
+++ b/include/openflow/openflow-1.1.h
@@ -70,6 +70,14 @@
 #define OFPP11_MAX    0xffffff00
 #define OFPP11_OFFSET (OFPP11_MAX - OFPP_MAX)
 
+/* Reserved wildcard port used only for flow mod (delete) and flow stats
+ * requests. Selects all flows regardless of output port
+ * (including flows with no output port)
+ *
+ * Define it via OFPP_NONE (0xFFFF) so that OFPP_ANY is still an enum ofp_port
+ */
+#define OFPP_ANY OFPP_NONE
+
 /* OpenFlow 1.1 port config flags are just the common flags. */
 #define OFPPC11_ALL \
     (OFPPC_PORT_DOWN | OFPPC_NO_RECV | OFPPC_NO_FWD | OFPPC_NO_PACKET_IN)
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 4facf0a..0664945 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1435,7 +1435,9 @@ ofputil_decode_flow_mod(struct ofputil_flow_mod *fm,
         if (error) {
             return error;
         }
-        if (ofm->out_group != htonl(OFPG_ANY)) {
+        if ((ofm->command == OFPFC_DELETE
+             || ofm->command == OFPFC_DELETE_STRICT)
+            && ofm->out_group != htonl(OFPG_ANY)) {
             return OFPERR_OFPFMFC_UNKNOWN;
         }
         fm->flags = ntohs(ofm->flags);
@@ -4467,9 +4469,12 @@ ofputil_decode_queue_stats_request(const struct ofp_header *request,
     }
 
     case OFP10_VERSION: {
-        const struct ofp10_queue_stats_request *qsr11 = ofpmsg_body(request);
-        oqsr->queue_id = ntohl(qsr11->queue_id);
-        oqsr->port_no = ntohs(qsr11->port_no);
+        const struct ofp10_queue_stats_request *qsr10 = ofpmsg_body(request);
+        oqsr->queue_id = ntohl(qsr10->queue_id);
+        oqsr->port_no = ntohs(qsr10->port_no);
+        /* OF 1.0 uses OFPP_ALL for OFPP_ANY */
+        if (oqsr->port_no == OFPP_ALL)
+            oqsr->port_no = OFPP_ANY;
         return 0;
     }
 
@@ -4501,7 +4506,9 @@ ofputil_encode_queue_stats_request(enum ofp_version ofp_version,
         struct ofp10_queue_stats_request *req;
         request = ofpraw_alloc(OFPRAW_OFPST10_QUEUE_REQUEST, ofp_version, 0);
         req = ofpbuf_put_zeros(request, sizeof *req);
-        req->port_no = htons(oqsr->port_no);
+        /* OpenFlow 1.0 needs OFPP_ALL instead of OFPP_ANY */
+        req->port_no = htons(oqsr->port_no == OFPP_ANY
+                             ? OFPP_ALL : oqsr->port_no);
         req->queue_id = htonl(oqsr->queue_id);
         break;
     }
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index dabb590..5721dad 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -3054,7 +3054,7 @@ handle_queue_stats_request(struct ofconn *ofconn,
         return error;
     }
 
-    if (oqsr.port_no == OFPP_ALL) {
+    if (oqsr.port_no == OFPP_ANY) {
         error = OFPERR_OFPQOFC_BAD_QUEUE;
         HMAP_FOR_EACH (port, hmap_node, &ofproto->ports) {
             if (!handle_queue_stats_for_port(port, oqsr.queue_id, &cbdata)) {
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 363c0a3..5fc778e 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -968,7 +968,7 @@ ofctl_queue_stats(int argc, char *argv[])
     if (argc > 2 && argv[2][0] && strcasecmp(argv[2], "all")) {
         oqs.port_no = str_to_port_no(argv[1], argv[2]);
     } else {
-        oqs.port_no = OFPP_ALL;
+        oqs.port_no = OFPP_ANY;
     }
     if (argc > 3 && argv[3][0] && strcasecmp(argv[3], "all")) {
         oqs.queue_id = atoi(argv[3]);

 


More information about the dev mailing list