connmgr: Improve interface for setting controllers.

Using an shash instead of an array simplifies the code for both the caller
and the callee.  Putting the set of allowed OpenFlow versions into the
ofproto_controller data structure also simplifies the overall function
interface slightly.

Tested-by: Yifeng Sun <pkusunyifeng@gmail.com>
Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>

4 files changed, 28 insertions, 42 deletions

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index c7532cf01..f5576d505 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -580,9 +580,7 @@ connmgr_free_controller_info(struct shash *info)
 /* Changes 'mgr''s set of controllers to the 'n_controllers' controllers in
  * 'controllers'. */
 void
-connmgr_set_controllers(struct connmgr *mgr,
-                        const struct ofproto_controller *controllers,
-                        size_t n_controllers, uint32_t allowed_versions)
+connmgr_set_controllers(struct connmgr *mgr, struct shash *controllers)
     OVS_EXCLUDED(ofproto_mutex)
 {
     bool had_controllers = connmgr_has_controllers(mgr);
@@ -591,52 +589,49 @@ connmgr_set_controllers(struct connmgr *mgr,
      * cover a smaller amount of code, if that yielded some benefit. */
     ovs_mutex_lock(&ofproto_mutex);
 
-    /* Create newly configured controllers and services.
-     * Create a name to ofproto_controller mapping in 'new_controllers'. */
-    struct shash new_controllers = SHASH_INITIALIZER(&new_controllers);
-    for (size_t i = 0; i < n_controllers; i++) {
-        const struct ofproto_controller *c = &controllers[i];
+    /* Create newly configured controllers and services. */
+    struct shash_node *node;
+    SHASH_FOR_EACH (node, controllers) {
+        const char *target = node->name;
+        const struct ofproto_controller *c = node->data;
 
-        if (!vconn_verify_name(c->target)) {
+        if (!vconn_verify_name(target)) {
             bool add = false;
-            struct ofconn *ofconn = find_controller_by_target(mgr, c->target);
+            struct ofconn *ofconn = find_controller_by_target(mgr, target);
             if (!ofconn) {
                 VLOG_INFO("%s: added primary controller \"%s\"",
-                          mgr->name, c->target);
+                          mgr->name, target);
                 add = true;
             } else if (rconn_get_allowed_versions(ofconn->rconn) !=
-                       allowed_versions) {
+                       c->allowed_versions) {
                 VLOG_INFO("%s: re-added primary controller \"%s\"",
-                          mgr->name, c->target);
+                          mgr->name, target);
                 add = true;
                 ofconn_destroy(ofconn);
             }
             if (add) {
-                add_controller(mgr, c->target, c->dscp, allowed_versions);
+                add_controller(mgr, target, c->dscp, c->allowed_versions);
             }
-        } else if (!pvconn_verify_name(c->target)) {
+        } else if (!pvconn_verify_name(target)) {
             bool add = false;
-            struct ofservice *ofservice = ofservice_lookup(mgr, c->target);
+            struct ofservice *ofservice = ofservice_lookup(mgr, target);
             if (!ofservice) {
                 VLOG_INFO("%s: added service controller \"%s\"",
-                          mgr->name, c->target);
+                          mgr->name, target);
                 add = true;
-            } else if (ofservice->allowed_versions != allowed_versions) {
+            } else if (ofservice->allowed_versions != c->allowed_versions) {
                 VLOG_INFO("%s: re-added service controller \"%s\"",
-                          mgr->name, c->target);
+                          mgr->name, target);
                 ofservice_destroy(mgr, ofservice);
                 add = true;
             }
             if (add) {
-                ofservice_create(mgr, c->target, allowed_versions, c->dscp);
+                ofservice_create(mgr, target, c->allowed_versions, c->dscp);
             }
         } else {
             VLOG_WARN_RL(&rl, "%s: unsupported controller \"%s\"",
-                         mgr->name, c->target);
-            continue;
+                         mgr->name, target);
         }
-
-        shash_add_once(&new_controllers, c->target, &controllers[i]);
     }
 
     /* Delete controllers that are no longer configured.
@@ -644,8 +639,7 @@ connmgr_set_controllers(struct connmgr *mgr,
     struct ofconn *ofconn, *next_ofconn;
     HMAP_FOR_EACH_SAFE (ofconn, next_ofconn, hmap_node, &mgr->controllers) {
         const char *target = ofconn_get_target(ofconn);
-        struct ofproto_controller *c = shash_find_data(&new_controllers,
-                                                       target);
+        struct ofproto_controller *c = shash_find_data(controllers, target);
         if (!c) {
             VLOG_INFO("%s: removed primary controller \"%s\"",
                       mgr->name, target);
@@ -660,8 +654,7 @@ connmgr_set_controllers(struct connmgr *mgr,
     struct ofservice *ofservice, *next_ofservice;
     HMAP_FOR_EACH_SAFE (ofservice, next_ofservice, node, &mgr->services) {
         const char *target = pvconn_get_name(ofservice->pvconn);
-        struct ofproto_controller *c = shash_find_data(&new_controllers,
-                                                       target);
+        struct ofproto_controller *c = shash_find_data(controllers, target);
         if (!c) {
             VLOG_INFO("%s: removed service controller \"%s\"",
                       mgr->name, target);
@@ -671,8 +664,6 @@ connmgr_set_controllers(struct connmgr *mgr,
         }
     }
 
-    shash_destroy(&new_controllers);
-
     ovs_mutex_unlock(&ofproto_mutex);
 
     update_in_band_remotes(mgr);
diff --git a/ofproto/connmgr.h b/ofproto/connmgr.h
index 4a22f1c26..11c8f9a85 100644
--- a/ofproto/connmgr.h
+++ b/ofproto/connmgr.h
@@ -56,6 +56,7 @@ enum ofconn_type {
     OFCONN_PRIMARY,             /* An ordinary OpenFlow controller. */
     OFCONN_SERVICE              /* A service connection, e.g. "ovs-ofctl". */
 };
+const char *ofconn_type_to_string(enum ofconn_type);
 
 /* An asynchronous message that might need to be queued between threads. */
 struct ofproto_async_msg {
@@ -94,9 +95,7 @@ void connmgr_retry(struct connmgr *);
 bool connmgr_has_controllers(const struct connmgr *);
 void connmgr_get_controller_info(struct connmgr *, struct shash *);
 void connmgr_free_controller_info(struct shash *);
-void connmgr_set_controllers(struct connmgr *,
-                             const struct ofproto_controller[], size_t n,
-                             uint32_t allowed_versions);
+void connmgr_set_controllers(struct connmgr *, struct shash *);
 void connmgr_reconnect(const struct connmgr *);
 
 int connmgr_set_snoops(struct connmgr *, const struct sset *snoops);
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 8b2e3ca97..222c74994 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -639,12 +639,9 @@ ofproto_set_datapath_id(struct ofproto *p, uint64_t datapath_id)
 }
 
 void
-ofproto_set_controllers(struct ofproto *p,
-                        const struct ofproto_controller *controllers,
-                        size_t n_controllers, uint32_t allowed_versions)
+ofproto_set_controllers(struct ofproto *p, struct shash *controllers)
 {
-    connmgr_set_controllers(p->connmgr, controllers, n_controllers,
-                            allowed_versions);
+    connmgr_set_controllers(p->connmgr, controllers);
 }
 
 void
diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
index 3ca88baf0..595729dd6 100644
--- a/ofproto/ofproto.h
+++ b/ofproto/ofproto.h
@@ -208,11 +208,12 @@ enum ofproto_band {
 };
 
 struct ofproto_controller {
-    char *target;               /* e.g. "tcp:127.0.0.1" */
     int max_backoff;            /* Maximum reconnection backoff, in seconds. */
     int probe_interval;         /* Max idle time before probing, in seconds. */
     enum ofproto_band band;     /* In-band or out-of-band? */
     bool enable_async_msgs;     /* Initially enable asynchronous messages? */
+    uint32_t allowed_versions;  /* OpenFlow protocol versions that may
+                                 * be negotiated for a session. */
 
     /* OpenFlow packet-in rate-limiting. */
     int rate_limit;             /* Max packet-in rate in packets per second. */
@@ -304,9 +305,7 @@ int ofproto_port_query_by_name(const struct ofproto *, const char *devname,
 /* Top-level configuration. */
 uint64_t ofproto_get_datapath_id(const struct ofproto *);
 void ofproto_set_datapath_id(struct ofproto *, uint64_t datapath_id);
-void ofproto_set_controllers(struct ofproto *,
-                             const struct ofproto_controller *, size_t n,
-                             uint32_t allowed_versions);
+void ofproto_set_controllers(struct ofproto *, struct shash *controllers);
 void ofproto_set_fail_mode(struct ofproto *, enum ofproto_fail_mode fail_mode);
 void ofproto_reconnect_controllers(struct ofproto *);
 void ofproto_set_extra_in_band_remotes(struct ofproto *,