[ovs-dev] [netlink v3 2/5] datapath: Make adding and attaching a vport a single step.

Jesse Gross jesse at nicira.com
Thu Dec 2 22:31:17 UTC 2010


On Tue, Nov 16, 2010 at 5:11 PM, Ben Pfaff <blp at nicira.com> wrote:
> -static int do_vport_mod(struct odp_vport_mod *vport_config)
> +static int do_vport_mod(struct odp_port *port)
>  {

There's now only caller of this function (vport_mod) and they're both
pretty small.  Originally the purpose of having two was to separate
out the compat code but that's no long needed.  Does it make sense to
combine them?

> -/**
> - *     vport_user_del - delete existing vport device (for userspace callers)
> - *
> - * @udevname: Name of device to delete
> - *
> - * Deletes the specified device.  Detaches the device from a datapath first
> - * if it is attached.  Deleting the device will fail if it does not exist or it
> - * is the datapath local port.  It is also possible to fail for less obvious
> - * reasons, such as lack of memory.  This function is for userspace callers and
> - * assumes no locks are held.
> - */
> -int vport_user_del(const char __user *udevname)

This function is gone now but the prototype still exists in vport.h.

> diff --git a/lib/dpif.h b/lib/dpif.h
> index c844289..e1bfeee 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -30,7 +30,9 @@ extern "C" {
>  #endif
>
>  struct dpif;
> +struct netdev;
>  struct ofpbuf;
> +struct shash;

It doesn't look like struct shash is actually used here.

> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 39e21bc..3779ada 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> +const struct netdev_class netdev_linux_class =
> +    NETDEV_LINUX_CLASS(
> +        "system",
> +        netdev_linux_create,
> +        netdev_linux_enumerate,
> +        netdev_vport_set_stats);
> +
> +const struct netdev_class netdev_tap_class =
> +    NETDEV_LINUX_CLASS(
> +        "tap",
> +        netdev_linux_create_tap,
> +        NULL,                   /* enumerate */
> +        NULL);                  /* set_stats */
> +
> +const struct netdev_class netdev_internal_class =
> +    NETDEV_LINUX_CLASS(
> +        "internal",
> +        netdev_linux_create,
> +        NULL,                   /* enumerate */
> +        NULL);                  /* set_stats */

Internal devices need a set_stats function, since that's what we're
using for the bonding stats.  It's not clear whether we should expose
it for system devices.  It will work (at least on the kernel datapath)
but I don't think anything should be using it and we don't want to
encourage it.

> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> index 13e897f..6e42470 100644
> --- a/vswitchd/bridge.c
> +++ b/vswitchd/bridge.c
> +                error = netdev_open(&options, &netdev);
> +                shash_destroy(&args);
>
> -            /* If it's not part of the datapath, add it. */
> -            if (!dpif_port) {
> -                error = dpif_port_add(br->dpif, if_name,
> -                                      internal ? ODP_PORT_INTERNAL : 0, NULL);

Isn't the netdev_open() call going to fail for internal devices since
they won't be created until the call to dpif_port_add()?  For vports
we get around this by just storing the config until the call to open
but internal devices check immediately for the device.  Maybe we have
to do something similar to vports?  I do like that it's consistent for
different device types.

Generally speaking though, I'm happy with this.  So whatever you think
on the above comments is fine with me:
Acked-by: Jesse Gross <jesse at nicira.com>




More information about the dev mailing list