[libvirt] [PATCH 2/4] FreeBSD: implement virNetDevTapCreate() and virNetDevTapDelete().

[John Ferlan]

On 01/04/2013 10:00 AM, Roman Bogorodskiy wrote:
> ---
>  src/util/virnetdevtap.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 137 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> index a884de1..426d629 100644
> --- a/src/util/virnetdevtap.c
> +++ b/src/util/virnetdevtap.c
> @@ -103,7 +103,7 @@ virNetDevProbeVnetHdr(int tapfd)
>  #endif
>  
>  
> -#ifdef TUNSETIFF
> +#if defined(TUNSETIFF)
>  /**
>   * virNetDevTapCreate:
>   * @ifname: the interface name
> @@ -230,7 +230,141 @@ cleanup:
>      VIR_FORCE_CLOSE(fd);
>      return ret;
>  }
> -#else /* ! TUNSETIFF */

Change the comment to add the !defined(TUNSETIFF)

> +#elif defined(__FreeBSD__)
> +int virNetDevTapCreate(char **ifname,
> +                       int *tapfd,
> +                       unsigned int flags ATTRIBUTE_UNUSED)
> +{
> +    int s;
> +    struct ifreq ifr;
> +    int ret = -1;
> +
> +    s = socket(AF_LOCAL, SOCK_DGRAM, 0);
> +    if (s < 0) {
> +        virReportSystemError(errno, "%s",
> +                             _("Unable to create socket"));
> +        return -1;
> +    }
> +
> +    memset(&ifr, 0, sizeof(ifr));
> +
> +    if (virStrcpyStatic(ifr.ifr_name, "tap") == NULL) {
> +        virReportSystemError(ERANGE,
> +                             _("Network interface name '%s' is too long"),
> +                             *ifname);
> +        goto cleanup;
> +
> +    }

Confused here - you are copying "tap", but using "*ifname" in the error
message.  Why not use the virNetDevSetupControl() if in fact you are
trying to use "*ifname"?

> +
> +    if (ioctl(s, SIOCIFCREATE2, &ifr) < 0) {
> +        virReportSystemError(errno,
> +                             _("Unable to create tap device %s"),
> +                             NULLSTR(*ifname));
> +        goto cleanup;
> +    }
> +
> +    char *newifname = NULL;

Put this with the other declarations.

> +    if (strstr(*ifname, "%d") == NULL) {
> +         newifname = *ifname;
> +    } else {
> +        int i;
> +        for (i = 0; i <= 255; i++) {
> +            virBuffer newname = VIR_BUFFER_INITIALIZER;
> +            virBufferAsprintf(&newname, *ifname, i);
> +
> +            if (virBufferError(&newname)) {
> +                virBufferFreeAndReset(&newname);
> +                virReportOOMError();
> +                ret = - 1;

Redundant with initialization

> +                goto cleanup;
> +            }
> +
> +            if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) {
> +                newifname = strdup(virBufferContentAndReset(&newname));
> +                break;
> +            }

In this instance memory is allocated for newifname, when is it free()'d?
Secondarily if not allocated, then the general message below may mask
the reason.  That is although you failed to generate a new name, it was
because you were out of memory.

> +        }
> +    }
> +
> +    if (newifname == NULL) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to generate new name for interface %s"),
> +                       ifr.ifr_name);
> +        ret = -1;

Redundant with initialization

> +        goto cleanup;
> +    }
> +
> +    if (virNetDevSetName(ifr.ifr_name, newifname) == -1) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to rename interface %s to %s"),
> +                       ifr.ifr_name, newifname);

This message is redundant with the virNetDevSetName() function.

> +        ret = -1;

Redundant with initialization

> +        goto cleanup;
> +    }
> +
> +    *ifname = newifname;

This is where a possible memory leak occurs if *ifname had been alloc'd
before we'd be now losing it. I see the other virNetDevTapCreate() will
VIR_FREE(*ifname) prior to strdup() attempt.

Secondarily, it seems to me that if we go through the "if strstr() ==
NULL" path, then we're not really changing the name, correct?  If that's
the case, then the only time you need to change the name is in the else
condition.

I think the answer probably goes back to the virStrcpyStatic using "tap"
above for what the expectations are here.

> +
> +    if (tapfd) {
> +        char *dev_prefix = "/dev/";
> +        char *dev_path;
> +        int dev_path_len = strlen(dev_prefix) +  strlen(*ifname) + 1;
> +
> +        if (VIR_ALLOC_N(dev_path, dev_path_len) < 0) {
> +            virReportOOMError();
> +            ret = -1;

Redundant with initialization

> +            goto cleanup;
> +        }
> +        snprintf(dev_path, dev_path_len, "%s%s", dev_prefix, *ifname);
> +
> +        *tapfd = open(dev_path, O_RDWR);
> +
> +        VIR_FREE(dev_path);
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    if (ret < 0)
> +        VIR_FORCE_CLOSE(s);
> +
> +    return ret;
> +}
> +
> +int virNetDevTapDelete(const char *ifname)
> +{
> +    int s;
> +    struct ifreq ifr;
> +    int ret = -1;
> +
> +    s = socket(AF_LOCAL, SOCK_DGRAM, 0);
> +    if (s < 0) {
> +        virReportSystemError(errno, "%s",
> +                             _("Unable to create socket"));
> +        return -1;
> +    }
> +
> +    memset(&ifr, 0, sizeof(ifr));
> +
> +    if (virStrcpyStatic(ifr.ifr_name, ifname) == NULL) {
> +        virReportSystemError(ERANGE,
> +                             _("Network interface name '%s' is too long"),
> +                             ifname);
> +        goto cleanup;
> +    }

Why not use virNetDevSetupControl()?
> +
> +    if (ioctl(s, SIOCIFDESTROY, &ifr) < 0) {
> +        virReportSystemError(errno,
> +                             _("Unable to remove tap device %s"),
> +                             ifname);
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    VIR_FORCE_CLOSE(s);
> +    return ret;
> +}
> +
> +#else
>  int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED,
>                         int *tapfd ATTRIBUTE_UNUSED,
>                         unsigned int flags ATTRIBUTE_UNUSED)
> @@ -245,7 +379,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED)
>                           _("Unable to delete TAP devices on this platform"));
>      return -1;
>  }
> -#endif /* ! TUNSETIFF */
> +#endif
>  

Add a /* defined(__FreeBSD__) */ to the #endif for clarity
>  
>  /**
>