[ovs-dev] [netdevs 3/4] netdev: Decouple creating and configuring network devices.
Ethan Jackson
ethan at nicira.com
Mon Aug 8 19:03:29 UTC 2011
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 <blp at nicira.com> 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 <paul at nicira.com>
> ---
> 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
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
More information about the dev
mailing list