[ovs-dev] [netdev 19/27] netdev: Make netdev_get_devices() take a reference to each netdev.

[Andy Zhou]

On Thu, Aug 1, 2013 at 2:29 PM, Ben Pfaff <blp at nicira.com> wrote:

> This API change is necessary for thread safety, to be added in an upcoming
> commit.  Otherwise, the client would not be able to actually use any of
> the returned netdevs because they could already have been destroyed.
>
> Signed-off-by: Ben Pfaff <blp at nicira.com>
> ---
>  lib/netdev-bsd.c   |   32 ++++++++++----------------------
>  lib/netdev-linux.c |    4 ++++
>  lib/netdev.c       |    5 +++--
>  3 files changed, 17 insertions(+), 24 deletions(-)
>
> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
> index 6ff6b3e..d6a8631 100644
> --- a/lib/netdev-bsd.c
> +++ b/lib/netdev-bsd.c
> @@ -255,6 +255,7 @@ netdev_bsd_cache_cb(const struct rtbsd_change *change,
>                  dev->cache_valid = 0;
>                  netdev_bsd_changed(dev);
>              }
> +            netdev_close(base_dev);
>          }
>      } else {
>          /*
> @@ -271,6 +272,7 @@ netdev_bsd_cache_cb(const struct rtbsd_change *change,
>              dev = netdev_bsd_cast(netdev);
>              dev->cache_valid = 0;
>              netdev_bsd_changed(dev);
> +            netdev_close(dev);
>          }
>          shash_destroy(&device_shash);
>      }
> @@ -1200,9 +1202,10 @@ netdev_bsd_get_in6(const struct netdev *netdev_,
> struct in6_addr *in6)
>  }
>
>  #if defined(__NetBSD__)
> -static struct netdev *
> -find_netdev_by_kernel_name(const char *kernel_name)
> +static char *
> +netdev_bsd_kernel_name_to_ovs_name(const char *kernel_name)
>  {
> +    char *ovs_name = NULL;
>      struct shash device_shash;
>      struct shash_node *node;
>
> @@ -1213,24 +1216,14 @@ find_netdev_by_kernel_name(const char *kernel_name)
>          struct netdev_bsd * const dev = netdev_bsd_cast(netdev);
>
>          if (!strcmp(dev->kernel_name, kernel_name)) {
> -            shash_destroy(&device_shash);
> -            return &dev->up;
> +            free(ovs_name);
> +            ovs_name = xstrdup(netdev_get_name(&dev->up));
>
Would netdev_close to be called for &dev->up ?

>          }
> +        netdev_close(netdev);
>      }
>      shash_destroy(&device_shash);
> -    return NULL;
> -}
> -
> -static const char *
> -netdev_bsd_convert_kernel_name_to_ovs_name(const char *kernel_name)
> -{
> -    const struct netdev * const netdev =
> -      find_netdev_by_kernel_name(kernel_name);
>
> -    if (netdev == NULL) {
> -        return NULL;
> -    }
> -    return netdev_get_name(netdev);
> +    return ovs_name ? ovs_name : xstrdup(kernel_name);
>  }
>  #endif
>
> @@ -1315,12 +1308,7 @@ netdev_bsd_get_next_hop(const struct in_addr *host
> OVS_UNUSED,
>                  char *kernel_name;
>
>                  kernel_name = xmemdup0(sdl->sdl_data, sdl->sdl_nlen);
> -                name =
> netdev_bsd_convert_kernel_name_to_ovs_name(kernel_name);
> -                if (name == NULL) {
> -                    ifname = xstrdup(kernel_name);
> -                } else {
> -                    ifname = xstrdup(name);
> -                }
> +                ifname = netdev_bsd_kernel_name_to_ovs_name(kernel_name);
>                  free(kernel_name);
>              }
>              RT_ADVANCE(cp, sa);
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index ba0d863..5bbaf63 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -555,6 +555,7 @@ netdev_linux_cache_cb(const struct
> rtnetlink_link_change *change,
>
>              get_flags(&dev->up, &flags);
>              netdev_linux_changed(dev, flags, 0);
> +            netdev_close(netdev);
>          }
>          shash_destroy(&device_shash);
>      }
> @@ -1180,6 +1181,7 @@ netdev_linux_miimon_run(void)
>          bool miimon;
>
>          if (dev->miimon_interval <= 0 ||
> !timer_expired(&dev->miimon_timer)) {
> +            netdev_close(netdev);
>              continue;
>          }
>
> @@ -1190,6 +1192,7 @@ netdev_linux_miimon_run(void)
>          }
>
>          timer_set_duration(&dev->miimon_timer, dev->miimon_interval);
> +        netdev_close(netdev);
>      }
>
>      shash_destroy(&device_shash);
> @@ -1210,6 +1213,7 @@ netdev_linux_miimon_wait(void)
>          if (dev->miimon_interval > 0) {
>              timer_wait(&dev->miimon_timer);
>          }
> +        netdev_close(netdev);
>      }
>      shash_destroy(&device_shash);
>  }
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 0079dc2..2561538 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -1395,8 +1395,8 @@ netdev_from_name(const char *name)
>
>  /* Fills 'device_list' with devices that match 'netdev_class'.
>   *
> - * The caller is responsible for initializing and destroying 'device_list'
> - * but the contained netdevs must not be freed. */
> + * The caller is responsible for initializing and destroying
> 'device_list' and
> + * must close each device on the list. */
>  void
>  netdev_get_devices(const struct netdev_class *netdev_class,
>                     struct shash *device_list)
> @@ -1406,6 +1406,7 @@ netdev_get_devices(const struct netdev_class
> *netdev_class,
>          struct netdev *dev = node->data;
>
>          if (dev->netdev_class == netdev_class) {
> +            dev->ref_cnt++;
>              shash_add(device_list, node->name, node->data);
>          }
>      }
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130805/fe314db9/attachment-0003.html>