From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44433) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ehRmi-0001rR-Ao for qemu-devel@nongnu.org; Thu, 01 Feb 2018 22:14:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ehRle-0000Sc-8R for qemu-devel@nongnu.org; Thu, 01 Feb 2018 22:13:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:54200) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1ehRld-0000RT-RT for qemu-devel@nongnu.org; Thu, 01 Feb 2018 22:11:58 -0500 References: <1516936096-48900-1-git-send-email-jianjay.zhou@huawei.com> From: Jason Wang Message-ID: <0a1502a8-cbba-cf91-bf6b-e6058a4878e5@redhat.com> Date: Fri, 2 Feb 2018 11:11:47 +0800 MIME-Version: 1.0 In-Reply-To: <1516936096-48900-1-git-send-email-jianjay.zhou@huawei.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2] tap: close fd conditionally when error occured List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jay Zhou , qemu-devel@nongnu.org Cc: weidong.huang@huawei.com, mst@redhat.com, arei.gonglei@huawei.com, imammedo@redhat.com, wangxinxin.wang@huawei.com On 2018=E5=B9=B401=E6=9C=8826=E6=97=A5 11:08, Jay Zhou wrote: > If netdev_add tap,id=3Dnet0,...,vhost=3Don failed in net_init_tap_one()= , > the followed up device_add virtio-net-pci,netdev=3Dnet0 will fail > too, prints: > > TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD > ioctl() failed: Bad file descriptor > > The reason is that the fd of tap is closed when error occured after > calling net_init_tap_one(). > > The fd should be closed when calling net_init_tap_one failed: > - if tap_set_sndbuf() failed > - if tap_set_sndbuf() succeeded but vhost failed to initialize with > vhostforce flag on > The fd should not be closed just because vhost failed to initialize > but without vhostforce flag. So the followed up device_add can fall > back to userspace virtio successfully. > > Suggested-by: Michael S. Tsirkin > Suggested-by: Igor Mammedov > Suggested-by: Jason Wang > Signed-off-by: Jay Zhou > --- > net/tap.c | 40 ++++++++++++++++++++++++++++++++-------- > 1 file changed, 32 insertions(+), 8 deletions(-) > > diff --git a/net/tap.c b/net/tap.c > index 979e622..8042c7d 100644 > --- a/net/tap.c > +++ b/net/tap.c > @@ -648,12 +648,6 @@ static void net_init_tap_one(const NetdevTapOption= s *tap, NetClientState *peer, > TAPState *s =3D net_tap_fd_init(peer, model, name, fd, vnet_hdr); > int vhostfd; > =20 > - tap_set_sndbuf(s->fd, tap, &err); > - if (err) { > - error_propagate(errp, err); > - return; > - } > - > if (tap->has_fd || tap->has_fds) { > snprintf(s->nc.info_str, sizeof(s->nc.info_str), "fd=3D%d", f= d); > } else if (tap->has_helper) { > @@ -781,6 +775,12 @@ int net_init_tap(const Netdev *netdev, const char = *name, > =20 > vnet_hdr =3D tap_probe_vnet_hdr(fd); > =20 > + tap_set_sndbuf(fd, tap, &err); > + if (err) { > + error_propagate(errp, err); > + return -1; > + } > + > net_init_tap_one(tap, peer, "tap", name, NULL, > script, downscript, > vhostfdname, vnet_hdr, fd, &err); > @@ -832,6 +832,12 @@ int net_init_tap(const Netdev *netdev, const char = *name, > goto free_fail; > } > =20 > + tap_set_sndbuf(fd, tap, &err); > + if (err) { > + error_propagate(errp, err); > + goto free_fail; > + } > + > net_init_tap_one(tap, peer, "tap", name, ifname, > script, downscript, > tap->has_vhostfds ? vhost_fds[i] : NULL, > @@ -872,12 +878,21 @@ free_fail: > fcntl(fd, F_SETFL, O_NONBLOCK); > vnet_hdr =3D tap_probe_vnet_hdr(fd); > =20 > + tap_set_sndbuf(fd, tap, &err); > + if (err) { > + error_propagate(errp, err); > + close(fd); > + return -1; > + } > + > net_init_tap_one(tap, peer, "bridge", name, ifname, > script, downscript, vhostfdname, > vnet_hdr, fd, &err); > if (err) { > error_propagate(errp, err); > - close(fd); > + if (tap->has_vhostforce && tap->vhostforce) { > + close(fd); > + } > return -1; > } > } else { > @@ -910,13 +925,22 @@ free_fail: > } > } > =20 > + tap_set_sndbuf(fd, tap, &err); > + if (err) { > + error_propagate(errp, err); > + close(fd); > + return -1; > + } > + > net_init_tap_one(tap, peer, "tap", name, ifname, > i >=3D 1 ? "no" : script, > i >=3D 1 ? "no" : downscript, > vhostfdname, vnet_hdr, fd, &err); > if (err) { > error_propagate(errp, err); > - close(fd); > + if (tap->has_vhostforce && tap->vhostforce) { > + close(fd); > + } > return -1; > } > } Hi: I still fail to understand why not just pass force flag to=20 net_tap_init_one(), and let it decide? Thanks