[ovs-discuss] [PATCH 00/20] Implement abstract interfaces to network devices

Jesse Gross jesse at nicira.com
Tue Jul 28 00:09:06 UTC 2009


Ben Pfaff wrote:
> This series of commits first refactors the network device layer to be
> more amenable to portability, then it (in a big commit toward the end)
> actually abstracts the network device implementation into a class
> structure.  This should make it easier to port netdev to other
> systems, or to support more than one kind of network device on a
> single system.
I looked through this little bundle of patches and it seemed generally 
good.  It doesn't look like we'll have any problems adding a Unix domain 
socket netdev, so it seems like a good abstraction.  Here are a couple 
of things that I noticed:

In ofproto.c/ofproto_create(), it seems weird to be calling 
pick_fallback_dpid() twice - once for p->fallback_dpid and once for 
p->datapath_id since this will generate two different random ID's.  
p->datapath_id gets overwritten later on with the final ID, so it seems 
like it is either redundant to generate it earlier (if no one uses it 
between those to calls) or potentially bad (if someone uses datapath_id 
and assumes that it is the final value).

In bridge.c, I'm not sure that open_iface_netdev is the best name for 
the function if it also retrieves other information like carrier status.

The name lxnetdev isn't overly descriptive of the actual functionality.  
I'm not sure what's better though.

The fact that an initialization failure in one of the classes of netdevs 
blocks all netdevs seems a little fragile to me.

Two comments about comments in netdev-provider.c/netdev_class:
 * prefix says that the empty string matches class names that wouldn't 
otherwise match.  In reality, the empty string only matches names 
without prefixes.
 * get_in6 refers to setting in4

I believe that netdev-linux.c/netdev_tap_class needs to have a prefix of 
"tap"

Having both a Linux netdev and a tap netdev will cause the 
initialization to happen twice, overwriting some of the static variables 
(mainly af_inet_sock).

Do we really want to reinitialize all of the netdev classes every time 
someone opens a netdev?

Some of the netdevs may want to use the config file, so it would be 
useful to have a reconfigure call in the netdev interface.




More information about the discuss mailing list