* [Qemu-devel] [PATCH v3] tap: close fd conditionally when error occured
@ 2018-02-02 9:50 Jay Zhou
2018-02-06 9:28 ` Jason Wang
0 siblings, 1 reply; 3+ messages in thread
From: Jay Zhou @ 2018-02-02 9:50 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().
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 open or
initialize with vhostforce flag on
The fd should not be closed just because vhost failed to open or
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 | 44 ++++++++++++++++++++++++++++++++------------
1 file changed, 32 insertions(+), 12 deletions(-)
diff --git a/net/tap.c b/net/tap.c
index 979e622..c46ab14 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -651,7 +651,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
tap_set_sndbuf(s->fd, tap, &err);
if (err) {
error_propagate(errp, err);
- return;
+ goto error;
}
if (tap->has_fd || tap->has_fds) {
@@ -686,15 +686,26 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
if (vhostfdname) {
vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
if (vhostfd == -1) {
- error_propagate(errp, err);
- return;
+ if (tap->has_vhostforce && tap->vhostforce) {
+ error_propagate(errp, err);
+ goto error;
+ } else {
+ warn_report_err(err);
+ return;
+ }
}
} else {
vhostfd = open("/dev/vhost-net", O_RDWR);
if (vhostfd < 0) {
- error_setg_errno(errp, errno,
- "tap: open vhost char device failed");
- return;
+ if (tap->has_vhostforce && tap->vhostforce) {
+ error_setg_errno(errp, errno,
+ "tap: open vhost char device failed");
+ goto error;
+ } else {
+ warn_report("tap: open vhost char device failed: %s",
+ strerror(errno));
+ return;
+ }
}
fcntl(vhostfd, F_SETFL, O_NONBLOCK);
}
@@ -702,12 +713,23 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
s->vhost_net = vhost_net_init(&options);
if (!s->vhost_net) {
- error_setg(errp,
- "vhost-net requested but could not be initialized");
- return;
+ if (tap->has_vhostforce && tap->vhostforce) {
+ error_setg(errp,
+ "vhost-net requested but could not be initialized");
+ goto error;
+ } else {
+ warn_report("vhost-net requested but could not be initialized");
+ return;
+ }
}
} else if (vhostfdname) {
- error_setg(errp, "vhostfd(s)= is not valid without vhost");
+ warn_report("vhostfd(s)= is not valid without vhost");
+ return;
+ }
+
+error:
+ if (!tap->has_fd && !tap->has_fds) {
+ close(fd);
}
}
@@ -877,7 +899,6 @@ free_fail:
vnet_hdr, fd, &err);
if (err) {
error_propagate(errp, err);
- close(fd);
return -1;
}
} else {
@@ -916,7 +937,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 v3] tap: close fd conditionally when error occured
2018-02-02 9:50 [Qemu-devel] [PATCH v3] tap: close fd conditionally when error occured Jay Zhou
@ 2018-02-06 9:28 ` Jason Wang
2018-02-06 11:26 ` Zhoujian (jay)
0 siblings, 1 reply; 3+ messages in thread
From: Jason Wang @ 2018-02-06 9:28 UTC (permalink / raw)
To: Jay Zhou, qemu-devel
Cc: mst, imammedo, weidong.huang, wangxinxin.wang, arei.gonglei
On 2018年02月02日 17:50, 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().
>
> 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 open or
> initialize with vhostforce flag on
> The fd should not be closed just because vhost failed to open or
> 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 | 44 ++++++++++++++++++++++++++++++++------------
> 1 file changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/net/tap.c b/net/tap.c
> index 979e622..c46ab14 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -651,7 +651,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
> tap_set_sndbuf(s->fd, tap, &err);
> if (err) {
> error_propagate(errp, err);
> - return;
> + goto error;
> }
>
> if (tap->has_fd || tap->has_fds) {
> @@ -686,15 +686,26 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
> if (vhostfdname) {
> vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
> if (vhostfd == -1) {
> - error_propagate(errp, err);
> - return;
> + if (tap->has_vhostforce && tap->vhostforce) {
> + error_propagate(errp, err);
> + goto error;
> + } else {
> + warn_report_err(err);
> + return;
> + }
> }
> } else {
> vhostfd = open("/dev/vhost-net", O_RDWR);
> if (vhostfd < 0) {
> - error_setg_errno(errp, errno,
> - "tap: open vhost char device failed");
> - return;
> + if (tap->has_vhostforce && tap->vhostforce) {
> + error_setg_errno(errp, errno,
> + "tap: open vhost char device failed");
> + goto error;
> + } else {
> + warn_report("tap: open vhost char device failed: %s",
> + strerror(errno));
> + return;
> + }
> }
> fcntl(vhostfd, F_SETFL, O_NONBLOCK);
> }
> @@ -702,12 +713,23 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>
> s->vhost_net = vhost_net_init(&options);
> if (!s->vhost_net) {
> - error_setg(errp,
> - "vhost-net requested but could not be initialized");
> - return;
> + if (tap->has_vhostforce && tap->vhostforce) {
> + error_setg(errp,
> + "vhost-net requested but could not be initialized");
> + goto error;
> + } else {
> + warn_report("vhost-net requested but could not be initialized");
> + return;
> + }
Can we reduce the duplication by doing such in a common error label?
> }
> } else if (vhostfdname) {
> - error_setg(errp, "vhostfd(s)= is not valid without vhost");
> + warn_report("vhostfd(s)= is not valid without vhost");
> + return;
> + }
> +
> +error:
> + if (!tap->has_fd && !tap->has_fds) {
> + close(fd);
I believe you can do this in the caller by setting errp appropriately
(e.g don't set it when we only need a warn).
Thanks
> }
> }
>
> @@ -877,7 +899,6 @@ free_fail:
> vnet_hdr, fd, &err);
> if (err) {
> error_propagate(errp, err);
> - close(fd);
> return -1;
> }
> } else {
> @@ -916,7 +937,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 v3] tap: close fd conditionally when error occured
2018-02-06 9:28 ` Jason Wang
@ 2018-02-06 11:26 ` Zhoujian (jay)
0 siblings, 0 replies; 3+ messages in thread
From: Zhoujian (jay) @ 2018-02-06 11:26 UTC (permalink / raw)
To: Jason Wang, qemu-devel@nongnu.org
Cc: mst@redhat.com, imammedo@redhat.com, Huangweidong (C),
wangxin (U), Gonglei (Arei)
Hi Jason,
> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Tuesday, February 06, 2018 5:29 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 v3] tap: close fd conditionally when error occured
>
>
>
> On 2018年02月02日 17:50, 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().
> >
> > 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 open or
> > initialize with vhostforce flag on The fd should not be closed
> > just because vhost failed to open or 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 | 44 ++++++++++++++++++++++++++++++++------------
> > 1 file changed, 32 insertions(+), 12 deletions(-)
> >
> > diff --git a/net/tap.c b/net/tap.c
> > index 979e622..c46ab14 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -651,7 +651,7 @@ static void net_init_tap_one(const NetdevTapOptions
> *tap, NetClientState *peer,
> > tap_set_sndbuf(s->fd, tap, &err);
> > if (err) {
> > error_propagate(errp, err);
> > - return;
> > + goto error;
> > }
> >
> > if (tap->has_fd || tap->has_fds) { @@ -686,15 +686,26 @@ static
> > void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
> > if (vhostfdname) {
> > vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
> > if (vhostfd == -1) {
> > - error_propagate(errp, err);
> > - return;
> > + if (tap->has_vhostforce && tap->vhostforce) {
> > + error_propagate(errp, err);
> > + goto error;
> > + } else {
> > + warn_report_err(err);
> > + return;
> > + }
> > }
> > } else {
> > vhostfd = open("/dev/vhost-net", O_RDWR);
> > if (vhostfd < 0) {
> > - error_setg_errno(errp, errno,
> > - "tap: open vhost char device failed");
> > - return;
> > + if (tap->has_vhostforce && tap->vhostforce) {
> > + error_setg_errno(errp, errno,
> > + "tap: open vhost char device failed");
> > + goto error;
> > + } else {
> > + warn_report("tap: open vhost char device failed: %s",
> > + strerror(errno));
> > + return;
> > + }
> > }
> > fcntl(vhostfd, F_SETFL, O_NONBLOCK);
> > }
> > @@ -702,12 +713,23 @@ static void net_init_tap_one(const
> > NetdevTapOptions *tap, NetClientState *peer,
> >
> > s->vhost_net = vhost_net_init(&options);
> > if (!s->vhost_net) {
> > - error_setg(errp,
> > - "vhost-net requested but could not be initialized");
> > - return;
> > + if (tap->has_vhostforce && tap->vhostforce) {
> > + error_setg(errp,
> > + "vhost-net requested but could not be
> initialized");
> > + goto error;
> > + } else {
> > + warn_report("vhost-net requested but could not be
> initialized");
> > + return;
> > + }
>
> Can we reduce the duplication by doing such in a common error label?
>
Yeah, I plan to define it like this in vhost_net.h
#define VHOST_NET_INIT_FAILED \
"vhost-net requested but could not be initialized"
> > }
> > } else if (vhostfdname) {
> > - error_setg(errp, "vhostfd(s)= is not valid without vhost");
> > + warn_report("vhostfd(s)= is not valid without vhost");
> > + return;
> > + }
> > +
> > +error:
> > + if (!tap->has_fd && !tap->has_fds) {
> > + close(fd);
>
> I believe you can do this in the caller by setting errp appropriately (e.g
> don't set it when we only need a warn).
You're right, thanks! :)
Regards,
Jay
>
> Thanks
>
> > }
> > }
> >
> > @@ -877,7 +899,6 @@ free_fail:
> > vnet_hdr, fd, &err);
> > if (err) {
> > error_propagate(errp, err);
> > - close(fd);
> > return -1;
> > }
> > } else {
> > @@ -916,7 +937,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-02-06 11:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-02 9:50 [Qemu-devel] [PATCH v3] tap: close fd conditionally when error occured Jay Zhou
2018-02-06 9:28 ` Jason Wang
2018-02-06 11:26 ` 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).