[ovs-dev] [netdev 23/27] netdev-bsd: Make use of AF_LINK socket thread-safe in NetBSD.

[YAMAMOTO Takashi]

looks ok to me.

YAMAMOTO Takashi

> Signed-off-by: Ben Pfaff <blp at nicira.com>
> CC: Ed Maste <emaste at freebsd.org>
> CC: YAMAMOTO Takashi <yamt at mwd.biglobe.ne.jp>
> ---
>  lib/netdev-bsd.c |   91 +++++++++++++++++++++++++-----------------------------
>  1 file changed, 42 insertions(+), 49 deletions(-)
> 
> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
> index 503207f..d365ebf 100644
> --- a/lib/netdev-bsd.c
> +++ b/lib/netdev-bsd.c
> @@ -50,6 +50,7 @@
>  #include "fatal-signal.h"
>  #include "ofpbuf.h"
>  #include "openflow/openflow.h"
> +#include "ovs-thread.h"
>  #include "packets.h"
>  #include "poll-loop.h"
>  #include "socket-util.h"
> @@ -107,11 +108,6 @@ enum {
>      VALID_CARRIER = 1 << 5
>  };
>  
> -#if defined(__NetBSD__)
> -/* AF_LINK socket used for netdev_bsd_get_stats and set_etheraddr */
> -static int af_link_sock = -1;
> -#endif /* defined(__NetBSD__) */
> -
>  #define PCAP_SNAPLEN 2048
>  
>  
> @@ -144,6 +140,10 @@ static int get_ifindex(const struct netdev *, int *ifindexp);
>  static int ifr_get_flags(const struct ifreq *);
>  static void ifr_set_flags(struct ifreq *, int flags);
>  
> +#ifdef __NetBSD__
> +static int af_link_ioctl(int command, const void *arg);
> +#endif
> +
>  static int netdev_bsd_init(void);
>  
>  static bool
> @@ -172,29 +172,6 @@ netdev_get_kernel_name(const struct netdev *netdev)
>      return netdev_bsd_cast(netdev)->kernel_name;
>  }
>  
> -/* Initialize the AF_INET socket used for ioctl operations */
> -static int
> -netdev_bsd_init(void)
> -{
> -#if defined(__NetBSD__)
> -    static int status = -1;
> -
> -    if (status >= 0) {  /* already initialized */
> -        return status;
> -    }
> -
> -    af_link_sock = socket(AF_LINK, SOCK_DGRAM, 0);
> -    status = af_link_sock >= 0 ? 0 : errno;
> -    if (status) {
> -        VLOG_ERR("failed to create link socket: %s", ovs_strerror(status));
> -    }
> -
> -    return status;
> -#else
> -    return 0;
> -#endif /* defined(__NetBSD__) */
> -}
> -
>  /*
>   * Perform periodic work needed by netdev. In BSD netdevs it checks for any
>   * interface status changes, and eventually calls all the user callbacks.
> @@ -931,19 +908,16 @@ netdev_bsd_get_stats(const struct netdev *netdev_, struct netdev_stats *stats)
>      return 0;
>  #elif defined(__NetBSD__)
>      struct ifdatareq ifdr;
> -    int saved_errno;
> -    int ret;
> +    int error;
>  
>      memset(&ifdr, 0, sizeof(ifdr));
>      strncpy(ifdr.ifdr_name, netdev_get_kernel_name(netdev_),
>              sizeof(ifdr.ifdr_name));
> -    ret = ioctl(af_link_sock, SIOCGIFDATA, &ifdr);
> -    saved_errno = errno;
> -    if (ret == -1) {
> -        return saved_errno;
> +    error = af_link_ioctl(SIOCGIFDATA, &ifdr);
> +    if (!error) {
> +        convert_stats(stats, &ifdr.ifdr_data);
>      }
> -    convert_stats(stats, &ifdr.ifdr_data);
> -    return 0;
> +    return error;
>  #else
>  #error not implemented
>  #endif
> @@ -1402,7 +1376,7 @@ netdev_bsd_change_seq(const struct netdev *netdev)
>  const struct netdev_class netdev_bsd_class = {
>      "system",
>  
> -    netdev_bsd_init,
> +    NULL, /* init */
>      netdev_bsd_run,
>      netdev_bsd_wait,
>      netdev_bsd_create_system,
> @@ -1625,7 +1599,7 @@ set_etheraddr(const char *netdev_name OVS_UNUSED, int hwaddr_family OVS_UNUSED,
>      struct if_laddrreq req;
>      struct sockaddr_dl *sdl;
>      struct sockaddr_storage oldaddr;
> -    int ret;
> +    int error;
>  
>      /*
>       * get the old address, add new one, and then remove old one.
> @@ -1641,9 +1615,10 @@ set_etheraddr(const char *netdev_name OVS_UNUSED, int hwaddr_family OVS_UNUSED,
>      req.addr.ss_family = hwaddr_family;
>      sdl = (struct sockaddr_dl *)&req.addr;
>      sdl->sdl_alen = hwaddr_len;
> -    ret = ioctl(af_link_sock, SIOCGLIFADDR, &req);
> -    if (ret == -1) {
> -        return errno;
> +
> +    error = af_link_ioctl(SIOCGLIFADDR, &req);
> +    if (error) {
> +        return error;
>      }
>      if (!memcmp(&sdl->sdl_data[sdl->sdl_nlen], mac, hwaddr_len)) {
>          return 0;
> @@ -1658,19 +1633,15 @@ set_etheraddr(const char *netdev_name OVS_UNUSED, int hwaddr_family OVS_UNUSED,
>      sdl->sdl_alen = hwaddr_len;
>      sdl->sdl_family = hwaddr_family;
>      memcpy(sdl->sdl_data, mac, hwaddr_len);
> -    ret = ioctl(af_link_sock, SIOCALIFADDR, &req);
> -    if (ret == -1) {
> -        return errno;
> +    error = af_link_ioctl(SIOCALIFADDR, &req);
> +    if (error) {
> +        return error;
>      }
>  
>      memset(&req, 0, sizeof(req));
>      strncpy(req.iflr_name, netdev_name, sizeof(req.iflr_name));
>      req.addr = oldaddr;
> -    ret = ioctl(af_link_sock, SIOCDLIFADDR, &req);
> -    if (ret == -1) {
> -        return errno;
> -    }
> -    return 0;
> +    return af_link_ioctl(SIOCDLIFADDR, &req);
>  #else
>  #error not implemented
>  #endif
> @@ -1694,3 +1665,25 @@ ifr_set_flags(struct ifreq *ifr, int flags)
>      ifr->ifr_flagshigh = flags >> 16;
>  #endif
>  }
> +
> +/* Calls ioctl() on an AF_LINK sock, passing the specified 'command' and
> + * 'arg'.  Returns 0 if successful, otherwise a positive errno value. */
> +int
> +af_link_ioctl(int command, const void *arg)
> +{
> +    static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> +    static int sock;
> +
> +    if (ovsthread_once_start(&once)) {
> +        sock = socket(AF_LINK, SOCK_DGRAM, 0);
> +        if (sock < 0) {
> +            sock = -errno;
> +            VLOG_ERR("failed to create link socket: %s", ovs_strerror(errno));
> +        }
> +        ovsthread_once_done(&once);
> +    }
> +
> +    return (sock < 0 ? -sock
> +            : ioctl(sock, command, arg) == -1 ? errno
> +            : 0);
> +}
> -- 
> 1.7.10.4
> 
> 
>