[ovs-dev] [PATCH 2/2] netdev: Allow explicit creation of netdev objects

[Ben Pfaff]

Thanks, Justin!  I'm glad to see that this worked out OK.

Some comments inline below:

Justin Pettit <jpettit at nicira.com> writes:

> @@ -194,9 +209,74 @@ netdev_linux_cache_cb(const struct rtnetlink_change *change,
>      }
>  }
 
> +/* Creates the netdev object of 'type' with 'name'. */
> +static int
> +netdev_linux_create(const char *name, const char *type, 
> +                    struct shash *args UNUSED)
> +{
> +    struct netdev_obj_linux *netdev_obj;

Would it be a good idea to log a message if 'args' is nonempty,
since those arguments will be ignored?

> +/* Destroys the netdev object 'netdev_obj_'. */
>  static int
> -netdev_linux_open(const char *name, char *suffix, int ethertype,
> -                  struct netdev **netdevp)
> +netdev_linux_destroy(struct netdev_obj *netdev_obj_)
> +{
> +    struct netdev_obj_linux *netdev_obj = netdev_obj_linux_cast(netdev_obj_);
> +
> +    if (!netdev_obj) {
> +        return 0;
> +    }

The "if" test needs to precede the call to
netdev_obj_linux_cast(), since the call will segfault if its
argument is null (without NDEBUG).

> +    close(netdev_obj->tap_fd);

close(-1) won't do any harm but I'd suggest avoiding it anyhow.

> +    free(netdev_obj);
> +
> +    return 0;

Hmm, usually a "destroy" or "close" call doesn't have a useful
return value.  Should "destroy" functions be "void" or do you
foresee a use for an error value?

> +    /* Attempts to create a network device object of 'type' with 'name'.  
> +     * 'type' corresponds to the 'type' field used in the netdev_class
> +     * structure.  Arguments for creation are provided in 'args', which
> +     * may be empty or NULL if none are needed. */
> +    int (*create)(const char *name, const char *type, struct shash *args);

It would be nice if the wrapper function for this function
normalized 'args' by ensuring that it is never null, supplying an
empty shash of its own in that case (or by ensuring that it is
always null if 'args' is empty, I guess).

Is the 'create' function allowed to modify 'args'?  If not then I
would mark it const; if so then I would state that in the
description.

> +
> +    /* Destroys 'netdev_obj'.  Netdev objects maintain a reference count
> +     * which is incremented on netdev_open() and decremented on
> +     * netdev_close().  If 'netdev_obj' has a non-zero reference count,
> +     * it will not destroy the object and return EBUSY. */
> +    int (*destroy)(struct netdev_obj *netdev_obj);

It looks to me like the "destroy" function will never get called
unless the reference count has fallen to zero.  If that's
correct, then I would update the description here to say so, to
make the interface clear to netdev class implementors.

> +
> +    /* Reconfigures the device object 'netdev_obj' with 'args'.  'args'
> +     * may be empty or NULL if none are needed. */
> +    int (*reconfigure)(struct netdev_obj *netdev_obj, struct shash *args);

Again I'd consider normalizing args, one way or another here.

It looks like "reconfigure" is allowed to be null.  I would
mention that in the description.

> +int
> +netdev_destroy(const char *name)
> +{
> +    struct shash_node *node;
> +    struct netdev_obj *netdev_obj;
> +
> +    node = shash_find(&netdev_obj_shash, name);
> +    if (!node) {
> +        return ENODEV;
> +    }
> +
> +    netdev_obj = node->data;

You could save a couple of lines here by using shash_find_data()
instead.

Looking at netdev_open() and netdev_close(), I think there's an
oddity in the netdev_obj refcounting.  Refcounts are maintained
when and only when a netdev_obj exists.  But for "system"
netdevs, can't a netdev_obj be created after the netdev is
already opened, and won't that allow the refcount to get off?
e.g.

        netdev_open("eth0")     // no netdev_obj, no refcount bump
        netdev_create("eth0")   // new netdev_obj, refcount == 0
        netdev_close(eth0)      // logs an error since refcount == 0

> +    netdev_obj = shash_find_data(&netdev_obj_shash, name);
> +    if (netdev_obj) {
> +        error = netdev_obj->class->open(name, ethertype, &netdev);
> +        if (!error) {
> +            netdev_obj->ref_cnt++;
> +        }
> +        goto exit;
> +    } else {
> +        /* Default to "system". */
> +        for (i = 0; i < n_netdev_classes; i++) {
> +            const struct netdev_class *class = netdev_classes[i];
> +            if (!strcmp(class->type, "system")) {
> +                error = class->open(name, ethertype, &netdev);
> +                goto exit;
> +            }
>          }
>      }
> +
>      error = EAFNOSUPPORT;
> 
> exit:

I think that you could remove the "exit:" label and the "goto"s
by moving "error = EAFNOSUPPORT;" just above the "for" loop and
changing the "goto exit;" inside the loop to a "break".

> +/* Initializes 'netdev_obj' as a netdev object named 'name' of the 
> + * specified 'class'.
> + *
> + * This function adds 'netdev_obj' to a netdev-owned shash, so it is
> + * very important that 'netdev_obj' only be freed after calling
> + * netdev_destroy().  */
> +void
> +netdev_obj_init(struct netdev_obj *netdev_obj, const char *name,
> +                const struct netdev_class *class)
> +{
> +    netdev_obj->class = class;
> +    netdev_obj->ref_cnt = 0;
> +    shash_add(&netdev_obj_shash, name, netdev_obj);
> +}
> +

Is it worth asserting that !shash_find(&netdev_obj_shash, name)?
Presumably that shouldn't ever happen since netdev_create()
checks, but...

> @@ -720,9 +822,18 @@ netdev_init(struct netdev *netdev, const char *name,
>      netdev->name = xstrdup(name);
>      netdev->save_flags = 0;
>      netdev->changed_flags = 0;
> +    netdev->netdev_obj = shash_find_data(&netdev_obj_shash, name);
>      list_push_back(&netdev_list, &netdev->node);
>  }
 
Does anything ever use this ->netdev_obj pointer?  I don't see
any users.  Can it be used reliably?  (What about the
netdev_open("eth0"), netdev_create("eth0") path that I mentioned
above?)

> +        if (value) {
> +            shash_add(&args, arg, strdup(value));
> +        }

Suggest xstrdup() instead.