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

[Justin Pettit]

Thanks for the review.  I've integrated most of your suggestions and sent out another patch for review.  Unfortunately, I think the netdev library will probably take another couple iterations before we're happy with it.  This patch at least fixes some of the pain-points we're experiencing right now.

Inline, I've responded to items that needed a bit more of a response than "Good point.  Done."

On Dec 1, 2009, at 10:31 AM, Ben Pfaff wrote:

>> +    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?

I made the "destroy" provider function be "void", but I left the netdev_destroy() as is, since it needs to be able to indicate that the call failed if the refcount wasn't zero.

>> +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.

I don't think I can, since a couple lines later, I want to delete the node, and I need a handle.

> 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

I've modified the netdev_open() call to create an ephemeral netdev_obj of type "system" when one wasn't created through a netdev_create() call.  Now, if a user attempts to call netdev_create() after a netdev_open(), it will fail, because the name already exists.  When all users have netdev_closed()'d all netdevs that reference an ephemeral netdev_obj, that netdev_obj will be automatically destroyed.

Thanks again!

--Justin