Couple of minor comments: The indentation is messed up at the definition of netdev_linux_create().
netdev_vport_set_config() always writes the configuration to the datapath if the given netdev's options are NULL. I would think that it would be common for the datapath to already have the exact same configuration installed from a previous run of vswitchd. Would it make sense to automatically run netdev_vport_get_config() at creation time? Ethan On Fri, Aug 5, 2011 at 14:42, Ben Pfaff <[email protected]> wrote: > Until now, each call to netdev_open() for a particular network device > had to either specify a set of network device arguments that was either > empty or (for devices that already existed) equal to the existing device's > configuration. Unfortunately, the definition of "equality" in the latter > case was mostly done in terms of strict equality of string-to-string maps, > which caused problems in cases where, for example, one set of arguments > specified the default value of an optional argument explicitly and the > other omitted it. > > The netdev interface does have provisions for defining equality other ways, > but this had only been done in one case that was especially problematic in > practice. One way to solve this particular problem would be to carefully > define equality in all the problematic cases. > > This commit takes another approach based on the realization that there is > really no need to do any comparisons. Instead, it removes configuration > at netdev_open() time entirely, because almost all of netdev_open()'s > callers are not interested in creating and configuring a netdev. Most of > them just want to open a configured device and use it. Therefore, this > commit stops providing any configuration arguments to netdev_open() and the > provider functions that it calls. Instead, a caller that does want to > configure a device does so after it opens it, by calling > netdev_set_config(). > > This change allows us to simplify the netdev interface a bit. There is no > longer any need to implement argument comparisons. As a result, there is > also no need for "struct netdev_dev" to keep track of configuration at all. > Instead, the network devices that have configuration keep track of it in > their own internal form. > > This new interface does mean that it becomes possible to accidentally > create and try to use an unconfigured netdev that requires configuration. > > Bug #6677. > Reported-by: Paul Ingram <[email protected]> > --- > lib/netdev-dummy.c | 7 +- > lib/netdev-linux.c | 21 ++----- > lib/netdev-provider.h | 32 +++------ > lib/netdev-vport.c | 181 > +++++++++++++++---------------------------------- > lib/netdev.c | 95 +++++++++----------------- > lib/netdev.h | 3 +- > utilities/ovs-dpctl.c | 62 +++++++++++------ > vswitchd/bridge.c | 40 +++++++---- > 8 files changed, 171 insertions(+), 270 deletions(-) > > diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c > index 15d97cf..4fb1151 100644 > --- a/lib/netdev-dummy.c > +++ b/lib/netdev-dummy.c > @@ -42,7 +42,7 @@ struct netdev_dummy { > }; > > static int netdev_dummy_create(const struct netdev_class *, const char *, > - const struct shash *, struct netdev_dev **); > + struct netdev_dev **); > static void netdev_dummy_poll_notify(const struct netdev *); > > static bool > @@ -68,14 +68,13 @@ netdev_dummy_cast(const struct netdev *netdev) > > static int > netdev_dummy_create(const struct netdev_class *class, const char *name, > - const struct shash *args, > struct netdev_dev **netdev_devp) > { > static unsigned int n = 0xaa550000; > struct netdev_dev_dummy *netdev_dev; > > netdev_dev = xzalloc(sizeof *netdev_dev); > - netdev_dev_init(&netdev_dev->netdev_dev, name, args, class); > + netdev_dev_init(&netdev_dev->netdev_dev, name, class); > netdev_dev->hwaddr[0] = 0xaa; > netdev_dev->hwaddr[1] = 0x55; > netdev_dev->hwaddr[2] = n >> 24; > @@ -241,8 +240,8 @@ static const struct netdev_class dummy_class = { > > netdev_dummy_create, > netdev_dummy_destroy, > + NULL, /* get_config */ > NULL, /* set_config */ > - NULL, /* config_equal */ > > netdev_dummy_open, > netdev_dummy_close, > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index ac511f0..7ca5a3d 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -516,18 +516,12 @@ netdev_linux_cache_cb(const struct > rtnetlink_link_change *change, > > /* Creates system and internal devices. */ > static int > -netdev_linux_create(const struct netdev_class *class, > - const char *name, const struct shash *args, > +netdev_linux_create(const struct netdev_class *class, const char *name, > struct netdev_dev **netdev_devp) > { > struct netdev_dev_linux *netdev_dev; > int error; > > - if (!shash_is_empty(args)) { > - VLOG_WARN("%s: arguments for %s devices should be empty", > - name, class->type); > - } > - > if (!cache_notifier_refcount) { > error = rtnetlink_link_notifier_register(&netdev_linux_cache_notifier, > netdev_linux_cache_cb, NULL); > @@ -539,7 +533,7 @@ netdev_linux_create(const struct netdev_class *class, > > netdev_dev = xzalloc(sizeof *netdev_dev); > netdev_dev->change_seq = 1; > - netdev_dev_init(&netdev_dev->netdev_dev, name, args, class); > + netdev_dev_init(&netdev_dev->netdev_dev, name, class); > > *netdev_devp = &netdev_dev->netdev_dev; > return 0; > @@ -553,8 +547,7 @@ netdev_linux_create(const struct netdev_class *class, > * be unavailable to other reads for tap devices. */ > static int > netdev_linux_create_tap(const struct netdev_class *class OVS_UNUSED, > - const char *name, const struct shash *args, > - struct netdev_dev **netdev_devp) > + const char *name, struct netdev_dev **netdev_devp) > { > struct netdev_dev_linux *netdev_dev; > struct tap_state *state; > @@ -562,10 +555,6 @@ netdev_linux_create_tap(const struct netdev_class *class > OVS_UNUSED, > struct ifreq ifr; > int error; > > - if (!shash_is_empty(args)) { > - VLOG_WARN("%s: arguments for TAP devices should be empty", name); > - } > - > netdev_dev = xzalloc(sizeof *netdev_dev); > state = &netdev_dev->state.tap; > > @@ -593,7 +582,7 @@ netdev_linux_create_tap(const struct netdev_class *class > OVS_UNUSED, > goto error; > } > > - netdev_dev_init(&netdev_dev->netdev_dev, name, args, &netdev_tap_class); > + netdev_dev_init(&netdev_dev->netdev_dev, name, &netdev_tap_class); > *netdev_devp = &netdev_dev->netdev_dev; > return 0; > > @@ -2252,8 +2241,8 @@ netdev_linux_change_seq(const struct netdev *netdev) > \ > CREATE, \ > netdev_linux_destroy, \ > + NULL, /* get_config */ \ > NULL, /* set_config */ \ > - NULL, /* config_equal */ \ > \ > netdev_linux_open, \ > netdev_linux_close, \ > diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h > index 093a25d..5214be3 100644 > --- a/lib/netdev-provider.h > +++ b/lib/netdev-provider.h > @@ -39,11 +39,9 @@ struct netdev_dev { > this device. */ > int ref_cnt; /* Times this devices was opened. */ > struct shash_node *node; /* Pointer to element in global map. > */ > - struct shash args; /* Argument list from last config. */ > }; > > void netdev_dev_init(struct netdev_dev *, const char *name, > - const struct shash *args, > const struct netdev_class *); > void netdev_dev_uninit(struct netdev_dev *, bool destroy); > const char *netdev_dev_get_type(const struct netdev_dev *); > @@ -52,8 +50,6 @@ const char *netdev_dev_get_name(const struct netdev_dev *); > struct netdev_dev *netdev_dev_from_name(const char *name); > void netdev_dev_get_devices(const struct netdev_class *, > struct shash *device_list); > -bool netdev_dev_args_equal(const struct netdev_dev *netdev_dev, > - const struct shash *args); > > static inline void netdev_dev_assert_class(const struct netdev_dev > *netdev_dev, > const struct netdev_class *class_) > @@ -116,11 +112,10 @@ struct netdev_class { > * needed here. */ > void (*wait)(void); > > - /* Attempts to create a network device named 'name' with initial 'args' > in > - * 'netdev_class'. On success sets 'netdev_devp' to the newly created > - * device. */ > + /* Attempts to create a network device named 'name' in 'netdev_class'. > On > + * success sets 'netdev_devp' to the newly created device. */ > int (*create)(const struct netdev_class *netdev_class, const char *name, > - const struct shash *args, struct netdev_dev **netdev_devp); > + struct netdev_dev **netdev_devp); > > /* Destroys 'netdev_dev'. > * > @@ -130,21 +125,18 @@ struct netdev_class { > * called. */ > void (*destroy)(struct netdev_dev *netdev_dev); > > - /* Changes the device 'netdev_dev''s configuration to 'args'. > + /* Fetches the device 'netdev_dev''s configuration, storing it in 'args'. > + * The caller owns 'args' and pre-initializes it to an empty shash. > * > - * If this netdev class does not support reconfiguring a netdev > - * device, this may be a null pointer. > - */ > - int (*set_config)(struct netdev_dev *netdev_dev, const struct shash > *args); > + * If this netdev class does not have any configuration options, this may > + * be a null pointer. */ > + int (*get_config)(struct netdev_dev *netdev_dev, struct shash *args); > > - /* Returns true if 'args' is equivalent to the "args" field in > - * 'netdev_dev', otherwise false. > + /* Changes the device 'netdev_dev''s configuration to 'args'. > * > - * If no special processing needs to be done beyond a simple > - * shash comparison, this may be a null pointer. > - */ > - bool (*config_equal)(const struct netdev_dev *netdev_dev, > - const struct shash *args); > + * If this netdev class does not support configuration, this may be a > null > + * pointer. */ > + int (*set_config)(struct netdev_dev *netdev_dev, const struct shash > *args); > > /* Attempts to open a network device. On success, sets 'netdevp' > * to the new network device. */ > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > index e3e480d..fc20232 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -68,13 +68,12 @@ struct vport_class { > int (*unparse_config)(const char *name, const char *type, > const struct nlattr *options, size_t options_len, > struct shash *args); > - bool (*config_equal)(const struct shash *nd_args, const struct shash > *args); > }; > > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); > > static int netdev_vport_create(const struct netdev_class *, const char *, > - const struct shash *, struct netdev_dev **); > + struct netdev_dev **); > static void netdev_vport_poll_notify(const struct netdev *); > static int tnl_port_config_from_nlattr(const struct nlattr *options, > size_t options_len, > @@ -175,77 +174,21 @@ netdev_vport_get_netdev_type(const struct > dpif_linux_vport *vport) > > static int > netdev_vport_create(const struct netdev_class *netdev_class, const char > *name, > - const struct shash *args, > struct netdev_dev **netdev_devp) > { > - const struct vport_class *vport_class = vport_class_cast(netdev_class); > - struct ofpbuf *options = NULL; > - struct shash fetched_args; > - int dp_ifindex; > - uint32_t port_no; > - int error; > - > - shash_init(&fetched_args); > - > - dp_ifindex = -1; > - port_no = UINT32_MAX; > - if (!shash_is_empty(args)) { > - /* Parse the provided configuration. */ > - options = ofpbuf_new(64); > - error = vport_class->parse_config(name, netdev_class->type, > - args, options); > - } else { > - /* Fetch an existing configuration from the kernel. > - * > - * This case could be ambiguous with initializing a new vport with an > - * empty configuration, but none of the existing vport classes accept > - * an empty configuration. */ > - struct dpif_linux_vport reply; > - struct ofpbuf *buf; > - > - error = dpif_linux_vport_get(name, &reply, &buf); > - if (!error) { > - /* XXX verify correct type */ > - error = vport_class->unparse_config(name, netdev_class->type, > - reply.options, > - reply.options_len, > - &fetched_args); > - if (error) { > - VLOG_ERR_RL(&rl, "%s: failed to parse kernel config (%s)", > - name, strerror(error)); > - } else { > - options = ofpbuf_clone_data(reply.options, > reply.options_len); > - dp_ifindex = reply.dp_ifindex; > - port_no = reply.port_no; > - } > - ofpbuf_delete(buf); > - } else { > - VLOG_ERR_RL(&rl, "%s: vport query failed (%s)", > - name, strerror(error)); > - } > - } > + struct netdev_dev_vport *dev; > > - if (!error) { > - struct netdev_dev_vport *dev; > - > - dev = xmalloc(sizeof *dev); > - netdev_dev_init(&dev->netdev_dev, name, > - shash_is_empty(&fetched_args) ? args : &fetched_args, > - netdev_class); > - dev->options = options; > - dev->dp_ifindex = dp_ifindex; > - dev->port_no = port_no; > - dev->change_seq = 1; > - > - *netdev_devp = &dev->netdev_dev; > - route_table_register(); > - } else { > - ofpbuf_delete(options); > - } > + dev = xmalloc(sizeof *dev); > + netdev_dev_init(&dev->netdev_dev, name, netdev_class); > + dev->options = NULL; > + dev->dp_ifindex = -1; > + dev->port_no = UINT32_MAX; > + dev->change_seq = 1; > > - shash_destroy(&fetched_args); > + *netdev_devp = &dev->netdev_dev; > + route_table_register(); > > - return error; > + return 0; > } > > static void > @@ -277,6 +220,43 @@ netdev_vport_close(struct netdev *netdev_) > } > > static int > +netdev_vport_get_config(struct netdev_dev *dev_, struct shash *args) > +{ > + const struct netdev_class *netdev_class = netdev_dev_get_class(dev_); > + const struct vport_class *vport_class = vport_class_cast(netdev_class); > + struct netdev_dev_vport *dev = netdev_dev_vport_cast(dev_); > + const char *name = netdev_dev_get_name(dev_); > + int error; > + > + if (!dev->options) { > + struct dpif_linux_vport reply; > + struct ofpbuf *buf; > + > + error = dpif_linux_vport_get(name, &reply, &buf); > + if (error) { > + VLOG_ERR_RL(&rl, "%s: vport query failed (%s)", > + name, strerror(error)); > + return error; > + } > + > + dev->options = ofpbuf_clone_data(reply.options, reply.options_len); > + dev->dp_ifindex = reply.dp_ifindex; > + dev->port_no = reply.port_no; > + ofpbuf_delete(buf); > + } > + > + error = vport_class->unparse_config(name, netdev_class->type, > + dev->options->data, > + dev->options->size, > + args); > + if (error) { > + VLOG_ERR_RL(&rl, "%s: failed to parse kernel config (%s)", > + name, strerror(error)); > + } > + return error; > +} > + > +static int > netdev_vport_set_config(struct netdev_dev *dev_, const struct shash *args) > { > const struct netdev_class *netdev_class = netdev_dev_get_class(dev_); > @@ -290,7 +270,8 @@ netdev_vport_set_config(struct netdev_dev *dev_, const > struct shash *args) > error = vport_class->parse_config(name, netdev_dev_get_type(dev_), > args, options); > if (!error > - && (options->size != dev->options->size > + && (!dev->options > + || options->size != dev->options->size > || memcmp(options->data, dev->options->data, options->size))) { > struct dpif_linux_vport vport; > > @@ -315,20 +296,6 @@ netdev_vport_set_config(struct netdev_dev *dev_, const > struct shash *args) > return error; > } > > -static bool > -netdev_vport_config_equal(const struct netdev_dev *dev_, > - const struct shash *args) > -{ > - const struct netdev_class *netdev_class = netdev_dev_get_class(dev_); > - const struct vport_class *vport_class = vport_class_cast(netdev_class); > - > - if (vport_class->config_equal) { > - return vport_class->config_equal(&dev_->args, args); > - } else { > - return smap_equal(&dev_->args, args); > - } > -} > - > static int > netdev_vport_send(struct netdev *netdev, const void *data, size_t size) > { > @@ -882,44 +849,6 @@ unparse_patch_config(const char *name OVS_UNUSED, const > char *type OVS_UNUSED, > smap_add(args, "peer", nl_attr_get_string(a[ODP_PATCH_ATTR_PEER])); > return 0; > } > - > -/* Returns true if 'nd_args' is equivalent to 'args', otherwise false. > - * Typically, 'nd_args' is the result of a call to unparse_tunnel_config() > - * and 'args' is the original definition of the port. > - * > - * IPsec key configuration is handled by an external program, so it is not > - * pushed down into the kernel module. Thus, when the "unparse_config" > - * method is called on an existing IPsec-based vport, a simple > - * comparison with the returned data will not match the original > - * configuration. This function ignores configuration about keys when > - * doing a comparison. > - */ > -static bool > -config_equal_ipsec(const struct shash *nd_args, const struct shash *args) > -{ > - struct shash tmp1, tmp2; > - bool result; > - > - smap_clone(&tmp1, nd_args); > - smap_clone(&tmp2, args); > - > - shash_find_and_delete(&tmp1, "psk"); > - shash_find_and_delete(&tmp2, "psk"); > - shash_find_and_delete(&tmp1, "peer_cert"); > - shash_find_and_delete(&tmp2, "peer_cert"); > - shash_find_and_delete(&tmp1, "certificate"); > - shash_find_and_delete(&tmp2, "certificate"); > - shash_find_and_delete(&tmp1, "private_key"); > - shash_find_and_delete(&tmp2, "private_key"); > - shash_find_and_delete(&tmp1, "use_ssl_cert"); > - shash_find_and_delete(&tmp2, "use_ssl_cert"); > - > - result = smap_equal(&tmp1, &tmp2); > - smap_destroy(&tmp1); > - smap_destroy(&tmp2); > - > - return result; > -} > > #define VPORT_FUNCTIONS(GET_STATUS) \ > NULL, \ > @@ -928,8 +857,8 @@ config_equal_ipsec(const struct shash *nd_args, const > struct shash *args) > \ > netdev_vport_create, \ > netdev_vport_destroy, \ > + netdev_vport_get_config, \ > netdev_vport_set_config, \ > - netdev_vport_config_equal, \ > \ > netdev_vport_open, \ > netdev_vport_close, \ > @@ -987,19 +916,19 @@ netdev_vport_register(void) > static const struct vport_class vport_classes[] = { > { ODP_VPORT_TYPE_GRE, > { "gre", VPORT_FUNCTIONS(netdev_vport_get_status) }, > - parse_tunnel_config, unparse_tunnel_config, NULL }, > + parse_tunnel_config, unparse_tunnel_config }, > > { ODP_VPORT_TYPE_GRE, > { "ipsec_gre", VPORT_FUNCTIONS(netdev_vport_get_status) }, > - parse_tunnel_config, unparse_tunnel_config, config_equal_ipsec }, > + parse_tunnel_config, unparse_tunnel_config }, > > { ODP_VPORT_TYPE_CAPWAP, > { "capwap", VPORT_FUNCTIONS(netdev_vport_get_status) }, > - parse_tunnel_config, unparse_tunnel_config, NULL }, > + parse_tunnel_config, unparse_tunnel_config }, > > { ODP_VPORT_TYPE_PATCH, > { "patch", VPORT_FUNCTIONS(NULL) }, > - parse_patch_config, unparse_patch_config, NULL } > + parse_patch_config, unparse_patch_config } > }; > > int i; > diff --git a/lib/netdev.c b/lib/netdev.c > index 9954929..ec8ae4f 100644 > --- a/lib/netdev.c > +++ b/lib/netdev.c > @@ -192,34 +192,21 @@ netdev_enumerate_types(struct sset *types) > } > } > > -void > -update_device_args(struct netdev_dev *dev, const struct shash *args) > -{ > - smap_destroy(&dev->args); > - smap_clone(&dev->args, args); > -} > - > /* Opens the network device named 'name' (e.g. "eth0") and returns zero if > * successful, otherwise a positive errno value. On success, sets '*netdevp' > * to the new network device, otherwise to null. > * > - * If this is the first time the device has been opened, then create is > called > - * before opening. The device is created using the given type and > - * arguments. */ > + * Some network devices may need to be configured (with netdev_set_config()) > + * before they can be used. */ > int > netdev_open(struct netdev_options *options, struct netdev **netdevp) > { > - struct shash empty_args = SHASH_INITIALIZER(&empty_args); > struct netdev_dev *netdev_dev; > int error; > > *netdevp = NULL; > netdev_initialize(); > > - if (!options->args) { > - options->args = &empty_args; > - } > - > netdev_dev = shash_find_data(&netdev_dev_shash, options->name); > > if (!netdev_dev) { > @@ -231,19 +218,12 @@ netdev_open(struct netdev_options *options, struct > netdev **netdevp) > options->name, options->type); > return EAFNOSUPPORT; > } > - error = class->create(class, options->name, options->args, > - &netdev_dev); > + error = class->create(class, options->name, &netdev_dev); > if (error) { > return error; > } > assert(netdev_dev->netdev_class == class); > > - } else if (!shash_is_empty(options->args) && > - !netdev_dev_args_equal(netdev_dev, options->args)) { > - > - VLOG_WARN("%s: attempted to open already open netdev with " > - "different arguments", options->name); > - return EINVAL; > } > > error = netdev_dev->netdev_class->open(netdev_dev, netdevp); > @@ -275,36 +255,44 @@ netdev_open_default(const char *name, struct netdev > **netdevp) > int > netdev_set_config(struct netdev *netdev, const struct shash *args) > { > - struct shash empty_args = SHASH_INITIALIZER(&empty_args); > struct netdev_dev *netdev_dev = netdev_get_dev(netdev); > > - if (!args) { > - args = &empty_args; > - } > - > if (netdev_dev->netdev_class->set_config) { > - if (!netdev_dev_args_equal(netdev_dev, args)) { > - update_device_args(netdev_dev, args); > - return netdev_dev->netdev_class->set_config(netdev_dev, args); > - } > - } else if (!shash_is_empty(args)) { > - VLOG_WARN("%s: arguments provided to device whose configuration " > - "cannot be changed", netdev_get_name(netdev)); > + struct shash no_args = SHASH_INITIALIZER(&no_args); > + return netdev_dev->netdev_class->set_config(netdev_dev, > + args ? args : &no_args); > + } else if (args && !shash_is_empty(args)) { > + VLOG_WARN("%s: arguments provided to device that is not > configurable", > + netdev_get_name(netdev)); > } > > return 0; > } > > -/* Returns the current configuration for 'netdev'. This is either the > - * configuration passed to netdev_open() or netdev_set_config(), or it is a > - * configuration retrieved from the device itself if no configuration was > - * passed to those functions. > +/* Returns the current configuration for 'netdev' in 'args'. The caller must > + * have already initialized 'args' with shash_init(). Returns 0 on success, > in > + * which case 'args' will be filled with 'netdev''s configuration. On > failure > + * returns a positive errno value, in which case 'args' will be empty. > * > - * 'netdev' retains ownership of the returned configuration. */ > -const struct shash * > -netdev_get_config(const struct netdev *netdev) > + * The caller owns 'args' and its contents and must eventually free them with > + * shash_destroy_free_data(). */ > +int > +netdev_get_config(const struct netdev *netdev, struct shash *args) > { > - return &netdev_get_dev(netdev)->args; > + struct netdev_dev *netdev_dev = netdev_get_dev(netdev); > + int error; > + > + shash_clear_free_data(args); > + if (netdev_dev->netdev_class->get_config) { > + error = netdev_dev->netdev_class->get_config(netdev_dev, args); > + if (error) { > + shash_clear_free_data(args); > + } > + } else { > + error = 0; > + } > + > + return error; > } > > /* Closes and destroys 'netdev'. */ > @@ -1295,17 +1283,11 @@ exit: > * 'netdev_class'. This function is ordinarily called from a netdev > provider's > * 'create' function. > * > - * 'args' should be the arguments that were passed to the netdev provider's > - * 'create'. If an empty set of arguments was passed, and 'name' is the name > - * of a network device that existed before the 'create' call, then 'args' may > - * instead be the configuration for that existing device. > - * > * This function adds 'netdev_dev' to a netdev-owned shash, so it is > * very important that 'netdev_dev' only be freed after calling > * the refcount drops to zero. */ > void > netdev_dev_init(struct netdev_dev *netdev_dev, const char *name, > - const struct shash *args, > const struct netdev_class *netdev_class) > { > assert(!shash_find(&netdev_dev_shash, name)); > @@ -1314,7 +1296,6 @@ netdev_dev_init(struct netdev_dev *netdev_dev, const > char *name, > netdev_dev->netdev_class = netdev_class; > netdev_dev->name = xstrdup(name); > netdev_dev->node = shash_add(&netdev_dev_shash, name, netdev_dev); > - smap_clone(&netdev_dev->args, args); > } > > /* Undoes the results of initialization. > @@ -1332,7 +1313,6 @@ netdev_dev_uninit(struct netdev_dev *netdev_dev, bool > destroy) > assert(!netdev_dev->ref_cnt); > > shash_delete(&netdev_dev_shash, netdev_dev->node); > - smap_destroy(&netdev_dev->args); > > if (destroy) { > netdev_dev->netdev_class->destroy(netdev_dev); > @@ -1392,19 +1372,6 @@ netdev_dev_get_devices(const struct netdev_class > *netdev_class, > } > } > > -/* Returns true if 'args' is equivalent to the "args" field in > - * 'netdev_dev', otherwise false. */ > -bool > -netdev_dev_args_equal(const struct netdev_dev *netdev_dev, > - const struct shash *args) > -{ > - if (netdev_dev->netdev_class->config_equal) { > - return netdev_dev->netdev_class->config_equal(netdev_dev, args); > - } else { > - return smap_equal(&netdev_dev->args, args); > - } > -} > - > /* Initializes 'netdev' as a instance of the netdev_dev. > * > * This function adds 'netdev' to a netdev-owned linked list, so it is very > diff --git a/lib/netdev.h b/lib/netdev.h > index 7e16bd3..bcbd8b0 100644 > --- a/lib/netdev.h > +++ b/lib/netdev.h > @@ -78,7 +78,6 @@ struct netdev_stats { > struct netdev_options { > const char *name; > const char *type; > - const struct shash *args; > }; > > struct netdev; > @@ -101,7 +100,7 @@ int netdev_enumerate(struct sset *); > > /* Options. */ > int netdev_set_config(struct netdev *, const struct shash *args); > -const struct shash *netdev_get_config(const struct netdev *); > +int netdev_get_config(const struct netdev *, struct shash *); > > /* Basic properties. */ > const char *netdev_get_name(const struct netdev *); > diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c > index 1c31c71..3b4749c 100644 > --- a/utilities/ovs-dpctl.c > +++ b/utilities/ovs-dpctl.c > @@ -224,14 +224,13 @@ do_add_if(int argc OVS_UNUSED, char *argv[]) > for (i = 2; i < argc; i++) { > char *save_ptr = NULL; > struct netdev_options options; > - struct netdev *netdev; > + struct netdev *netdev = NULL; > struct shash args; > char *option; > int error; > > options.name = strtok_r(argv[i], ",", &save_ptr); > options.type = "system"; > - options.args = &args; > > if (!options.name) { > ovs_error(0, "%s is not a valid network device name", argv[i]); > @@ -260,16 +259,26 @@ do_add_if(int argc OVS_UNUSED, char *argv[]) > if (error) { > ovs_error(error, "%s: failed to open network device", > options.name); > - } else { > - error = dpif_port_add(dpif, netdev, NULL); > - if (error) { > - ovs_error(error, "adding %s to %s failed", > - options.name, argv[1]); > - } else { > - error = if_up(options.name); > - } > - netdev_close(netdev); > + goto next; > + } > + > + error = netdev_set_config(netdev, &args); > + if (error) { > + ovs_error(error, "%s: failed to configure network device", > + options.name); > + goto next; > } > + > + error = dpif_port_add(dpif, netdev, NULL); > + if (error) { > + ovs_error(error, "adding %s to %s failed", options.name, > argv[1]); > + goto next; > + } > + > + error = if_up(options.name); > + > +next: > + netdev_close(netdev); > if (error) { > failure = true; > } > @@ -382,21 +391,28 @@ show_dpif(struct dpif *dpif) > > netdev_options.name = dpif_port.name; > netdev_options.type = dpif_port.type; > - netdev_options.args = NULL; > error = netdev_open(&netdev_options, &netdev); > if (!error) { > - const struct shash_node **nodes; > - const struct shash *config; > - size_t i; > - > - config = netdev_get_config(netdev); > - nodes = shash_sort(config); > - for (i = 0; i < shash_count(config); i++) { > - const struct shash_node *node = nodes[i]; > - printf("%c %s=%s", i ? ',' : ':', > - node->name, (char *) node->data); > + struct shash config; > + > + shash_init(&config); > + error = netdev_get_config(netdev, &config); > + if (!error) { > + const struct shash_node **nodes; > + size_t i; > + > + nodes = shash_sort(&config); > + for (i = 0; i < shash_count(&config); i++) { > + const struct shash_node *node = nodes[i]; > + printf("%c %s=%s", i ? ',' : ':', > + node->name, (char *) node->data); > + } > + free(nodes); > + } else { > + printf(", could not retrieve configuration (%s)", > + strerror(error)); > } > - free(nodes); > + shash_destroy_free_data(&config); > > netdev_close(netdev); > } else { > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index f43902b..f417859 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -846,28 +846,39 @@ bridge_add_ofproto_ports(struct bridge *br) > struct ofproto_port ofproto_port; > > LIST_FOR_EACH_SAFE (iface, next_iface, port_elem, &port->ifaces) { > - struct shash args; > int error; > > - /* Open the netdev or reconfigure it. */ > - shash_init(&args); > - shash_from_ovs_idl_map(iface->cfg->key_options, > - iface->cfg->value_options, > - iface->cfg->n_options, &args); > + /* Open the netdev. */ > if (!iface->netdev) { > struct netdev_options options; > options.name = iface->name; > options.type = iface->type; > - options.args = &args; > error = netdev_open(&options, &iface->netdev); > + if (error) { > + VLOG_WARN("could not open network device %s (%s)", > + iface->name, strerror(error)); > + } > } else { > - error = netdev_set_config(iface->netdev, &args); > + error = 0; > } > - shash_destroy(&args); > - if (error) { > - VLOG_WARN("could not %s network device %s (%s)", > - iface->netdev ? "reconfigure" : "open", > - iface->name, strerror(error)); > + > + /* Configure the netdev. */ > + if (iface->netdev) { > + struct shash args; > + > + shash_init(&args); > + shash_from_ovs_idl_map(iface->cfg->key_options, > + iface->cfg->value_options, > + iface->cfg->n_options, &args); > + error = netdev_set_config(iface->netdev, &args); > + shash_destroy(&args); > + > + if (error) { > + VLOG_WARN("could not configure network device %s (%s)", > + iface->name, strerror(error)); > + netdev_close(iface->netdev); > + iface->netdev = NULL; > + } > } > > /* Add the port, if necessary. */ > @@ -891,7 +902,7 @@ bridge_add_ofproto_ports(struct bridge *br) > iface_refresh_status(iface); > } > > - /* Delete the iface if */ > + /* Delete the iface if we failed. */ > if (iface->netdev && iface->ofp_port >= 0) { > VLOG_DBG("bridge %s: interface %s is on port %d", > br->name, iface->name, iface->ofp_port); > @@ -923,7 +934,6 @@ bridge_add_ofproto_ports(struct bridge *br) > > options.name = port->name; > options.type = "internal"; > - options.args = NULL; > error = netdev_open(&options, &netdev); > if (!error) { > ofproto_port_add(br->ofproto, netdev, NULL); > -- > 1.7.4.4 > > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
