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: qemu-devel@nongnu.org > Cc: m...@redhat.com; imamm...@redhat.com; Huangweidong (C) > <weidong.hu...@huawei.com>; wangxin (U) <wangxinxin.w...@huawei.com>; Gonglei > (Arei) <arei.gong...@huawei.com>; Liuzhe (Ahriy, Euler) <liuzh...@huawei.com>; > Zhoujian (jay) <jianjay.z...@huawei.com> > 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 <m...@redhat.com> > Suggested-by: Igor Mammedov <imamm...@redhat.com> > Signed-off-by: Jay Zhou <jianjay.z...@huawei.com> > --- > 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 >