| Message ID | 1517565055-14588-1-git-send-email-jianjay.zhou@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] tap: close fd conditionally when error occured | expand |
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; > } > }
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; > > } > > }
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; } }
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(-)