[libvirt] [PATCH 3/6] virNetDevMacVLanTapOpen: Rework to support multiple FDs

[Laine Stump]

On 12/04/2015 07:30 AM, Michal Privoznik wrote:
> So, for the multiqueue on macvtaps we are going to need to open

s/So, for the multiqueue/For multiqueue support/
> the device multiple times. Currently, this is not supported.
> Rework the function, so that upper layers can be reworked too.
>
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>   src/util/virnetdevmacvlan.c | 58 ++++++++++++++++++++++++++++-----------------
>   1 file changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/src/util/virnetdevmacvlan.c b/src/util/virnetdevmacvlan.c
> index c1a5f0f..d20990b 100644
> --- a/src/util/virnetdevmacvlan.c
> +++ b/src/util/virnetdevmacvlan.c
> @@ -226,15 +226,21 @@ int virNetDevMacVLanDelete(const char *ifname)
>   
>   /**
>    * virNetDevMacVLanTapOpen:
> - * Open the macvtap's tap device.
>    * @ifname: Name of the macvtap interface
> + * @tapfd: array of file descriptor return value for the new macvtap device
> + * @tapfdSize: number of file descriptors in @tapfd
>    * @retries : Number of retries in case udev for example may need to be
>    *            waited for to create the tap chardev
> - * Returns negative value in case of error, the file descriptor otherwise.
> + *
> + * Open the macvtap's tap device, possibly multiple times if @tapfdSize > 1.
> + *
> + * Returns 0 on success, -1 otherwise.
>    */
> -static
> -int virNetDevMacVLanTapOpen(const char *ifname,
> -                            int retries)
> +static int
> +virNetDevMacVLanTapOpen(const char *ifname,
> +                        int *tapfd,
> +                        size_t tapfdSize,
> +                        int retries)
>   {
>       int ret = -1;
>       char *path;
> @@ -242,7 +248,8 @@ int virNetDevMacVLanTapOpen(const char *ifname,
>       char *tapname = NULL;
>       char *buf = NULL;
>       char *c;
> -    int tapfd;
> +    size_t i = 0;
> +    int fd = -1;
>   
>       if (virNetDevSysfsFile(&path, ifname, "ifindex") < 0)
>           return -1;
> @@ -260,25 +267,32 @@ int virNetDevMacVLanTapOpen(const char *ifname,
>       if (virAsprintf(&tapname, "/dev/tap%d", ifindex) < 0)
>           goto cleanup;
>   
> -    while (1) {
> -        /* may need to wait for udev to be done */
> -        tapfd = open(tapname, O_RDWR);
> -        if (tapfd < 0 && retries > 0) {
> -            retries--;
> -            usleep(20000);
> -            continue;
> +    for (i = 0; i < tapfdSize; i++) {
> +     retry:
> +        fd = open(tapname, O_RDWR);
> +        if (fd < 0) {
> +            if (retries > 0) {
> +                /* may need to wait for udev to be done */
> +                retries--;
> +                usleep(20000);
> +                goto retry;
> +            }
> +            /* However, if haven't succeeded, quit. */
> +            virReportSystemError(errno,
> +                                 _("cannot open macvtap tap device %s"),
> +                                 tapname);
> +            goto cleanup;
>           }
> -        break;
> +        tapfd[i] = fd;
>       }

Although libvirt very commonly uses goto essentially for "exception 
handling in C", and that does make the code quite a bit cleaner, I still 
have an aversion to goto created by the graders in my very first Pascal 
class eons ago - if you had a goto, it wasn't accepted unless you 
explained how you would have written the code without it, and then they 
*still* took half a mark from your grade.

This is why when I see the above, I would prefer to write something like 
this:

     for (i = 0; i < tapfdSize; i++) {
          int fd = -1;
          while (fd < 0) {
              if ((fd = open(tapname, O_RDWR)) >= 0) {
                  tapfd[i] = fd;
              } else if (retries-- > 0) {
                  usleep(20000);
              } else {
                  virReportSystemError(errno,
                                      _("cannot open macvtap tap device %s"),
                                        tapname);
                  goto cleanup;
              }
         }
     }

Note that this has the effect of allowing some retries to count when 
opening the first fd, and other retries to count when opening the others 
(similar to your loop, btw). Nothing wrong with this, since the retries 
are there to wait for udev to complete creation of the tap device (so 
once it's done, we'll never need to wait for it again), just pointing it 
out.

ACK. It's up to you whether you want to use the loop with an extra label 
or the one without; I'm not going to insist on it (but my grader from 
[redacted to avoid shocking anyone] years ago would frown at me in my 
sleep if I didn't mention it :-))


>   
> -    if (tapfd < 0) {
> -        virReportSystemError(errno,
> -                             _("cannot open macvtap tap device %s"),
> -                             tapname);
> -        goto cleanup;
> -    }
> -    ret = tapfd;
> +    ret = 0;
> +
>    cleanup:
> +    if (ret < 0) {
> +        while (i--)
> +            VIR_FORCE_CLOSE(tapfd[i]);
> +    }
>       VIR_FREE(path);
>       VIR_FREE(tapname);
>       VIR_FREE(buf);
> @@ -847,7 +861,7 @@ int virNetDevMacVLanCreateWithVPortProfile(const char *tgifname,
>       }
>   
>       if (flags & VIR_NETDEV_MACVLAN_CREATE_WITH_TAP) {
> -        if ((rc = virNetDevMacVLanTapOpen(cr_ifname, 10)) < 0)
> +        if (virNetDevMacVLanTapOpen(cr_ifname, &rc, 1, 10) < 0)
>               goto disassociate_exit;
>   
>           if (virNetDevMacVLanTapSetup(rc, vnet_hdr) < 0) {