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

Ben Pfaff blp at nicira.com
Wed Apr 14 17:20:57 UTC 2010


On Wed, Apr 14, 2010 at 12:31:25AM -0700, Justin Pettit wrote:
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 736b588..4d4bf90 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -177,7 +178,7 @@ netdev_dev_linux_cast(const struct netdev_dev *netdev_dev)
>  {
>      const char *type = netdev_dev_get_type(netdev_dev);
>      assert(!strcmp(type, "system") || !strcmp(type, "tap")
> -            || !strcmp(type, "gre"));
> +            || !strcmp(type, "gre") || !strcmp(type, "patch"));
>      return CONTAINER_OF(netdev_dev, struct netdev_dev_linux, netdev_dev);
>  }
>  
> @@ -186,7 +187,7 @@ netdev_linux_cast(const struct netdev *netdev)
>  {
>      const char *type = netdev_get_type(netdev);
>      assert(!strcmp(type, "system") || !strcmp(type, "tap")
> -            || !strcmp(type, "gre"));
> +            || !strcmp(type, "gre") || !strcmp(type, "patch"));
>      return CONTAINER_OF(netdev, struct netdev_linux, netdev);
>  }

I realize that you didn't introduce the above, but it still seems like a
silly amount of overhead in the assertions.  Why not something like this:
        assert(netdev_get_dev(netdev)->netdev_class->init == netdev_linux_init);
Much cheaper than up to 4 string compares.

Did you consider rate-limiting the log messages in create_patch()?

I think you could use shash_find_data(args, "peer") to simplify
setup_patch().

setup_patch() has a label "error:" but I don't see anything that jumps
to it.

netdev_linux_create_patch() could avoid a free() by postponing the
xzalloc() until after setup_patch() succeeds or fails.

In a few places, the code assumes that fprintf() returns a negative
errno value, but it doesn't: the error code must still be obtained from
errno on failure.

It might be a cleanup to factor out writing a string to
/sys/class/net/veth_pairs into a new function, since those 15 or so
lines appear twice.

> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index d3f3efb..873c145 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -357,6 +357,9 @@
>            <dd>A TUN/TAP device managed by Open vSwitch.</dd>
>            <dt><code>gre</code></dt>
>            <dd>A GRE tunnel device managed by Open vSwitch.</dd>
> +          <dt><code>patch</code></dt>
> +          <dd>A pair of virtual devices that act as patch cable managed by 
> +            Open vSwitch.</dd>
>          </dl>
>        </column>

The XML file should also document the "peer" option for patch devices.
(I think that it doesn't document the options for "gre" devices, but in
my opinion that's a bug.)




More information about the dev mailing list