[ovs-dev] [PATCH 1/3] netdev: Fully handle netdev lifecycle through refcounting.

[Jesse Gross]

Thanks for the review.  I made the changes in response to your comments, 
with a few responses below.  I pushed the set last week plus a few 
things today that I had previous run out of time for.

Ben Pfaff wrote:

> lib/netdev.c:
> 
> I'm also a little nervous about assuming that if the hash is the same
> then there's no reason to reconfigure at all:
> 
>>     if (netdev_dev->class->reconfigure) {
>>         uint32_t args_hash = shash_hash(args);
>>
>>         if (netdev_dev->args_hash != args_hash) {
>>             netdev_dev->args_hash = args_hash;

I changed this to directly store and compare the arguments instead of 
using a hash to avoid any potential problems.

> Also, if 'netdev_dev->class->reconfigure' returns an error code, we
> still update the hash.  I'm not sure whether that's correct or not.
> 
>>             return netdev_dev->class->reconfigure(netdev_dev, args);
>>         }
>>     }

I believe that this is correct.  Not updating it would just cause it to 
reconfigure every time with the same result.  If something might change 
in the future it is better handled at the netdev provider level, 
triggering on whatever would cause the change.

> It's a slightly unusual style to initialize a refcount to 0, as in
> netdev_dev_init(), and then increment it later to 1.

I don't really think this is that unusual, especially since in this 
context it reduces the number of code paths, such as with devices that 
haven't been fully initialized yet.

> netdev_dev_uninit() and netdev_uninit() are exported in case a netdev
> class needs to initialize and then uninitialize a netdev(_dev) without
> letting the netdev library get a hold on it in the meantime.  But do you
> foresee a situation where that would be necessary?  I think that
> normally a class could simply call netdev(_dev)_init as its very last
> step, so that this could not come up.

It's actually being used by the linux netdev open function.  Most of the 
time it is possible to just make the init the last step and in these 
cases that is the easiest solution.  However, the linux netdev makes 
some calls that require a netdev (i.e. calling back into the netdev 
library for the flags functions).  Since this happens before the netdev 
is fully initialized, it needs to be possible to uninitialize it.

> ----------------------------------------------------------------------
> 
> ofproto.c:
> 
> Why did the init_ports() call move?  Why is it now called once per
> program execution instead of once per ofproto?  (A vswitch might have
> lots of ofprotos.)

Thanks for catching this - init_ports() should be called once per 
ofproto, not once per program execution.  However, the reason why it 
moved is because the netdevs need to be setup by the bridge before the 
call to init_ports().  The ofproto is created when the bridge is created 
but the netdevs are not created until the bridge is configured.