netdev: Decouple creating and configuring network devices.
authorBen Pfaff <blp@nicira.com>
Mon, 8 Aug 2011 19:49:17 +0000 (12:49 -0700)
committerBen Pfaff <blp@nicira.com>
Mon, 8 Aug 2011 19:49:17 +0000 (12:49 -0700)
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@nicira.com>
lib/netdev-dummy.c
lib/netdev-linux.c
lib/netdev-provider.h
lib/netdev-vport.c
lib/netdev.c
lib/netdev.h
utilities/ovs-dpctl.c
vswitchd/bridge.c

index 15d97cf..4fb1151 100644 (file)
@@ -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,
index ac511f0..05b830c 100644 (file)
@@ -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,
-                           struct netdev_dev **netdev_devp)
+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,                                         \
index 093a25d..5214be3 100644 (file)
@@ -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. */
index e3e480d..fc20232 100644 (file)
@@ -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
@@ -276,6 +219,43 @@ netdev_vport_close(struct netdev *netdev_)
     free(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)
 {
@@ -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;
-}
 \f
 #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;
index 9954929..ec8ae4f 100644 (file)
@@ -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
index 7e16bd3..bcbd8b0 100644 (file)
@@ -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 *);
index 1c31c71..3b4749c 100644 (file)
@@ -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 {
index 9b70e63..ab8527c 100644 (file)
@@ -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);