| Message ID | 20181029225751.5936-3-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,1/6] connmgr: Modernize coding style. | expand |
Thanks for the improvement. Tested-by: Yifeng Sun <pkusunyifeng@gmail.com> Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com> On Mon, Oct 29, 2018 at 3:59 PM Ben Pfaff <blp@ovn.org> wrote: > 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@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 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
Thanks, I applied this to master. On Wed, Oct 31, 2018 at 02:49:28PM -0700, Yifeng Sun wrote: > Thanks for the improvement. > > Tested-by: Yifeng Sun <pkusunyifeng@gmail.com> > Reviewed-by: Yifeng Sun <pkusunyifeng@gmail.com> > > On Mon, Oct 29, 2018 at 3:59 PM Ben Pfaff <blp@ovn.org> wrote: > > > 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@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 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
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
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@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(-)