Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize

[Zhoujian (jay)]

Sorry about missing to cc Jason.


Close the fd of the tap unconditionally when netdev_add tap,id=net0,vhost=on 
failed
in net_init_tap_one() will make the followed up device_add 
virtio-net-pci,netdev=net0
failed too, which prints:

    TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD 
    ioctl() failed: Bad file descriptor

This patch makes the followed up device_add be able to fall back to userspace 
virtio
when netdev_add failed with vhost turning on but vhost force flag does not set.

Here is the original discussion:
https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg05205.html


This is a RFC version, if I'm wrong, please let me know, thanks!

PS: there're two places updated, see below.


> -----Original Message-----
> From: Zhoujian (jay)
> Sent: Wednesday, January 10, 2018 12:40 AM
> To: address@hidden
> Cc: address@hidden; address@hidden; Huangweidong (C)
> <address@hidden>; wangxin (U) <address@hidden>; Gonglei
> (Arei) <address@hidden>; Liuzhe (Ahriy, Euler) <address@hidden>;
> Zhoujian (jay) <address@hidden>
> Subject: [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to
> initialize
> 
> Making the followed up device_add to fall back to userspace virtio when
> netdev_add fails if vhost force flag does not set.
> 
> Suggested-by: Michael S. Tsirkin <address@hidden>
> Suggested-by: Igor Mammedov <address@hidden>
> Signed-off-by: Jay Zhou <address@hidden>
> ---
>  net/tap.c | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/net/tap.c b/net/tap.c
> index 979e622..03f226f 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -642,7 +642,8 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
> NetClientState *peer,
>                               const char *model, const char *name,
>                               const char *ifname, const char *script,
>                               const char *downscript, const char *vhostfdname,
> -                             int vnet_hdr, int fd, Error **errp)
> +                             int vnet_hdr, int fd, Error **errp,
> +                             bool *vhost_init_failed)
>  {
>      Error *err = NULL;
>      TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr); @@ -
> 687,6 +688,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
> NetClientState *peer,
>              vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
>              if (vhostfd == -1) {
>                  error_propagate(errp, err);
> +                *vhost_init_failed = true;
>                  return;
>              }
>          } else {
> @@ -694,6 +696,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
> NetClientState *peer,
>              if (vhostfd < 0) {
>                  error_setg_errno(errp, errno,
>                                   "tap: open vhost char device failed");
> +                *vhost_init_failed = true;
>                  return;
>              }
>              fcntl(vhostfd, F_SETFL, O_NONBLOCK); @@ -704,6 +707,7 @@ static
> void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>          if (!s->vhost_net) {
>              error_setg(errp,
>                         "vhost-net requested but could not be initialized");
> +            *vhost_init_failed = true;
>              return;
>          }
>      } else if (vhostfdname) {
> @@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>      Error *err = NULL;
>      const char *vhostfdname;
>      char ifname[128];
> +    bool vhost_init_failed = false;
> 
>      assert(netdev->type == NET_CLIENT_DRIVER_TAP);
>      tap = &netdev->u.tap;
> @@ -783,7 +788,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
> 
>          net_init_tap_one(tap, peer, "tap", name, NULL,
>                           script, downscript,
> -                         vhostfdname, vnet_hdr, fd, &err);
> +                         vhostfdname, vnet_hdr, fd, &err,
> + &vhost_init_failed);
>          if (err) {
>              error_propagate(errp, err);
>              return -1;
> @@ -835,7 +840,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>              net_init_tap_one(tap, peer, "tap", name, ifname,
>                               script, downscript,
>                               tap->has_vhostfds ? vhost_fds[i] : NULL,
> -                             vnet_hdr, fd, &err);
> +                             vnet_hdr, fd, &err, &vhost_init_failed);
>              if (err) {
>                  error_propagate(errp, err);
>                  goto free_fail;
> @@ -874,10 +879,12 @@ free_fail:
> 
>          net_init_tap_one(tap, peer, "bridge", name, ifname,
>                           script, downscript, vhostfdname,
> -                         vnet_hdr, fd, &err);
> +                         vnet_hdr, fd, &err, &vhost_init_failed);
>          if (err) {
>              error_propagate(errp, err);
> -            close(fd);
> +            if (tap->has_vhostforce && tap->vhostforce && vhost_init_failed)
> {
> +                close(fd);
> +            }

This should be:
        
        if (!vhost_init_failed || (tap->has_vhostforce && tap->vhostforce)) {
            close(fd);
        }

>              return -1;
>          }
>      } else {
> @@ -913,10 +920,14 @@ free_fail:
>              net_init_tap_one(tap, peer, "tap", name, ifname,
>                               i >= 1 ? "no" : script,
>                               i >= 1 ? "no" : downscript,
> -                             vhostfdname, vnet_hdr, fd, &err);
> +                             vhostfdname, vnet_hdr, fd, &err,
> +                             &vhost_init_failed);
>              if (err) {
>                  error_propagate(errp, err);
> -                close(fd);
> +                if (tap->has_vhostforce && tap->vhostforce &&
> +                        vhost_init_failed) {
> +                    close(fd);
> +                }

Same here, this should be:

         if (!vhost_init_failed || (tap->has_vhostforce && tap->vhostforce)) {
             close(fd);
         }

>                  return -1;
>              }
>          }
> --
> 1.8.3.1
>