[ovs-dev] [ext-272 3/4] ofp-util: Simplify struct ofputil_role_request.

Ben Pfaff blp at nicira.com
Tue Feb 12 08:04:00 UTC 2013


It makes more sense to use enum ofp12_controller_role here than
to use enum nx_role, because the former is a superset of the latter and
we can then get rid of a bool member too.

Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 lib/ofp-print.c   |   15 ++++----
 lib/ofp-util.c    |  101 ++++++++++++++++++++++++++++-------------------------
 lib/ofp-util.h    |    7 ++--
 ofproto/connmgr.c |   29 +++++++--------
 ofproto/connmgr.h |    7 ++--
 ofproto/ofproto.c |   42 ++++++++++------------
 ofproto/ofproto.h |    4 +--
 vswitchd/bridge.c |   12 ++++---
 8 files changed, 111 insertions(+), 106 deletions(-)

diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 1a9ca0e..f7872cb 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -1550,19 +1550,18 @@ ofp_print_role_message(struct ds *string, const struct ofp_header *oh)
     }
 
     ds_put_cstr(string, " role=");
-    if (rr.request_current_role_only) {
-        ds_put_cstr(string, "nochange");
-        return;
-    }
 
     switch (rr.role) {
-    case NX_ROLE_OTHER:
+    case OFPCR12_ROLE_NOCHANGE:
+        ds_put_cstr(string, "nochange");
+        break;
+    case OFPCR12_ROLE_EQUAL:
         ds_put_cstr(string, "equal"); /* OF 1.2 wording */
         break;
-    case NX_ROLE_MASTER:
+    case OFPCR12_ROLE_MASTER:
         ds_put_cstr(string, "master");
         break;
-    case NX_ROLE_SLAVE:
+    case OFPCR12_ROLE_SLAVE:
         ds_put_cstr(string, "slave");
         break;
     default:
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index cd51ef7..ef8de6b 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -3382,43 +3382,53 @@ enum ofperr
 ofputil_decode_role_message(const struct ofp_header *oh,
                             struct ofputil_role_request *rr)
 {
-    const struct ofp12_role_request *orr = ofpmsg_body(oh);
-    uint32_t role = ntohl(orr->role);
     struct ofpbuf b;
     enum ofpraw raw;
 
-    memset(rr, 0, sizeof *rr);
-
     ofpbuf_use_const(&b, oh, ntohs(oh->length));
     raw = ofpraw_pull_assert(&b);
 
-    if (raw == OFPRAW_OFPT12_ROLE_REQUEST
-        || raw == OFPRAW_OFPT12_ROLE_REPLY) {
+    if (raw == OFPRAW_OFPT12_ROLE_REQUEST ||
+        raw == OFPRAW_OFPT12_ROLE_REPLY) {
+        const struct ofp12_role_request *orr = b.l3;
 
-        if (raw == OFPRAW_OFPT12_ROLE_REQUEST) {
-            if (role == OFPCR12_ROLE_NOCHANGE) {
-                rr->request_current_role_only = true;
-                return 0;
-            }
-            if (role == OFPCR12_ROLE_MASTER || role == OFPCR12_ROLE_SLAVE) {
-                rr->generation_id = ntohll(orr->generation_id);
-                rr->have_generation_id = true;
-            }
+        if (orr->role != htonl(OFPCR12_ROLE_NOCHANGE) &&
+            orr->role != htonl(OFPCR12_ROLE_EQUAL) &&
+            orr->role != htonl(OFPCR12_ROLE_MASTER) &&
+            orr->role != htonl(OFPCR12_ROLE_SLAVE)) {
+            return OFPERR_OFPRRFC_BAD_ROLE;
         }
 
-        /* Map to enum nx_role */
-        role -= 1; /* OFPCR12_ROLE_MASTER -> NX_ROLE_MASTER etc. */
-    } else if (raw != OFPRAW_NXT_ROLE_REQUEST
-               && raw != OFPRAW_NXT_ROLE_REPLY) {
-        return OFPERR_OFPBRC_BAD_TYPE;
-    }
+        rr->role = ntohl(orr->role);
+        if (raw == OFPRAW_OFPT12_ROLE_REPLY
+            || orr->role == htonl(OFPCR12_ROLE_NOCHANGE)) {
+            rr->have_generation_id = false;
+            rr->generation_id = 0;
+        } else {
+            rr->have_generation_id = true;
+            rr->generation_id = ntohll(orr->generation_id);
+        }
+    } else if (raw == OFPRAW_NXT_ROLE_REQUEST ||
+               raw == OFPRAW_NXT_ROLE_REPLY) {
+        const struct nx_role_request *nrr = b.l3;
+
+        BUILD_ASSERT(NX_ROLE_OTHER + 1 == OFPCR12_ROLE_EQUAL);
+        BUILD_ASSERT(NX_ROLE_MASTER + 1 == OFPCR12_ROLE_MASTER);
+        BUILD_ASSERT(NX_ROLE_SLAVE + 1 == OFPCR12_ROLE_SLAVE);
+
+        if (nrr->role != htonl(NX_ROLE_OTHER) &&
+            nrr->role != htonl(NX_ROLE_MASTER) &&
+            nrr->role != htonl(NX_ROLE_SLAVE)) {
+            return OFPERR_OFPRRFC_BAD_ROLE;
+        }
 
-    if (role != NX_ROLE_OTHER && role != NX_ROLE_MASTER
-        && role != NX_ROLE_SLAVE) {
-        return OFPERR_OFPRRFC_BAD_ROLE;
+        rr->role = ntohl(nrr->role) + 1;
+        rr->have_generation_id = false;
+        rr->generation_id = 0;
+    } else {
+        NOT_REACHED();
     }
 
-    rr->role = role;
     return 0;
 }
 
@@ -3426,34 +3436,19 @@ ofputil_decode_role_message(const struct ofp_header *oh,
  * buffer owned by the caller. */
 struct ofpbuf *
 ofputil_encode_role_reply(const struct ofp_header *request,
-                          enum nx_role role)
+                          const struct ofputil_role_request *rr)
 {
-    struct ofp12_role_request *reply;
     struct ofpbuf *buf;
-    size_t reply_size;
-
-    struct ofpbuf b;
     enum ofpraw raw;
 
-    ofpbuf_use_const(&b, request, ntohs(request->length));
-    raw = ofpraw_pull_assert(&b);
+    raw = ofpraw_decode_assert(request);
     if (raw == OFPRAW_OFPT12_ROLE_REQUEST) {
-        reply_size = sizeof (struct ofp12_role_request);
-        raw = OFPRAW_OFPT12_ROLE_REPLY;
-    }
-    else if (raw == OFPRAW_NXT_ROLE_REQUEST) {
-        reply_size = sizeof (struct nx_role_request);
-        raw = OFPRAW_NXT_ROLE_REPLY;
-    } else {
-        NOT_REACHED();
-    }
+        struct ofp12_role_request *orr;
 
-    buf = ofpraw_alloc_reply(raw, request, 0);
-    reply = ofpbuf_put_zeros(buf, reply_size);
+        buf = ofpraw_alloc_reply(OFPRAW_OFPT12_ROLE_REPLY, request, 0);
+        orr = ofpbuf_put_zeros(buf, sizeof *orr);
+        orr->role = htonl(rr->role);
 
-    if (raw == OFPRAW_OFPT12_ROLE_REPLY) {
-        /* Map to OpenFlow enum ofp12_controller_role */
-        role += 1; /* NX_ROLE_MASTER -> OFPCR12_ROLE_MASTER etc. */
         /*
          * OpenFlow specification does not specify use of generation_id field
          * on reply messages.  Intuitively, it would seem a good idea to return
@@ -3465,8 +3460,20 @@ ofputil_encode_role_reply(const struct ofp_header *request,
          * A request for clarification has been filed with the Open Networking
          * Foundation as EXT-272.
          */
+        orr->generation_id = htonll(0);
+    } else if (raw == OFPRAW_NXT_ROLE_REQUEST) {
+        struct nx_role_request *nrr;
+
+        BUILD_ASSERT(NX_ROLE_OTHER == OFPCR12_ROLE_EQUAL - 1);
+        BUILD_ASSERT(NX_ROLE_MASTER == OFPCR12_ROLE_MASTER - 1);
+        BUILD_ASSERT(NX_ROLE_SLAVE == OFPCR12_ROLE_SLAVE - 1);
+
+        buf = ofpraw_alloc_reply(OFPRAW_NXT_ROLE_REPLY, request, 0);
+        nrr = ofpbuf_put_zeros(buf, sizeof *nrr);
+        nrr->role = htonl(rr->role - 1);
+    } else {
+        NOT_REACHED();
     }
-    reply->role = htonl(role);
 
     return buf;
 }
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 8a14f41..955886d 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -508,16 +508,15 @@ struct ofpbuf *ofputil_encode_port_mod(const struct ofputil_port_mod *,
 
 /* Abstract ofp_role_request and reply. */
 struct ofputil_role_request {
-    bool request_current_role_only; /* no role change */
+    enum ofp12_controller_role role;
     bool have_generation_id;
-    enum nx_role role;
     uint64_t generation_id;
 };
 
 enum ofperr ofputil_decode_role_message(const struct ofp_header *,
                                         struct ofputil_role_request *);
 struct ofpbuf *ofputil_encode_role_reply(const struct ofp_header *,
-                                         enum nx_role current_role);
+                                         const struct ofputil_role_request *);
 
 /* Abstract table stats.
  *
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index b94c337..29321d5 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -60,7 +60,7 @@ struct ofconn {
 /* State that should be cleared from one connection to the next. */
 
     /* OpenFlow state. */
-    enum nx_role role;           /* Role. */
+    enum ofp12_controller_role role;           /* Role. */
     enum ofputil_protocol protocol; /* Current protocol variant. */
     enum nx_packet_in_format packet_in_format; /* OFPT_PACKET_IN format. */
 
@@ -777,12 +777,13 @@ static int
 snoop_preference(const struct ofconn *ofconn)
 {
     switch (ofconn->role) {
-    case NX_ROLE_MASTER:
+    case OFPCR12_ROLE_MASTER:
         return 3;
-    case NX_ROLE_OTHER:
+    case OFPCR12_ROLE_EQUAL:
         return 2;
-    case NX_ROLE_SLAVE:
+    case OFPCR12_ROLE_SLAVE:
         return 1;
+    case OFPCR12_ROLE_NOCHANGE:
     default:
         /* Shouldn't happen. */
         return 0;
@@ -844,24 +845,24 @@ ofconn_set_master_election_id(struct ofconn *ofconn, uint64_t id)
 
 /* Returns the role configured for 'ofconn'.
  *
- * The default role, if no other role has been set, is NX_ROLE_OTHER. */
-enum nx_role
+ * The default role, if no other role has been set, is OFPCR12_ROLE_EQUAL. */
+enum ofp12_controller_role
 ofconn_get_role(const struct ofconn *ofconn)
 {
     return ofconn->role;
 }
 
-/* Changes 'ofconn''s role to 'role'.  If 'role' is NX_ROLE_MASTER then any
- * existing master is demoted to a slave. */
+/* Changes 'ofconn''s role to 'role'.  If 'role' is OFPCR12_ROLE_MASTER then
+ * any existing master is demoted to a slave. */
 void
-ofconn_set_role(struct ofconn *ofconn, enum nx_role role)
+ofconn_set_role(struct ofconn *ofconn, enum ofp12_controller_role role)
 {
-    if (role == NX_ROLE_MASTER) {
+    if (role == OFPCR12_ROLE_MASTER) {
         struct ofconn *other;
 
         HMAP_FOR_EACH (other, hmap_node, &ofconn->connmgr->controllers) {
-            if (other->role == NX_ROLE_MASTER) {
-                other->role = NX_ROLE_SLAVE;
+            if (other->role == OFPCR12_ROLE_MASTER) {
+                other->role = OFPCR12_ROLE_SLAVE;
             }
         }
     }
@@ -1092,7 +1093,7 @@ ofconn_flush(struct ofconn *ofconn)
     struct ofmonitor *monitor, *next_monitor;
     int i;
 
-    ofconn->role = NX_ROLE_OTHER;
+    ofconn->role = OFPCR12_ROLE_EQUAL;
     ofconn_set_protocol(ofconn, OFPUTIL_P_NONE);
     ofconn->packet_in_format = NXPIF_OPENFLOW10;
 
@@ -1307,7 +1308,7 @@ ofconn_receives_async_msg(const struct ofconn *ofconn,
         return false;
     }
 
-    async_config = (ofconn->role == NX_ROLE_SLAVE
+    async_config = (ofconn->role == OFPCR12_ROLE_SLAVE
                     ? ofconn->slave_async_config
                     : ofconn->master_async_config);
     if (!(async_config[type] & (1u << reason))) {
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 6ce413e..48b8119 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -96,9 +96,10 @@ void connmgr_get_snoops(const struct connmgr *, struct sset *snoops);
 /* Individual connections to OpenFlow controllers. */
 enum ofconn_type ofconn_get_type(const struct ofconn *);
 
+bool ofconn_get_master_election_id(const struct ofconn *, uint64_t *idp);
 bool ofconn_set_master_election_id(struct ofconn *, uint64_t);
-enum nx_role ofconn_get_role(const struct ofconn *);
-void ofconn_set_role(struct ofconn *, enum nx_role);
+enum ofp12_controller_role ofconn_get_role(const struct ofconn *);
+void ofconn_set_role(struct ofconn *, enum ofp12_controller_role);
 
 enum ofputil_protocol ofconn_get_protocol(const struct ofconn *);
 void ofconn_set_protocol(struct ofconn *, enum ofputil_protocol);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index c0d94f7..856d6fa 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -2238,7 +2238,7 @@ handle_set_config(struct ofconn *ofconn, const struct ofp_header *oh)
     uint16_t flags = ntohs(osc->flags);
 
     if (ofconn_get_type(ofconn) != OFCONN_PRIMARY
-        || ofconn_get_role(ofconn) != NX_ROLE_SLAVE) {
+        || ofconn_get_role(ofconn) != OFPCR12_ROLE_SLAVE) {
         enum ofp_config_flags cur = ofproto->frag_handling;
         enum ofp_config_flags next = flags & OFPC_FRAG_MASK;
 
@@ -2272,7 +2272,7 @@ static enum ofperr
 reject_slave_controller(struct ofconn *ofconn)
 {
     if (ofconn_get_type(ofconn) == OFCONN_PRIMARY
-        && ofconn_get_role(ofconn) == NX_ROLE_SLAVE) {
+        && ofconn_get_role(ofconn) == OFPCR12_ROLE_SLAVE) {
         return OFPERR_OFPBRC_EPERM;
     } else {
         return 0;
@@ -3576,38 +3576,34 @@ handle_flow_mod__(struct ofproto *ofproto, struct ofconn *ofconn,
 static enum ofperr
 handle_role_request(struct ofconn *ofconn, const struct ofp_header *oh)
 {
-    struct ofputil_role_request rr;
+    struct ofputil_role_request request;
+    struct ofputil_role_request reply;
     struct ofpbuf *buf;
-    uint32_t role;
     enum ofperr error;
 
-    error = ofputil_decode_role_message(oh, &rr);
+    error = ofputil_decode_role_message(oh, &request);
     if (error) {
         return error;
     }
 
-    if (rr.request_current_role_only) {
-        role = ofconn_get_role(ofconn); /* NX_ROLE_* */
-        goto reply;
-    }
-
-    role = rr.role;
-
-    if (ofconn_get_role(ofconn) != role
-        && ofconn_has_pending_opgroups(ofconn)) {
-        return OFPROTO_POSTPONE;
-    }
+    if (request.role != OFPCR12_ROLE_NOCHANGE) {
+        if (ofconn_get_role(ofconn) != request.role
+            && ofconn_has_pending_opgroups(ofconn)) {
+            return OFPROTO_POSTPONE;
+        }
 
-    if (rr.have_generation_id) {
-        if (!ofconn_set_master_election_id(ofconn, rr.generation_id)) {
-            return OFPERR_OFPRRFC_STALE;
+        if (request.have_generation_id
+            && !ofconn_set_master_election_id(ofconn, request.generation_id)) {
+                return OFPERR_OFPRRFC_STALE;
         }
-    }
 
-    ofconn_set_role(ofconn, role);
+        ofconn_set_role(ofconn, request.role);
+    }
 
-reply:
-    buf = ofputil_encode_role_reply(oh, role);
+    reply.role = ofconn_get_role(ofconn);
+    reply.have_generation_id = false;
+    reply.generation_id = 0;
+    buf = ofputil_encode_role_reply(oh, &reply);
     ofconn_send_reply(ofconn, buf);
 
     return 0;
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 9e55f65..3a66d1b 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -44,7 +44,7 @@ struct netdev_stats;
 
 struct ofproto_controller_info {
     bool is_connected;
-    enum nx_role role;
+    enum ofp12_controller_role role;
     struct {
         const char *keys[4];
         const char *values[4];
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index fdd7c64..ab0ecd6 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -1961,15 +1961,16 @@ run_system_stats(void)
 }
 
 static inline const char *
-nx_role_to_str(enum nx_role role)
+ofp12_controller_role_to_str(enum ofp12_controller_role role)
 {
     switch (role) {
-    case NX_ROLE_OTHER:
+    case OFPCR12_ROLE_EQUAL:
         return "other";
-    case NX_ROLE_MASTER:
+    case OFPCR12_ROLE_MASTER:
         return "master";
-    case NX_ROLE_SLAVE:
+    case OFPCR12_ROLE_SLAVE:
         return "slave";
+    case OFPCR12_ROLE_NOCHANGE:
     default:
         return "*** INVALID ROLE ***";
     }
@@ -2005,7 +2006,8 @@ refresh_controller_status(void)
             }
 
             ovsrec_controller_set_is_connected(cfg, cinfo->is_connected);
-            ovsrec_controller_set_role(cfg, nx_role_to_str(cinfo->role));
+            ovsrec_controller_set_role(cfg, ofp12_controller_role_to_str(
+                                           cinfo->role));
             ovsrec_controller_set_status(cfg, &smap);
             smap_destroy(&smap);
         } else {
-- 
1.7.10.4




More information about the dev mailing list