[ovs-dev] [netlink v2 3/4] netdev-vport: Merge in netdev-patch and netdev-tunnel.

[Jesse Gross]

On Fri, Sep 24, 2010 at 5:15 PM, Ben Pfaff <blp at nicira.com> wrote:
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 088de42..90eb741 100644
> +static int
> +netdev_vport_create(const struct netdev_class *class, const char *name,
> +                    const struct shash *args OVS_UNUSED,
> +                    struct netdev_dev **netdev_devp)
> +{
> +    int err;
> +    struct odp_vport_add ova;
> +    struct netdev_dev_vport *netdev_dev;
> +
> +    ovs_strlcpy(ova.port_type, class->type, sizeof ova.port_type);
> +    ovs_strlcpy(ova.devname, name, sizeof ova.devname);
> +    err = netdev_vport_parse_config(class, name, args, &ova.config);
> +    if (err) {
> +        return err;
> +    }
> +
> +    err = netdev_vport_do_ioctl(ODP_VPORT_ADD, &ova);
> +    free(ova.config);
> +
> +    if (err == EBUSY) {
> +        VLOG_WARN("%s: destroying existing device", name);
> +
> +        err = netdev_vport_do_ioctl(ODP_VPORT_DEL, ova.devname);
> +        if (err) {
> +            return err;
> +        }
> +
> +        err = netdev_vport_do_ioctl(ODP_VPORT_ADD, &ova);
> +    }

If we have to retry we're going to be in trouble because the config
was already freed.

> +static int
> +netdev_vport_reconfigure(struct netdev_dev *netdev_dev,
> +                         const struct shash *args)
> +{
> +    const char *name = netdev_dev_get_name(netdev_dev);
> +    struct odp_vport_mod ovm;
> +    void *config;
> +    int err;
> +
> +    ovs_strlcpy(ovm.devname, name, sizeof ovm.devname);
> +    err = netdev_vport_parse_config(netdev_dev_get_class(netdev_dev), name,
> +                                    args, &config);
> +    if (err) {
> +        return err;
> +    }
> +
> +    err = netdev_vport_do_ioctl(ODP_VPORT_MOD, &ovm);
> +    free(config);

config isn't attached to anything here and doesn't actually get passed
to the kernel.  I think you want ovm.config.

> diff --git a/lib/netdev.c b/lib/netdev.c
> index df9c690..ed2fe9e 100644
>  static void
>  netdev_initialize(void)
>  {
> -    static int status = -1;
> +    static bool inited;
>
> -    if (status < 0) {
> -        int i;
> +    if (!inited) {
> +        inited = true;
>
>         fatal_signal_add_hook(close_all_netdevs, NULL, NULL, true);
>
> -        status = 0;
> -        for (i = 0; i < ARRAY_SIZE(base_netdev_classes); i++) {
> -            netdev_register_provider(base_netdev_classes[i]);
> -        }
> +#ifdef HAVE_NETLINK
> +        netdev_register_provider(&netdev_linux_class);
> +        netdev_register_provider(&netdev_tap_class);
> +        netdev_vport_register();
> +#endif

I'm curious about the move to make all of the types dynamically
registered.  I guess the goal is to keep the vport types self
contained but I'm not sure there's that much benefit to it, maybe the
amount of code is slightly less.  I don't have strong feelings about
it either way though.

>  int
>  netdev_register_provider(const struct netdev_class *new_class)
>  {
> -    struct netdev_class *new_provider;
> -
>     if (shash_find(&netdev_classes, new_class->type)) {
>         VLOG_WARN("attempted to register duplicate netdev provider: %s",
>                    new_class->type);
> @@ -138,10 +128,7 @@ netdev_register_provider(const struct netdev_class *new_class)
>         }
>     }
>
> -    new_provider = xmalloc(sizeof *new_provider);
> -    memcpy(new_provider, new_class, sizeof *new_provider);
> -
> -    shash_add(&netdev_classes, new_class->type, new_provider);
> +    shash_add(&netdev_classes, new_class->type, new_class);

This removes the malloc when registering but keeps the free on
unregister, so if anyone ever dynamically registers a provider there
will be a problem.