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

[Jason Wang]

On 2018年01月10日 12:18, Zhoujian (jay) wrote:
> 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;

Why not simply check s->vhost_net after call net_init_tap_one()?

Thanks

> >               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