netdev: Decouple creating and configuring network devices.

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>

1 files changed, 39 insertions, 23 deletions

diff --git a/utilities/ovs-dpctl.c b/utilities/ovs-dpctl.c
index 1c31c7106..3b4749c7f 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 {