[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