[ovs-dev] [PATCH 4/4] netdev: Add support for "patch" type

Justin Pettit jpettit at nicira.com
Thu Apr 15 08:49:07 UTC 2010


Yeah, I wasn't very happy with this initial implementation.  I was rushing out a working but far from perfect version so that I could get this feature to those who were blocked on it.  I've now sent out a reworked implementation that I think is closer to how it should be done.  One of the big improvements is that it won't destroy the veth devices unless there are no netdev devices referring to them.

Please take a look at the new version and let me know what you think.

Thanks!

--Justin


On Apr 14, 2010, at 10:55 AM, Jesse Gross wrote:

> I have some a couple additional comments:
> 
> On Wed, Apr 14, 2010 at 3:31 AM, Justin Pettit <jpettit at nicira.com> wrote:
> +static int
> +create_patch(const char *name, const char *peer)
> +{
> +    FILE *veth_file;
> +    int retval;
> +
> +    /* Assume if both sides exist that they're part of the same patch */
> +    if (netdev_exists(name) && netdev_exists(peer)) {
> +        return 0;
> +    }
> 
> This makes me a little bit nervous.  netdev_exists() is implemented by calling netdev_open() and checking to see if it succeeds.  In this case since the device has not yet been created it will default to using the system netdev.  However, had you moved the netdev_dev_init a up few lines it would have caused an infinite loop.  I guess it works but it didn't seem obviously safe to me.
>  
> +static void
> +destroy_patch(const char *name)
> +{
> +    FILE *veth_file;
> +    int retval;
> +
> +    if (!netdev_exists(name)) {
> +        VLOG_WARN("patch device %s does not exist", name);
> +        return;
> +    }
> 
> This one is definitely not good.  First of all it seems like it will produce spurious error messages: since you delete both devices the first time around won't it always trigger when you delete the second?  More importantly, you are effectively calling open from the close function, which is just asking for trouble.





More information about the dev mailing list