ofp-util: Rename OFPUTIL_P_* constants and update comments for clarity.

It wasn't clear to me, at least, whether an OFPUTIL_P_* constant
indicated an OpenFlow version and a flow format or just a flow format.
After some reflection, I think it's more useful if it indicates both,
because otherwise it might be necessary to pass both an OpenFlow version
and an OFPUTIL_P_* constant in some contexts, but this way only the latter
is needed.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Reviewed-by: Simon Horman <horms@verge.net.au>

diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index dea4aeafce0ce006e6ec56b69fb9ecba3fd4eee6..c2e97384639a14b9fd1e51a7a26b0a99ac201c55 100644 (file)
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -581,15 +581,15 @@ struct proto_abbrev {
 /* Most users really don't care about some of the differences between
  * protocols.  These abbreviations help with that. */
 static const struct proto_abbrev proto_abbrevs[] = {
-    { OFPUTIL_P_ANY,      "any" },
-    { OFPUTIL_P_OF10_ANY, "OpenFlow10" },
-    { OFPUTIL_P_NXM_ANY,  "NXM" },
+    { OFPUTIL_P_ANY,          "any" },
+    { OFPUTIL_P_OF10_STD_ANY, "OpenFlow10" },
+    { OFPUTIL_P_OF10_NXM_ANY,  "NXM" },
 };
 #define N_PROTO_ABBREVS ARRAY_SIZE(proto_abbrevs)
 
 enum ofputil_protocol ofputil_flow_dump_protocols[] = {
-    OFPUTIL_P_NXM,
-    OFPUTIL_P_OF10,
+    OFPUTIL_P_OF10_NXM,
+    OFPUTIL_P_OF10_STD,
 };
 size_t ofputil_n_flow_dump_protocols = ARRAY_SIZE(ofputil_flow_dump_protocols);
 
@@ -603,9 +603,9 @@ ofputil_protocol_from_ofp_version(enum ofp_version version)
 {
     switch (version) {
     case OFP10_VERSION:
-        return OFPUTIL_P_OF10;
+        return OFPUTIL_P_OF10_STD;
     case OFP12_VERSION:
-        return OFPUTIL_P_OF12;
+        return OFPUTIL_P_OF12_OXM;
     case OFP11_VERSION:
     default:
         return 0;
@@ -618,12 +618,12 @@ enum ofp_version
 ofputil_protocol_to_ofp_version(enum ofputil_protocol protocol)
 {
     switch (protocol) {
-    case OFPUTIL_P_OF10:
-    case OFPUTIL_P_OF10_TID:
-    case OFPUTIL_P_NXM:
-    case OFPUTIL_P_NXM_TID:
+    case OFPUTIL_P_OF10_STD:
+    case OFPUTIL_P_OF10_STD_TID:
+    case OFPUTIL_P_OF10_NXM:
+    case OFPUTIL_P_OF10_NXM_TID:
         return OFP10_VERSION;
-    case OFPUTIL_P_OF12:
+    case OFPUTIL_P_OF12_OXM:
         return OFP12_VERSION;
     }
 
@@ -652,16 +652,16 @@ enum ofputil_protocol
 ofputil_protocol_set_tid(enum ofputil_protocol protocol, bool enable)
 {
     switch (protocol) {
-    case OFPUTIL_P_OF10:
-    case OFPUTIL_P_OF10_TID:
-        return enable ? OFPUTIL_P_OF10_TID : OFPUTIL_P_OF10;
+    case OFPUTIL_P_OF10_STD:
+    case OFPUTIL_P_OF10_STD_TID:
+        return enable ? OFPUTIL_P_OF10_STD_TID : OFPUTIL_P_OF10_STD;
 
-    case OFPUTIL_P_NXM:
-    case OFPUTIL_P_NXM_TID:
-        return enable ? OFPUTIL_P_NXM_TID : OFPUTIL_P_NXM;
+    case OFPUTIL_P_OF10_NXM:
+    case OFPUTIL_P_OF10_NXM_TID:
+        return enable ? OFPUTIL_P_OF10_NXM_TID : OFPUTIL_P_OF10_NXM;
 
-    case OFPUTIL_P_OF12:
-        return OFPUTIL_P_OF12;
+    case OFPUTIL_P_OF12_OXM:
+        return OFPUTIL_P_OF12_OXM;
 
     default:
         NOT_REACHED();
@@ -686,16 +686,16 @@ ofputil_protocol_set_base(enum ofputil_protocol cur,
     bool tid = (cur & OFPUTIL_P_TID) != 0;
 
     switch (new_base) {
-    case OFPUTIL_P_OF10:
-    case OFPUTIL_P_OF10_TID:
-        return ofputil_protocol_set_tid(OFPUTIL_P_OF10, tid);
+    case OFPUTIL_P_OF10_STD:
+    case OFPUTIL_P_OF10_STD_TID:
+        return ofputil_protocol_set_tid(OFPUTIL_P_OF10_STD, tid);
 
-    case OFPUTIL_P_NXM:
-    case OFPUTIL_P_NXM_TID:
-        return ofputil_protocol_set_tid(OFPUTIL_P_NXM, tid);
+    case OFPUTIL_P_OF10_NXM:
+    case OFPUTIL_P_OF10_NXM_TID:
+        return ofputil_protocol_set_tid(OFPUTIL_P_OF10_NXM, tid);
 
-    case OFPUTIL_P_OF12:
-        return ofputil_protocol_set_tid(OFPUTIL_P_OF12, tid);
+    case OFPUTIL_P_OF12_OXM:
+        return ofputil_protocol_set_tid(OFPUTIL_P_OF12_OXM, tid);
 
     default:
         NOT_REACHED();
@@ -713,19 +713,19 @@ ofputil_protocol_to_string(enum ofputil_protocol protocol)
     /* Use a "switch" statement for single-bit names so that we get a compiler
      * warning if we forget any. */
     switch (protocol) {
-    case OFPUTIL_P_NXM:
+    case OFPUTIL_P_OF10_NXM:
         return "NXM-table_id";
 
-    case OFPUTIL_P_NXM_TID:
+    case OFPUTIL_P_OF10_NXM_TID:
         return "NXM+table_id";
 
-    case OFPUTIL_P_OF10:
+    case OFPUTIL_P_OF10_STD:
         return "OpenFlow10-table_id";
 
-    case OFPUTIL_P_OF10_TID:
+    case OFPUTIL_P_OF10_STD_TID:
         return "OpenFlow10+table_id";
 
-    case OFPUTIL_P_OF12:
+    case OFPUTIL_P_OF12_OXM:
         return NULL;
     }
 
@@ -972,68 +972,68 @@ ofputil_usable_protocols(const struct match *match)
     /* NXM and OF1.1+ supports bitwise matching on ethernet addresses. */
     if (!eth_mask_is_exact(wc->masks.dl_src)
         && !eth_addr_is_zero(wc->masks.dl_src)) {
-        return OFPUTIL_P_NXM_ANY;
+        return OFPUTIL_P_OF10_NXM_ANY;
     }
     if (!eth_mask_is_exact(wc->masks.dl_dst)
         && !eth_addr_is_zero(wc->masks.dl_dst)) {
-        return OFPUTIL_P_NXM_ANY;
+        return OFPUTIL_P_OF10_NXM_ANY;
     }
 
     /* NXM and OF1.1+ support matching metadata. */
     if (wc->masks.metadata != htonll(0)) {
-        return OFPUTIL_P_NXM_ANY;
+        return OFPUTIL_P_OF10_NXM_ANY;
     }
 
     /* Only NXM supports matching ARP hardware addresses. */
     if (!eth_addr_is_zero(wc->masks.arp_sha) ||
         !eth_addr_is_zero(wc->masks.arp_tha)) {
-        return OFPUTIL_P_NXM_ANY;
+        return OFPUTIL_P_OF10_NXM_ANY;
     }
 
     /* Only NXM supports matching IPv6 traffic. */
     if (match->flow.dl_type == htons(ETH_TYPE_IPV6)) {
-        return OFPUTIL_P_NXM_ANY;
+        return OFPUTIL_P_OF10_NXM_ANY;
     }
 
     /* Only NXM supports matching registers. */
     if (!regs_fully_wildcarded(wc)) {
-        return OFPUTIL_P_NXM_ANY;
+        return OFPUTIL_P_OF10_NXM_ANY;
     }
 
     /* Only NXM supports matching tun_id. */
     if (wc->masks.tunnel.tun_id != htonll(0)) {
-        return OFPUTIL_P_NXM_ANY;
+        return OFPUTIL_P_OF10_NXM_ANY;
     }
 
     /* Only NXM supports matching fragments. */
     if (wc->masks.nw_frag) {
-        return OFPUTIL_P_NXM_ANY;
+        return OFPUTIL_P_OF10_NXM_ANY;
     }
 
     /* Only NXM supports matching IPv6 flow label. */
     if (wc->masks.ipv6_label) {
-        return OFPUTIL_P_NXM_ANY;
+        return OFPUTIL_P_OF10_NXM_ANY;
     }
 
     /* Only NXM supports matching IP ECN bits. */
     if (wc->masks.nw_tos & IP_ECN_MASK) {
-        return OFPUTIL_P_NXM_ANY;
+        return OFPUTIL_P_OF10_NXM_ANY;
     }
 
     /* Only NXM supports matching IP TTL/hop limit. */
     if (wc->masks.nw_ttl) {
-        return OFPUTIL_P_NXM_ANY;
+        return OFPUTIL_P_OF10_NXM_ANY;
     }
 
     /* Only NXM supports non-CIDR IPv4 address masks. */
     if (!ip_is_cidr(wc->masks.nw_src) || !ip_is_cidr(wc->masks.nw_dst)) {
-        return OFPUTIL_P_NXM_ANY;
+        return OFPUTIL_P_OF10_NXM_ANY;
     }
 
     /* Only NXM supports bitwise matching on transport port. */
     if ((wc->masks.tp_src && wc->masks.tp_src != htons(UINT16_MAX)) ||
         (wc->masks.tp_dst && wc->masks.tp_dst != htons(UINT16_MAX))) {
-        return OFPUTIL_P_NXM_ANY;
+        return OFPUTIL_P_OF10_NXM_ANY;
     }
 
     /* Other formats can express this rule. */
@@ -1216,17 +1216,17 @@ ofputil_encode_set_protocol(enum ofputil_protocol current,
         *next = ofputil_protocol_set_base(current, want_base);
 
         switch (want_base) {
-        case OFPUTIL_P_NXM:
+        case OFPUTIL_P_OF10_NXM:
             return ofputil_encode_nx_set_flow_format(NXFF_NXM);
 
-        case OFPUTIL_P_OF10:
+        case OFPUTIL_P_OF10_STD:
             return ofputil_encode_nx_set_flow_format(NXFF_OPENFLOW10);
 
-        case OFPUTIL_P_OF12:
+        case OFPUTIL_P_OF12_OXM:
             return ofputil_encode_nx_set_flow_format(NXFF_OPENFLOW12);
 
-        case OFPUTIL_P_OF10_TID:
-        case OFPUTIL_P_NXM_TID:
+        case OFPUTIL_P_OF10_STD_TID:
+        case OFPUTIL_P_OF10_NXM_TID:
             NOT_REACHED();
         }
     }
@@ -1268,13 +1268,13 @@ ofputil_nx_flow_format_to_protocol(enum nx_flow_format flow_format)
 {
     switch (flow_format) {
     case NXFF_OPENFLOW10:
-        return OFPUTIL_P_OF10;
+        return OFPUTIL_P_OF10_STD;
 
     case NXFF_NXM:
-        return OFPUTIL_P_NXM;
+        return OFPUTIL_P_OF10_NXM;
 
     case NXFF_OPENFLOW12:
-        return OFPUTIL_P_OF12;
+        return OFPUTIL_P_OF12_OXM;
 
     default:
         return 0;
@@ -1499,7 +1499,7 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm,
     struct ofpbuf *msg;
 
     switch (protocol) {
-    case OFPUTIL_P_OF12: {
+    case OFPUTIL_P_OF12_OXM: {
         struct ofp11_flow_mod *ofm;
 
         msg = ofpraw_alloc(OFPRAW_OFPT11_FLOW_MOD, OFP12_VERSION,
@@ -1525,8 +1525,8 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm,
         break;
     }
 
-    case OFPUTIL_P_OF10:
-    case OFPUTIL_P_OF10_TID: {
+    case OFPUTIL_P_OF10_STD:
+    case OFPUTIL_P_OF10_STD_TID: {
         struct ofp10_flow_mod *ofm;
 
         msg = ofpraw_alloc(OFPRAW_OFPT10_FLOW_MOD, OFP10_VERSION,
@@ -1545,8 +1545,8 @@ ofputil_encode_flow_mod(const struct ofputil_flow_mod *fm,
         break;
     }
 
-    case OFPUTIL_P_NXM:
-    case OFPUTIL_P_NXM_TID: {
+    case OFPUTIL_P_OF10_NXM:
+    case OFPUTIL_P_OF10_NXM_TID: {
         struct nx_flow_mod *nfm;
         int match_len;
 
@@ -1599,7 +1599,7 @@ ofputil_flow_mod_usable_protocols(const struct ofputil_flow_mod *fms,
 
         /* Matching of the cookie is only supported through NXM. */
         if (fm->cookie_mask != htonll(0)) {
-            usable_protocols &= OFPUTIL_P_NXM_ANY;
+            usable_protocols &= OFPUTIL_P_OF10_NXM_ANY;
         }
     }
     assert(usable_protocols);
@@ -1720,7 +1720,7 @@ ofputil_encode_flow_stats_request(const struct ofputil_flow_stats_request *fsr,
     enum ofpraw raw;
 
     switch (protocol) {
-    case OFPUTIL_P_OF12: {
+    case OFPUTIL_P_OF12_OXM: {
         struct ofp11_flow_stats_request *ofsr;
 
         raw = (fsr->aggregate
@@ -1737,8 +1737,8 @@ ofputil_encode_flow_stats_request(const struct ofputil_flow_stats_request *fsr,
         break;
     }
 
-    case OFPUTIL_P_OF10:
-    case OFPUTIL_P_OF10_TID: {
+    case OFPUTIL_P_OF10_STD:
+    case OFPUTIL_P_OF10_STD_TID: {
         struct ofp10_flow_stats_request *ofsr;
 
         raw = (fsr->aggregate
@@ -1752,8 +1752,8 @@ ofputil_encode_flow_stats_request(const struct ofputil_flow_stats_request *fsr,
         break;
     }
 
-    case OFPUTIL_P_NXM:
-    case OFPUTIL_P_NXM_TID: {
+    case OFPUTIL_P_OF10_NXM:
+    case OFPUTIL_P_OF10_NXM_TID: {
         struct nx_flow_stats_request *nfsr;
         int match_len;
 
@@ -1791,7 +1791,7 @@ ofputil_flow_stats_request_usable_protocols(
 
     usable_protocols = ofputil_usable_protocols(&fsr->match);
     if (fsr->cookie_mask != htonll(0)) {
-        usable_protocols &= OFPUTIL_P_NXM_ANY;
+        usable_protocols &= OFPUTIL_P_OF10_NXM_ANY;
     }
     return usable_protocols;
 }
@@ -2203,7 +2203,7 @@ ofputil_encode_flow_removed(const struct ofputil_flow_removed *fr,
     struct ofpbuf *msg;
 
     switch (protocol) {
-    case OFPUTIL_P_OF12: {
+    case OFPUTIL_P_OF12_OXM: {
         struct ofp12_flow_removed *ofr;
 
         msg = ofpraw_alloc_xid(OFPRAW_OFPT11_FLOW_REMOVED,
@@ -2224,8 +2224,8 @@ ofputil_encode_flow_removed(const struct ofputil_flow_removed *fr,
         break;
     }
 
-    case OFPUTIL_P_OF10:
-    case OFPUTIL_P_OF10_TID: {
+    case OFPUTIL_P_OF10_STD:
+    case OFPUTIL_P_OF10_STD_TID: {
         struct ofp_flow_removed *ofr;
 
         msg = ofpraw_alloc_xid(OFPRAW_OFPT10_FLOW_REMOVED, OFP10_VERSION,
@@ -2243,8 +2243,8 @@ ofputil_encode_flow_removed(const struct ofputil_flow_removed *fr,
         break;
     }
 
-    case OFPUTIL_P_NXM:
-    case OFPUTIL_P_NXM_TID: {
+    case OFPUTIL_P_OF10_NXM:
+    case OFPUTIL_P_OF10_NXM_TID: {
         struct nx_flow_removed *nfr;
         int match_len;
 
@@ -2396,7 +2396,7 @@ ofputil_encode_packet_in(const struct ofputil_packet_in *pin,
     struct ofpbuf *packet;
 
     /* Add OFPT_PACKET_IN. */
-    if (protocol == OFPUTIL_P_OF12) {
+    if (protocol == OFPUTIL_P_OF12_OXM) {
         struct ofp12_packet_in *opi;
         struct match match;
 

diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 4bd5a0014e98d3ccc2b14395cc1ae2a3ddd84d60..a081cbf0bd26467c7ffffdd2a762993429ffdbe0 100644 (file)
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -45,12 +45,15 @@ ovs_be32 ofputil_wcbits_to_netmask(int wcbits);
 int ofputil_netmask_to_wcbits(ovs_be32 netmask);
 
 /* Protocols.
+ *
+ * A "protocol" is an OpenFlow version plus, for some OpenFlow versions,
+ * a bit extra about the flow match format in use.
  *
  * These are arranged from most portable to least portable, or alternatively
- * from least powerful to most powerful.  Formats earlier on the list are more
- * likely to be understood for the purpose of making requests, but formats
- * later on the list are more likely to accurately describe a flow within a
- * switch.
+ * from least powerful to most powerful.  Protocols earlier on the list are
+ * more likely to be understood for the purpose of making requests, but
+ * protocol later on the list are more likely to accurately describe a flow
+ * within a switch.
  *
  * On any given OpenFlow connection, a single protocol is in effect at any
  * given time.  These values use separate bits only because that makes it easy
@@ -58,24 +61,35 @@ int ofputil_netmask_to_wcbits(ovs_be32 netmask);
  * to implement set union and intersection.
  */
 enum ofputil_protocol {
-    /* OpenFlow 1.0-based protocols. */
-    OFPUTIL_P_OF10     = 1 << 0, /* OpenFlow 1.0 flow format. */
-    OFPUTIL_P_OF10_TID = 1 << 1, /* OF1.0 + flow_mod_table_id extension. */
-#define OFPUTIL_P_OF10_ANY (OFPUTIL_P_OF10 | OFPUTIL_P_OF10_TID)
-
-    /* OpenFlow 1.0 with NXM-based flow formats. */
-    OFPUTIL_P_NXM      = 1 << 2, /* Nicira extended match. */
-    OFPUTIL_P_NXM_TID  = 1 << 3, /* NXM + flow_mod_table_id extension. */
-#define OFPUTIL_P_NXM_ANY (OFPUTIL_P_NXM | OFPUTIL_P_NXM_TID)
-
-    /* OpenFlow 1.2 */
-    OFPUTIL_P_OF12      = 1 << 4, /* OpenFlow 1.2 flow format. */
+    /* OpenFlow 1.0 protocols.
+     *
+     * The "STD" protocols use the standard OpenFlow 1.0 flow format.
+     * The "NXM" protocols use the Nicira Extensible Match (NXM) flow format.
+     *
+     * The protocols with "TID" mean that the nx_flow_mod_table_id Nicira
+     * extension has been enabled.  The other protocols have it disabled.
+     */
+    OFPUTIL_P_OF10_STD     = 1 << 0,
+    OFPUTIL_P_OF10_STD_TID = 1 << 1,
+    OFPUTIL_P_OF10_NXM     = 1 << 2,
+    OFPUTIL_P_OF10_NXM_TID = 1 << 3,
+#define OFPUTIL_P_OF10_STD_ANY (OFPUTIL_P_OF10_STD | OFPUTIL_P_OF10_STD_TID)
+#define OFPUTIL_P_OF10_NXM_ANY (OFPUTIL_P_OF10_NXM | OFPUTIL_P_OF10_NXM_TID)
+
+    /* OpenFlow 1.2 protocol (only one variant).
+     *
+     * This uses the standard OpenFlow Extensible Match (OXM) flow format.
+     *
+     * OpenFlow 1.2 always operates with an equivalent of the
+     * nx_flow_mod_table_id Nicira extension enabled, so there is no "TID"
+     * variant. */
+    OFPUTIL_P_OF12_OXM      = 1 << 4,
 
     /* All protocols. */
-#define OFPUTIL_P_ANY (OFPUTIL_P_OF10_ANY | OFPUTIL_P_NXM_ANY)
+#define OFPUTIL_P_ANY (OFPUTIL_P_OF10_STD_ANY | OFPUTIL_P_OF10_NXM_ANY)
 
     /* Protocols in which a specific table may be specified in flow_mods. */
-#define OFPUTIL_P_TID (OFPUTIL_P_OF10_TID | OFPUTIL_P_NXM_TID)
+#define OFPUTIL_P_TID (OFPUTIL_P_OF10_STD_TID | OFPUTIL_P_OF10_NXM_TID)
 };
 
 /* Protocols to use for flow dumps, from most to least preferred. */

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index ba93a1d58df1e261c417e15fe07ddc767864120a..288580460f94e433103e7a2013e0181c7188bb4b 100644 (file)
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -840,7 +840,7 @@ ofconn_get_invalid_ttl_to_controller(struct ofconn *ofconn)
 
 /* Returns the currently configured protocol for 'ofconn', one of OFPUTIL_P_*.
  *
- * The default, if no other format has been set, is OFPUTIL_P_OPENFLOW10. */
+ * The default, if no other format has been set, is OFPUTIL_P_OF10_STD. */
 enum ofputil_protocol
 ofconn_get_protocol(struct ofconn *ofconn)
 {
@@ -1034,7 +1034,7 @@ ofconn_flush(struct ofconn *ofconn)
     int i;
 
     ofconn->role = NX_ROLE_OTHER;
-    ofconn->protocol = OFPUTIL_P_OF10;
+    ofconn->protocol = OFPUTIL_P_OF10_STD;
     ofconn->packet_in_format = NXPIF_OPENFLOW10;
 
     /* Disassociate 'ofconn' from all of the ofopgroups that it initiated that