[ovs-dev] [PATCH OF12+ 1/4 v3] OpenFlow 1.2 fixes

Jarno Rajahalme jarno.rajahalme at nsn.com
Mon Nov 26 16:17:08 UTC 2012


   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*.

As part of the change to all clients to use OFPP_ANY regardless of version, this patch
changes "none" ports print out. "none" is still accepted on input for backwards compatibility,
but it prints out as "ANY". To make this less confusing, I changed the test cases to use
"controller" or "any" instead of "none". The test case that tests for both "none" and "controller"
still tests for them.

 include/openflow/openflow-1.1.h |    8 ++++++++
 lib/ofp-parse.c                 |    2 +-
 lib/ofp-print.c                 |    4 ++--
 lib/ofp-util.c                  |   25 +++++++++++++++++++------
 ofproto/ofproto.c               |   14 +++++++-------
 tests/ofp-print.at              |   10 +++++-----
 tests/ofproto.at                |   22 +++++++++++-----------
 utilities/ovs-ofctl.c           |    8 ++++----
 8 files changed, 57 insertions(+), 36 deletions(-)

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-parse.c b/lib/ofp-parse.c
index 054db60..1d71370 100644
--- a/lib/ofp-parse.c
+++ b/lib/ofp-parse.c
@@ -829,7 +829,7 @@ parse_ofp_str(struct ofputil_flow_mod *fm, int command, const char *str_,
     fm->idle_timeout = OFP_FLOW_PERMANENT;
     fm->hard_timeout = OFP_FLOW_PERMANENT;
     fm->buffer_id = UINT32_MAX;
-    fm->out_port = OFPP_NONE;
+    fm->out_port = OFPP_ANY;
     fm->flags = 0;
     if (fields & F_ACTIONS) {
         act_str = strstr(string, "action");
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index a0f04dc..1e35fba 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -765,7 +765,7 @@ ofp_print_flow_mod(struct ds *s, const struct ofp_header *oh, int verbosity)
     if (fm.buffer_id != UINT32_MAX) {
         ds_put_format(s, "buf:0x%"PRIx32" ", fm.buffer_id);
     }
-    if (fm.out_port != OFPP_NONE) {
+    if (fm.out_port != OFPP_ANY) {
         ds_put_format(s, "out_port:");
         ofputil_format_port(fm.out_port, s);
         ds_put_char(s, ' ');
@@ -1003,7 +1003,7 @@ ofp_print_flow_stats_request(struct ds *string, const struct ofp_header *oh)
         ds_put_format(string, " table=%"PRIu8, fsr.table_id);
     }
 
-    if (fsr.out_port != OFPP_NONE) {
+    if (fsr.out_port != OFPP_ANY) {
         ds_put_cstr(string, " out_port=");
         ofputil_format_port(fsr.out_port, string);
     }
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 4facf0a..bab6b2c 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);
@@ -3804,6 +3806,11 @@ ofputil_check_output_port(uint16_t port, int max_ports)
         OFPUTIL_NAMED_PORT(ALL)                 \
         OFPUTIL_NAMED_PORT(CONTROLLER)          \
         OFPUTIL_NAMED_PORT(LOCAL)               \
+        OFPUTIL_NAMED_PORT(ANY)
+
+/* For backwards compatibility, so that "none" is recognized as OFPP_ANY */
+#define OFPUTIL_NAMED_PORTS_WITH_NONE           \
+        OFPUTIL_NAMED_PORTS                     \
         OFPUTIL_NAMED_PORT(NONE)
 
 /* Stores the port number represented by 's' into '*portp'.  's' may be an
@@ -3863,7 +3870,7 @@ ofputil_port_from_string(const char *s, uint16_t *portp)
         };
         static const struct pair pairs[] = {
 #define OFPUTIL_NAMED_PORT(NAME) {#NAME, OFPP_##NAME},
-            OFPUTIL_NAMED_PORTS
+            OFPUTIL_NAMED_PORTS_WITH_NONE
 #undef OFPUTIL_NAMED_PORT
         };
         const struct pair *p;
@@ -4467,9 +4474,13 @@ 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 +4512,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..0992fe4 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2129,7 +2129,7 @@ ofproto_rule_destroy(struct rule *rule)
 bool
 ofproto_rule_has_out_port(const struct rule *rule, uint16_t port)
 {
-    return (port == OFPP_NONE
+    return (port == OFPP_ANY
             || ofpacts_output_to_port(rule->ofpacts, rule->ofpacts_len, port));
 }
 
@@ -2542,7 +2542,7 @@ handle_port_stats_request(struct ofconn *ofconn,
     }
 
     ofpmp_init(&replies, request);
-    if (port_no != OFPP_NONE) {
+    if (port_no != OFPP_ANY) {
         port = ofproto_get_port(p, port_no);
         if (port) {
             append_port_stat(port, &replies);
@@ -2659,7 +2659,7 @@ next_matching_table(const struct ofproto *ofproto,
  * OpenFlow OFPFC_MODIFY and OFPFC_DELETE requests and puts them on list
  * 'rules'.
  *
- * If 'out_port' is anything other than OFPP_NONE, then only rules that output
+ * If 'out_port' is anything other than OFPP_ANY, then only rules that output
  * to 'out_port' are included.
  *
  * Hidden rules are always omitted.
@@ -2710,7 +2710,7 @@ exit:
  * OpenFlow OFPFC_MODIFY_STRICT and OFPFC_DELETE_STRICT requests and puts them
  * on list 'rules'.
  *
- * If 'out_port' is anything other than OFPP_NONE, then only rules that output
+ * If 'out_port' is anything other than OFPP_ANY, then only rules that output
  * to 'out_port' are included.
  *
  * Hidden rules are always omitted.
@@ -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)) {
@@ -3327,7 +3327,7 @@ modify_flows_loose(struct ofproto *ofproto, struct ofconn *ofconn,
 
     error = collect_rules_loose(ofproto, fm->table_id, &fm->match,
                                 fm->cookie, fm->cookie_mask,
-                                OFPP_NONE, &rules);
+                                OFPP_ANY, &rules);
     if (error) {
         return error;
     } else if (list_is_empty(&rules)) {
@@ -3352,7 +3352,7 @@ modify_flow_strict(struct ofproto *ofproto, struct ofconn *ofconn,
 
     error = collect_rules_strict(ofproto, fm->table_id, &fm->match,
                                  fm->priority, fm->cookie, fm->cookie_mask,
-                                 OFPP_NONE, &rules);
+                                 OFPP_ANY, &rules);
 
     if (error) {
         return error;
diff --git a/tests/ofp-print.at b/tests/ofp-print.at
index bcf9d2c..7869a86 100644
--- a/tests/ofp-print.at
+++ b/tests/ofp-print.at
@@ -1149,7 +1149,7 @@ AT_CHECK([ovs-ofctl ofp-print "\
 01 10 00 14 00 00 00 01 00 05 00 00 ff fc 00 00 \
 ff ff ff ff \
 "], [0], [dnl
-OFPST_QUEUE request (xid=0x1):port=ALL queue=ALL
+OFPST_QUEUE request (xid=0x1):port=ANY queue=ALL
 ])
 AT_CLEANUP
 
@@ -1157,9 +1157,9 @@ AT_SETUP([OFPST_QUEUE request - OF1.1])
 AT_KEYWORDS([ofp-print OFPT_STATS_REQUEST])
 AT_CHECK([ovs-ofctl ofp-print "\
 02 12 00 18 00 00 00 02 00 05 00 00 00 00 00 00 \
-ff ff ff fc ff ff ff ff \
+ff ff ff ff ff ff ff ff \
 "], [0], [dnl
-OFPST_QUEUE request (OF1.1) (xid=0x2):port=ALL queue=ALL
+OFPST_QUEUE request (OF1.1) (xid=0x2):port=ANY queue=ALL
 ])
 AT_CLEANUP
 
@@ -1167,9 +1167,9 @@ AT_SETUP([OFPST_QUEUE request - OF1.2])
 AT_KEYWORDS([ofp-print OFPT_STATS_REQUEST])
 AT_CHECK([ovs-ofctl ofp-print "\
 03 12 00 18 00 00 00 02 00 05 00 00 00 00 00 00 \
-ff ff ff fc ff ff ff ff \
+ff ff ff ff ff ff ff ff \
 "], [0], [dnl
-OFPST_QUEUE request (OF1.2) (xid=0x2):port=ALL queue=ALL
+OFPST_QUEUE request (OF1.2) (xid=0x2):port=ANY queue=ALL
 ])
 AT_CLEANUP
 
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 38bc406..7f468bb 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -90,9 +90,9 @@ AT_CHECK([ovs-ofctl -vwarn queue-stats br0], [0], [stdout])
 AT_CHECK([STRIP_XIDS stdout], [0], [dnl
 OFPST_QUEUE reply: 0 queues
 ])
-AT_CHECK([ovs-ofctl -vwarn queue-stats br0 ALL 5], [0],
+AT_CHECK([ovs-ofctl -vwarn queue-stats br0 ANY 5], [0],
   [OFPT_ERROR (xid=0x2): OFPQOFC_BAD_QUEUE
-OFPST_QUEUE request (xid=0x2):port=ALL queue=5
+OFPST_QUEUE request (xid=0x2):port=ANY queue=5
 ])
 AT_CHECK([ovs-ofctl -vwarn queue-stats br0 10], [0],
   [OFPT_ERROR (xid=0x2): OFPQOFC_BAD_PORT
@@ -657,23 +657,23 @@ check_async () {
     : > expout
 
     # OFPT_PACKET_IN, OFPR_ACTION (controller_id=0)
-    ovs-ofctl -v packet-out br0 none controller '0001020304050010203040501234'
+    ovs-ofctl -v packet-out br0 controller controller '0001020304050010203040501234'
     if test X"$1" = X"OFPR_ACTION"; then shift;
-        echo >>expout "OFPT_PACKET_IN: total_len=14 in_port=NONE (via action) data_len=14 (unbuffered)
+        echo >>expout "OFPT_PACKET_IN: total_len=14 in_port=CONTROLLER (via action) data_len=14 (unbuffered)
 priority=0,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x1234"
     fi
 
     # OFPT_PACKET_IN, OFPR_NO_MATCH (controller_id=123)
-    ovs-ofctl -v packet-out br0 none 'controller(reason=no_match,id=123)' '0001020304050010203040501234'
+    ovs-ofctl -v packet-out br0 controller 'controller(reason=no_match,id=123)' '0001020304050010203040501234'
     if test X"$1" = X"OFPR_NO_MATCH"; then shift;
-        echo >>expout "OFPT_PACKET_IN: total_len=14 in_port=NONE (via no_match) data_len=14 (unbuffered)
+        echo >>expout "OFPT_PACKET_IN: total_len=14 in_port=CONTROLLER (via no_match) data_len=14 (unbuffered)
 priority=0,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x1234"
     fi
 
     # OFPT_PACKET_IN, OFPR_INVALID_TTL (controller_id=0)
-    ovs-ofctl packet-out br0 none dec_ttl '002583dfb4000026b98cb0f908004500003fb7e200000011339bac11370dac100002d7730035002b8f6d86fb0100000100000000000006626c702d7873066e696369726103636f6d00000f00'
+    ovs-ofctl packet-out br0 controller dec_ttl '002583dfb4000026b98cb0f908004500003fb7e200000011339bac11370dac100002d7730035002b8f6d86fb0100000100000000000006626c702d7873066e696369726103636f6d00000f00'
     if test X"$1" = X"OFPR_INVALID_TTL"; then shift;
-        echo >>expout "OFPT_PACKET_IN: total_len=76 in_port=NONE (via invalid_ttl) data_len=76 (unbuffered)
+        echo >>expout "OFPT_PACKET_IN: total_len=76 in_port=CONTROLLER (via invalid_ttl) data_len=76 (unbuffered)
 priority=0,udp,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:26:b9:8c:b0:f9,dl_dst=00:25:83:df:b4:00,nw_src=172.17.55.13,nw_dst=172.16.0.2,nw_tos=0,nw_ecn=0,nw_ttl=0,tp_src=55155,tp_dst=53 udp_csum:8f6d"
     fi
 
@@ -771,7 +771,7 @@ ovs-appctl -t ovs-ofctl ofctl/barrier
 ovs-appctl -t ovs-ofctl exit
 
 AT_CHECK([sed 's/ (xid=0x[[0-9a-fA-F]]*)//' monitor.log], [0], [dnl
-OFPT_PACKET_IN: total_len=14 in_port=NONE (via action) data_len=14 (unbuffered)
+OFPT_PACKET_IN: total_len=14 in_port=ANY (via action) data_len=14 (unbuffered)
 priority=0,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x1234
 OFPT_PACKET_IN: total_len=14 in_port=CONTROLLER (via action) data_len=14 (unbuffered)
 priority=0,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x5678
@@ -794,14 +794,14 @@ ovs-appctl -t ovs-ofctl ofctl/set-output-file monitor.log
 AT_CAPTURE_FILE([monitor.log])
 
 # Send a packet-out with a load action to set some metadata, and forward to controller
-AT_CHECK([ovs-ofctl packet-out br0 none 'load(0xfafafafa5a5a5a5a->OXM_OF_METADATA[[0..63]]), controller' '0001020304050010203040501234'])
+AT_CHECK([ovs-ofctl packet-out br0 controller 'load(0xfafafafa5a5a5a5a->OXM_OF_METADATA[[0..63]]), controller' '0001020304050010203040501234'])
 
 # Stop the monitor and check its output.
 ovs-appctl -t ovs-ofctl ofctl/barrier
 ovs-appctl -t ovs-ofctl exit
 
 AT_CHECK([sed 's/ (xid=0x[[0-9a-fA-F]]*)//' monitor.log], [0], [dnl
-NXT_PACKET_IN: total_len=14 in_port=NONE metadata=0xfafafafa5a5a5a5a (via action) data_len=14 (unbuffered)
+NXT_PACKET_IN: total_len=14 in_port=CONTROLLER metadata=0xfafafafa5a5a5a5a (via action) data_len=14 (unbuffered)
 priority=0,metadata=0,in_port=0,vlan_tci=0x0000,dl_src=00:10:20:30:40:50,dl_dst=00:01:02:03:04:05,dl_type=0x1234
 OFPT_BARRIER_REPLY:
 ])
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index 363c0a3..5037398 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]);
@@ -1430,7 +1430,7 @@ ofctl_dump_ports(int argc, char *argv[])
     uint16_t port;
 
     open_vconn(argv[1], &vconn);
-    port = argc > 2 ? str_to_port_no(argv[1], argv[2]) : OFPP_NONE;
+    port = argc > 2 ? str_to_port_no(argv[1], argv[2]) : OFPP_ANY;
     request = ofputil_encode_dump_ports_request(vconn_get_version(vconn), port);
     dump_stats_transaction(vconn, request);
     vconn_close(vconn);
@@ -1940,7 +1940,7 @@ read_flows_from_switch(struct vconn *vconn,
 
     fsr.aggregate = false;
     match_init_catchall(&fsr.match);
-    fsr.out_port = OFPP_NONE;
+    fsr.out_port = OFPP_ANY;
     fsr.table_id = 0xff;
     fsr.cookie = fsr.cookie_mask = htonll(0);
     request = ofputil_encode_flow_stats_request(&fsr, protocol);
@@ -1983,7 +1983,7 @@ fte_make_flow_mod(const struct fte *fte, int index, uint16_t command,
     fm.idle_timeout = version->idle_timeout;
     fm.hard_timeout = version->hard_timeout;
     fm.buffer_id = UINT32_MAX;
-    fm.out_port = OFPP_NONE;
+    fm.out_port = OFPP_ANY;
     fm.flags = version->flags;
     if (command == OFPFC_ADD || command == OFPFC_MODIFY ||
         command == OFPFC_MODIFY_STRICT) {




More information about the dev mailing list