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

Jesse Gross jesse at nicira.com
Wed Apr 14 17:55:45 UTC 2010


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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20100414/b7e5b495/attachment-0003.html>


More information about the dev mailing list