From 43776b8fa5df28adfe08e7bdc8bffdac252b1e01 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 27 Sep 2010 13:15:19 -0700 Subject: [PATCH] vswitchd: Break set_up_iface() into two different functions. set_up_iface() had two only loosely related purposes. It's cleaner to use two separate functions. --- vswitchd/bridge.c | 142 +++++++++++++++++++++++++++------------------- 1 file changed, 83 insertions(+), 59 deletions(-) diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index cce4515f..5210da16 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -256,6 +256,9 @@ static bool iface_is_internal(const struct bridge *, const char *name); static void iface_set_mac(struct iface *); static void iface_update_qos(struct iface *, const struct ovsrec_qos *); +static void shash_from_ovs_idl_map(char **keys, char **values, size_t n, + struct shash *); + /* Hooks into ofproto processing. */ static struct ofhooks bridge_ofhooks; @@ -359,73 +362,89 @@ bridge_configure_once(const struct ovsrec_open_vswitch *cfg) svec_destroy(&dpif_types); } -/* Attempt to create the network device 'iface_name' through the netdev +/* Initializes 'options' and fills it with the options for 'if_cfg'. Merges + * keys from "options" and "other_config", preferring "options" keys over + * "other_config" keys. + * + * The value strings in '*options' are taken directly from if_cfg, not copied, + * so the caller should not modify or free them. */ +static void +iface_get_options(const struct ovsrec_interface *if_cfg, struct shash *options) +{ + size_t i; + + shash_from_ovs_idl_map(if_cfg->key_options, if_cfg->value_options, + if_cfg->n_options, options); + + for (i = 0; i < if_cfg->n_other_config; i++) { + char *key = if_cfg->key_other_config[i]; + char *value = if_cfg->value_other_config[i]; + + if (!shash_find_data(options, key)) { + shash_add(options, key, value); + } else { + VLOG_WARN("%s: ignoring \"other_config\" key %s that conflicts " + "with \"options\" key %s", if_cfg->name, key, key); + } + } +} + +/* Attempt to create the network device for 'iface' through the netdev * library. */ static int -set_up_iface(struct iface *iface, bool create) +create_iface_netdev(struct iface *iface) { + struct netdev_options netdev_options; struct shash options; - int error = 0; - size_t i; + int error; - shash_init(&options); - for (i = 0; i < iface->cfg->n_options; i++) { - shash_add(&options, iface->cfg->key_options[i], - xstrdup(iface->cfg->value_options[i])); - } - - /* Include 'other_config' keys in hash of netdev options. The - * namespace of 'other_config' and 'options' must be disjoint. - * Prefer 'options' keys over 'other_config' keys. */ - for (i = 0; i < iface->cfg->n_other_config; i++) { - char *value = xstrdup(iface->cfg->value_other_config[i]); - if (!shash_add_once(&options, iface->cfg->key_other_config[i], - value)) { - VLOG_WARN("%s: \"other_config\" key %s conflicts with existing " - "\"other_config\" or \"options\" entry...ignoring", - iface->cfg->name, iface->cfg->key_other_config[i]); - free(value); - } + memset(&netdev_options, 0, sizeof netdev_options); + netdev_options.name = iface->cfg->name; + if (!strcmp(iface->cfg->type, "internal")) { + /* An "internal" config type maps to a netdev "system" type. */ + netdev_options.type = "system"; + } else { + netdev_options.type = iface->cfg->type; } + netdev_options.args = &options; + netdev_options.ethertype = NETDEV_ETH_TYPE_NONE; - if (create) { - struct netdev_options netdev_options; + iface_get_options(iface->cfg, &options); - memset(&netdev_options, 0, sizeof netdev_options); - netdev_options.name = iface->cfg->name; - if (!strcmp(iface->cfg->type, "internal")) { - /* An "internal" config type maps to a netdev "system" type. */ - netdev_options.type = "system"; - } else { - netdev_options.type = iface->cfg->type; - } - netdev_options.args = &options; - netdev_options.ethertype = NETDEV_ETH_TYPE_NONE; + error = netdev_open(&netdev_options, &iface->netdev); - error = netdev_open(&netdev_options, &iface->netdev); + if (iface->netdev) { + netdev_get_carrier(iface->netdev, &iface->enabled); + } - if (iface->netdev) { - netdev_get_carrier(iface->netdev, &iface->enabled); - } - } else if (iface->netdev) { - const char *netdev_type = netdev_get_type(iface->netdev); - const char *iface_type = iface->cfg->type && strlen(iface->cfg->type) - ? iface->cfg->type : NULL; + shash_destroy(&options); - /* An "internal" config type maps to a netdev "system" type. */ - if (iface_type && !strcmp(iface_type, "internal")) { - iface_type = "system"; - } + return error; +} - if (!iface_type || !strcmp(netdev_type, iface_type)) { - error = netdev_reconfigure(iface->netdev, &options); - } else { - VLOG_WARN("%s: attempting change device type from %s to %s", - iface->cfg->name, netdev_type, iface_type); - error = EINVAL; - } +static int +reconfigure_iface_netdev(struct iface *iface) +{ + const char *netdev_type, *iface_type; + struct shash options; + int error; + + /* Skip reconfiguration if the device has the wrong type. This shouldn't + * happen, but... */ + iface_type = (!iface->cfg->type[0] ? NULL + : !strcmp(iface->cfg->type, "internal") ? "system" + : iface->cfg->type); + netdev_type = netdev_get_type(iface->netdev); + if (iface_type && strcmp(netdev_type, iface_type)) { + VLOG_WARN("%s: attempting change device type from %s to %s", + iface->cfg->name, netdev_type, iface_type); + return EINVAL; } - shash_destroy_free_data(&options); + + /* Reconfigure device. */ + iface_get_options(iface->cfg, &options); + error = netdev_reconfigure(iface->netdev, &options); + shash_destroy(&options); return error; } @@ -435,7 +454,7 @@ check_iface_netdev(struct bridge *br OVS_UNUSED, struct iface *iface, void *aux OVS_UNUSED) { if (!iface->netdev) { - int error = set_up_iface(iface, true); + int error = create_iface_netdev(iface); if (error) { VLOG_WARN("could not open netdev on %s, dropping: %s", iface->name, strerror(error)); @@ -659,9 +678,10 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg) struct iface *iface = node->data; if (shash_find(&cur_ifaces, if_name)) { - /* Already exists, just reconfigure it. */ - if (iface) { - set_up_iface(iface, false); + /* Already exists on the datapath. If we have it open, + * reconfigure it; otherwise we'll open it later. */ + if (iface && iface->netdev) { + reconfigure_iface_netdev(iface); } } else { /* Need to add to datapath. */ @@ -3688,7 +3708,7 @@ iface_create(struct port *port, const struct ovsrec_interface *if_cfg) /* Attempt to create the network interface in case it doesn't exist yet. */ if (!iface_is_internal(br, iface->name)) { - error = set_up_iface(iface, true); + error = create_iface_netdev(iface); if (error) { VLOG_WARN("could not create iface %s: %s", iface->name, strerror(error)); @@ -3825,6 +3845,10 @@ iface_set_mac(struct iface *iface) } } +/* Adds the 'n' key-value pairs in 'keys' in 'values' to 'shash'. + * + * The value strings in '*shash' are taken directly from values[], not copied, + * so the caller should not modify or free them. */ static void shash_from_ovs_idl_map(char **keys, char **values, size_t n, struct shash *shash) -- 2.30.2