> -----Original Message-----
> From: Jason Wang [mailto:jasow...@redhat.com]
> Sent: Wednesday, January 10, 2018 2:02 PM
> To: Zhoujian (jay) <jianjay.z...@huawei.com>; qemu-devel@nongnu.org
> Cc: Huangweidong (C) <weidong.hu...@huawei.com>; m...@redhat.com; wangxin (U)
> <wangxinxin.w...@huawei.com>; Gonglei (Arei) <arei.gong...@huawei.com>;
> imamm...@redhat.com; Liuzhe (Ahriy, Euler) <liuzh...@huawei.com>
> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only
> vhost failed to initialize
>
>
>
> 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: 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;
>
> Why not simply check s->vhost_net after call net_init_tap_one()?
s->vhost_net is always NULL if net_init_tap_one() failed, it can't distinguish
failure reasons.
fd should be closed in these two cases:
(1) tap_set_sndbuf() failed, regardless of whether having vhost or vhostforce
flag
(2) with vhostforce flag
fd should not be closed if tap_set_sndbuf() succeeded and vhost init failed
without
vhostforce flag.
Regards,
Jay
>
> 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
> >>
> >
