* [Qemu-devel] [PATCH] tap: close fd conditionally when error occured
@ 2018-01-12 7:30 Jay Zhou
2018-01-15 7:21 ` Jason Wang
0 siblings, 1 reply; 3+ messages in thread
From: Jay Zhou @ 2018-01-12 7:30 UTC (permalink / raw)
To: qemu-devel
Cc: jasowang, mst, imammedo, weidong.huang, wangxinxin.wang,
arei.gonglei, jianjay.zhou
If netdev_add tap,id=net0,vhost=on failed in net_init_tap_one(),
the followed up device_add virtio-net-pci,netdev=net0 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().
I think the fd should be closed in these two case:
1.tap_set_sndbuf() failed
2.tap_set_sndbuf() succeeded but vhost failed to initialize with
vhostforce flag on
Meanwhile, 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 <mst@redhat.com>
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
---
net/tap.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 979e622..3ed72eb 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -651,6 +651,9 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
tap_set_sndbuf(s->fd, tap, &err);
if (err) {
error_propagate(errp, err);
+ if (!tap->has_fd && !tap->has_fds) {
+ close(fd);
+ }
return;
}
@@ -687,14 +690,14 @@ 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);
- return;
+ goto cleanup;
}
} else {
vhostfd = open("/dev/vhost-net", O_RDWR);
if (vhostfd < 0) {
error_setg_errno(errp, errno,
"tap: open vhost char device failed");
- return;
+ goto cleanup;
}
fcntl(vhostfd, F_SETFL, O_NONBLOCK);
}
@@ -704,11 +707,18 @@ 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");
- return;
+ goto cleanup;
}
} else if (vhostfdname) {
error_setg(errp, "vhostfd(s)= is not valid without vhost");
}
+
+cleanup:
+ if (!tap->has_fd && !tap->has_fds && tap->has_vhostforce &&
+ tap->vhostforce) {
+ close(fd);
+ }
+ return;
}
static int get_fds(char *str, char *fds[], int max)
@@ -877,7 +887,6 @@ free_fail:
vnet_hdr, fd, &err);
if (err) {
error_propagate(errp, err);
- close(fd);
return -1;
}
} else {
@@ -916,7 +925,6 @@ free_fail:
vhostfdname, vnet_hdr, fd, &err);
if (err) {
error_propagate(errp, err);
- close(fd);
return -1;
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [PATCH] tap: close fd conditionally when error occured
2018-01-12 7:30 [Qemu-devel] [PATCH] tap: close fd conditionally when error occured Jay Zhou
@ 2018-01-15 7:21 ` Jason Wang
2018-01-15 12:55 ` Zhoujian (jay)
0 siblings, 1 reply; 3+ messages in thread
From: Jason Wang @ 2018-01-15 7:21 UTC (permalink / raw)
To: Jay Zhou, qemu-devel
Cc: mst, imammedo, weidong.huang, wangxinxin.wang, arei.gonglei
On 2018年01月12日 15:30, Jay Zhou wrote:
> If netdev_add tap,id=net0,vhost=on failed in net_init_tap_one(),
> the followed up device_add virtio-net-pci,netdev=net0 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().
>
> I think the fd should be closed in these two case:
> 1.tap_set_sndbuf() failed
> 2.tap_set_sndbuf() succeeded but vhost failed to initialize with
> vhostforce flag on
> Meanwhile, 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 <mst@redhat.com>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
> ---
> net/tap.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index 979e622..3ed72eb 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -651,6 +651,9 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
> tap_set_sndbuf(s->fd, tap, &err);
> if (err) {
> error_propagate(errp, err);
> + if (!tap->has_fd && !tap->has_fds) {
> + close(fd);
> + }
> return;
> }
>
> @@ -687,14 +690,14 @@ 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);
> - return;
> + goto cleanup;
> }
> } else {
> vhostfd = open("/dev/vhost-net", O_RDWR);
> if (vhostfd < 0) {
> error_setg_errno(errp, errno,
> "tap: open vhost char device failed");
> - return;
> + goto cleanup;
> }
> fcntl(vhostfd, F_SETFL, O_NONBLOCK);
> }
> @@ -704,11 +707,18 @@ 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");
So error_setg() is not appropriate here consider it was not trated as an
error. We probably just need some warning.
> - return;
> + goto cleanup;
> }
> } else if (vhostfdname) {
> error_setg(errp, "vhostfd(s)= is not valid without vhost");
> }
> +
> +cleanup:
> + if (!tap->has_fd && !tap->has_fds && tap->has_vhostforce &&
> + tap->vhostforce) {
> + close(fd);
I would still let caller to decide whether or not to close the fd.
Thanks
> + }
> + return;
> }
>
> static int get_fds(char *str, char *fds[], int max)
> @@ -877,7 +887,6 @@ free_fail:
> vnet_hdr, fd, &err);
> if (err) {
> error_propagate(errp, err);
> - close(fd);
> return -1;
> }
> } else {
> @@ -916,7 +925,6 @@ free_fail:
> vhostfdname, vnet_hdr, fd, &err);
> if (err) {
> error_propagate(errp, err);
> - close(fd);
> return -1;
> }
> }
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [PATCH] tap: close fd conditionally when error occured
2018-01-15 7:21 ` Jason Wang
@ 2018-01-15 12:55 ` Zhoujian (jay)
0 siblings, 0 replies; 3+ messages in thread
From: Zhoujian (jay) @ 2018-01-15 12:55 UTC (permalink / raw)
To: Jason Wang, qemu-devel@nongnu.org
Cc: mst@redhat.com, imammedo@redhat.com, Huangweidong (C),
wangxin (U), Gonglei (Arei)
> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Monday, January 15, 2018 3:21 PM
> To: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org
> Cc: mst@redhat.com; imammedo@redhat.com; Huangweidong (C)
> <weidong.huang@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>; Gonglei
> (Arei) <arei.gonglei@huawei.com>
> Subject: Re: [PATCH] tap: close fd conditionally when error occured
>
>
>
> On 2018年01月12日 15:30, Jay Zhou wrote:
> > If netdev_add tap,id=net0,vhost=on failed in net_init_tap_one(), the
> > followed up device_add virtio-net-pci,netdev=net0 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().
> >
> > I think the fd should be closed in these two case:
> > 1.tap_set_sndbuf() failed
> > 2.tap_set_sndbuf() succeeded but vhost failed to initialize with
> > vhostforce flag on
> > Meanwhile, 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 <mst@redhat.com>
> > Suggested-by: Igor Mammedov <imammedo@redhat.com>
> > Suggested-by: Jason Wang <jasowang@redhat.com>
> > Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
> > ---
> > net/tap.c | 18 +++++++++++++-----
> > 1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/tap.c b/net/tap.c
> > index 979e622..3ed72eb 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -651,6 +651,9 @@ static void net_init_tap_one(const NetdevTapOptions
> *tap, NetClientState *peer,
> > tap_set_sndbuf(s->fd, tap, &err);
> > if (err) {
> > error_propagate(errp, err);
> > + if (!tap->has_fd && !tap->has_fds) {
> > + close(fd);
> > + }
> > return;
> > }
> >
> > @@ -687,14 +690,14 @@ 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);
> > - return;
> > + goto cleanup;
> > }
> > } else {
> > vhostfd = open("/dev/vhost-net", O_RDWR);
> > if (vhostfd < 0) {
> > error_setg_errno(errp, errno,
> > "tap: open vhost char device failed");
> > - return;
> > + goto cleanup;
> > }
> > fcntl(vhostfd, F_SETFL, O_NONBLOCK);
> > }
> > @@ -704,11 +707,18 @@ 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");
>
> So error_setg() is not appropriate here consider it was not trated as an
> error. We probably just need some warning.
Should it be warning too if vhostforce flag on?
>
> > - return;
> > + goto cleanup;
> > }
> > } else if (vhostfdname) {
> > error_setg(errp, "vhostfd(s)= is not valid without vhost");
> > }
> > +
> > +cleanup:
> > + if (!tap->has_fd && !tap->has_fds && tap->has_vhostforce &&
> > + tap->vhostforce) {
> > + close(fd);
>
> I would still let caller to decide whether or not to close the fd.
How about moving tap_set_sndbuf() out of net_init_tap_one()? So the caller
could decide whether or not close the fd easily(The reason is explained in
the commit message). Any better suggestion?
Regards,
Jay
>
> Thanks
>
> > + }
> > + return;
> > }
> >
> > static int get_fds(char *str, char *fds[], int max) @@ -877,7 +887,6
> > @@ free_fail:
> > vnet_hdr, fd, &err);
> > if (err) {
> > error_propagate(errp, err);
> > - close(fd);
> > return -1;
> > }
> > } else {
> > @@ -916,7 +925,6 @@ free_fail:
> > vhostfdname, vnet_hdr, fd, &err);
> > if (err) {
> > error_propagate(errp, err);
> > - close(fd);
> > return -1;
> > }
> > }
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-01-15 12:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-12 7:30 [Qemu-devel] [PATCH] tap: close fd conditionally when error occured Jay Zhou
2018-01-15 7:21 ` Jason Wang
2018-01-15 12:55 ` Zhoujian (jay)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).