[libvirt] [PATCH v2 3/5] FreeBSD: implement virNetDevTapCreate() and virNetDevTapDelete().

[John Ferlan]

On 01/20/2013 11:22 AM, Roman Bogorodskiy wrote:
> ---
>  src/util/virnetdevtap.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 109 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c
> index a884de1..1b02e1f 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,113 @@ cleanup:
>      VIR_FORCE_CLOSE(fd);
>      return ret;
>  }
> -#else /* ! TUNSETIFF */
> +#elif defined(__FreeBSD__)
> +int virNetDevTapCreate(char **ifname,
> +                       int *tapfd,
> +                       unsigned int flags ATTRIBUTE_UNUSED)
> +{
> +    int s;
> +    struct ifreq ifr;
> +    int ret = -1;
> +    char *newifname = NULL;
> +
> +    /* As FreeBSD determines interface type by name,
> +     * we have to create 'tap' interface first and
> +     * then rename it to 'vnet'
> +     */
> +    if ((s = virNetDevSetupControl("tap", &ifr)) < 0)
> +        return -1;
> +
> +    if (ioctl(s, SIOCIFCREATE2, &ifr) < 0) {
> +        virReportSystemError(errno, "%s",
> +                             _("Unable to create tap device"));
> +        goto cleanup;
> +    }


Trying to figure the relationship between this socket and the file (eg,
tapfd) created below).


> +
> +    /* In case we were given exact interface name (e.g. 'vnetN'),
> +     * we just rename to it. If we have format string like
> +     * 'vnet%d', we need to find the first available name that
> +     * matches this pattern
> +     */
> +    if (strstr(*ifname, "%d") == NULL) {
> +         newifname = strdup(*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();
> +                goto cleanup;
> +            }
> +
> +            if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) {
> +                newifname = strdup(virBufferContentAndReset(&newname));
> +                break;
> +            }
> +        }
> +    }
> +
> +    VIR_FREE(*ifname);
> +
> +    if (newifname == NULL) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("Failed to generate new name for interface %s"),
> +                       ifr.ifr_name);
> +        goto cleanup;
> +    }

My comments from v1 still apply above. Since the "else" part of the
above check is just replacing *ifname, then that's the only time we need
to mess with *ifname.  That is use "!=" and just replace *ifname at the
end of the for loop as long as we generated one. The error message here
would be wrong if you went through the existing "if" condition - it
failed to strdup().  Thus you have:

if (strstr(*ifname, "%d") != NULL) {
    int i;
    for (i = 0; i <= 255; i++) {
        virBuffer newname = VIR_BUFFER_INITIALIZER;
        virBufferAsprintf(&newname, *ifname, i);

        if (virBufferError(&newname)) {
            virBufferFreeAndReset(&newname);
            virReportOOMError();
            goto cleanup;
        }

        if (virNetDevExists(virBufferCurrentContent(&newname)) == 0) {
            newifname = strdup(virBufferContentAndReset(&newname));
            break;
        }
    }
    if (newifname) {
        VIR_FREE(*ifname);
        *ifname = newifname;
    } else {
        virReportError(VIR_ERR_INTERNAL_ERROR,
                     _("Failed to generate new name for interface %s"),
                       ifr.ifr_name);
        goto cleanup;
    }
}

Also, I missed this the first time around, is there an existing constant
for 255? Or is it/will it be dynamic?

Beyond that I have some "thoughts" around the use of 255. First, it
could be a time consuming loop - there's a lot of open, ioctl/check,
close going on (and that doesn't include all the printf & Buffer mgmt code).

Second, what are the chances some day someone wants 1024 tap devices.
This loop is then outdated and even worse performance wise. It's too bad
there isn't a way to just request the next available device and let the
driver handle things rather than a linear algorithm which will cause
performance problems some day. Yes, I've been down this road before.


> +
> +    if (virNetDevSetName(ifr.ifr_name, newifname) == -1) {

Assuming the above change, then 's/newifname/*ifname/'


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

Removing the above too.

The way the code is written now, if you goto cleanup from the SetName,
then newifname is leaked.

> +
> +    if (tapfd) {
> +        char *dev_path = NULL;
> +        if (virAsprintf(&dev_path, "/dev/%s", *ifname) < 0) {
> +            virReportOOMError();
> +            goto cleanup;
> +        }
> +
> +        *tapfd = open(dev_path, O_RDWR);
> +
> +        VIR_FREE(dev_path);
> +    }
> +
> +    ret = 0;
> +cleanup:
> +    if (ret < 0)
> +        VIR_FORCE_CLOSE(s);
> +
> +    return ret;
> +}

On success, we leave here with 's' and 'tapfd' being opened, right?
Since the TapDelete will open another 's', should the "if (ret < 0)"
above be removed and we always close 's'?

If tapfd == NULL, then what happens?  Shouldn't the device path still be
opened & closed (like the "existing" code)? As it stands you return
zero, but haven't necessarily opened the new dev_path. I'm trying to
learn the relationship between the two.

What happens when/if *tapfd == -1 because open() failed? and ret = 0?

> +
> +int virNetDevTapDelete(const char *ifname)
> +{
> +    int s;
> +    struct ifreq ifr;
> +    int ret = -1;
> +
> +    if ((s = virNetDevSetupControl(ifname, &ifr)) < 0)
> +        return -1;
> +
> +    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 /* !defined(__FreeBSD__) */
>  int virNetDevTapCreate(char **ifname ATTRIBUTE_UNUSED,
>                         int *tapfd ATTRIBUTE_UNUSED,
>                         unsigned int flags ATTRIBUTE_UNUSED)
> @@ -245,7 +351,7 @@ int virNetDevTapDelete(const char *ifname ATTRIBUTE_UNUSED)
>                           _("Unable to delete TAP devices on this platform"));
>      return -1;
>  }
> -#endif /* ! TUNSETIFF */
> +#endif
>  
>  
>  /**
>