[ovs-dev] [PATCH 3/6] connmgr: Improve interface for setting controllers.

Ben Pfaff blp at ovn.org
Mon Oct 29 22:57:48 UTC 2018


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.

Signed-off-by: Ben Pfaff <blp at ovn.org>
---
 ofproto/connmgr.c |  51 +++++++++++----------------
 ofproto/connmgr.h |   5 ++-
 ofproto/ofproto.c |   7 ++--
 ofproto/ofproto.h |   7 ++--
 vswitchd/bridge.c | 102 ++++++++++++++++++++++--------------------------------
 5 files changed, 69 insertions(+), 103 deletions(-)

diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index c7532cf01217..f5576d50524d 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 4a22f1c26611..11c8f9a85121 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 8b2e3ca97a2b..222c749940ec 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 3ca88baf018f..595729dd61f1 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 *,
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 9d230b20641c..83708ee51c7a 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -3462,48 +3462,6 @@ bridge_del_ports(struct bridge *br, const struct shash *wanted_ports)
     }
 }
 
-/* Initializes 'oc' appropriately as a management service controller for
- * 'br'.
- *
- * The caller must free oc->target when it is no longer needed. */
-static void
-bridge_ofproto_controller_for_mgmt(const struct bridge *br,
-                                   struct ofproto_controller *oc)
-{
-    oc->target = xasprintf("punix:%s/%s.mgmt", ovs_rundir(), br->name);
-    oc->max_backoff = 0;
-    oc->probe_interval = 60;
-    oc->band = OFPROTO_OUT_OF_BAND;
-    oc->rate_limit = 0;
-    oc->burst_limit = 0;
-    oc->enable_async_msgs = true;
-    oc->dscp = 0;
-}
-
-/* Converts ovsrec_controller 'c' into an ofproto_controller in 'oc'.  */
-static void
-bridge_ofproto_controller_from_ovsrec(const struct ovsrec_controller *c,
-                                      struct ofproto_controller *oc)
-{
-    int dscp;
-
-    oc->target = c->target;
-    oc->max_backoff = c->max_backoff ? *c->max_backoff / 1000 : 8;
-    oc->probe_interval = c->inactivity_probe ? *c->inactivity_probe / 1000 : 5;
-    oc->band = (!c->connection_mode || !strcmp(c->connection_mode, "in-band")
-                ? OFPROTO_IN_BAND : OFPROTO_OUT_OF_BAND);
-    oc->rate_limit = c->controller_rate_limit ? *c->controller_rate_limit : 0;
-    oc->burst_limit = (c->controller_burst_limit
-                       ? *c->controller_burst_limit : 0);
-    oc->enable_async_msgs = (!c->enable_async_messages
-                             || *c->enable_async_messages);
-    dscp = smap_get_int(&c->other_config, "dscp", DSCP_DEFAULT);
-    if (dscp < 0 || dscp > 63) {
-        dscp = DSCP_DEFAULT;
-    }
-    oc->dscp = dscp;
-}
-
 /* Configures the IP stack for 'br''s local interface properly according to the
  * configuration in 'c'.  */
 static void
@@ -3589,10 +3547,6 @@ bridge_configure_remotes(struct bridge *br,
 
     enum ofproto_fail_mode fail_mode;
 
-    struct ofproto_controller *ocs;
-    size_t n_ocs;
-    size_t i;
-
     /* Check if we should disable in-band control on this bridge. */
     disable_in_band = smap_get_bool(&br->cfg->other_config, "disable-in-band",
                                     false);
@@ -3611,13 +3565,22 @@ bridge_configure_remotes(struct bridge *br,
     n_controllers = (ofproto_get_flow_restore_wait() ? 0
                      : bridge_get_controllers(br, &controllers));
 
-    ocs = xmalloc((n_controllers + 1) * sizeof *ocs);
-    n_ocs = 0;
-
-    bridge_ofproto_controller_for_mgmt(br, &ocs[n_ocs++]);
-    for (i = 0; i < n_controllers; i++) {
+    /* The set of controllers to pass down to ofproto. */
+    struct shash ocs = SHASH_INITIALIZER(&ocs);
+
+    /* Add managment controller. */
+    struct ofproto_controller *oc = xmalloc(sizeof *oc);
+    *oc = (struct ofproto_controller) {
+        .probe_interval = 60,
+        .band = OFPROTO_OUT_OF_BAND,
+        .enable_async_msgs = true,
+        .allowed_versions = bridge_get_allowed_versions(br),
+    };
+    shash_add_nocopy(
+        &ocs, xasprintf("punix:%s/%s.mgmt", ovs_rundir(), br->name), oc);
+
+    for (size_t i = 0; i < n_controllers; i++) {
         struct ovsrec_controller *c = controllers[i];
-
         if (daemon_should_self_confine()
             && (!strncmp(c->target, "punix:", 6)
             || !strncmp(c->target, "unix:", 5))) {
@@ -3668,17 +3631,34 @@ bridge_configure_remotes(struct bridge *br,
         }
 
         bridge_configure_local_iface_netdev(br, c);
-        bridge_ofproto_controller_from_ovsrec(c, &ocs[n_ocs]);
-        if (disable_in_band) {
-            ocs[n_ocs].band = OFPROTO_OUT_OF_BAND;
+
+        int dscp = smap_get_int(&c->other_config, "dscp", DSCP_DEFAULT);
+        if (dscp < 0 || dscp > 63) {
+            dscp = DSCP_DEFAULT;
         }
-        n_ocs++;
-    }
 
-    ofproto_set_controllers(br->ofproto, ocs, n_ocs,
-                            bridge_get_allowed_versions(br));
-    free(ocs[0].target); /* From bridge_ofproto_controller_for_mgmt(). */
-    free(ocs);
+        oc = xmalloc(sizeof *oc);
+        *oc = (struct ofproto_controller) {
+            .max_backoff = c->max_backoff ? *c->max_backoff / 1000 : 8,
+            .probe_interval = (c->inactivity_probe
+                               ? *c->inactivity_probe / 1000 : 5),
+            .band = ((!c->connection_mode
+                      || !strcmp(c->connection_mode, "in-band"))
+                     && !disable_in_band
+                     ? OFPROTO_IN_BAND : OFPROTO_OUT_OF_BAND),
+            .enable_async_msgs = (!c->enable_async_messages
+                                  || *c->enable_async_messages),
+            .allowed_versions = bridge_get_allowed_versions(br),
+            .rate_limit = (c->controller_rate_limit
+                           ? *c->controller_rate_limit : 0),
+            .burst_limit = (c->controller_burst_limit
+                            ? *c->controller_burst_limit : 0),
+            .dscp = dscp,
+        };
+        shash_add(&ocs, c->target, oc);
+    }
+    ofproto_set_controllers(br->ofproto, &ocs);
+    shash_destroy_free_data(&ocs);
 
     /* Set the fail-mode. */
     fail_mode = !br->cfg->fail_mode
-- 
2.16.1



More information about the dev mailing list