Hi Jason,
> -----Original Message-----
> From: Jason Wang [mailto:jasow...@redhat.com]
> Sent: Tuesday, February 06, 2018 5:29 PM
> To: Zhoujian (jay) <jianjay.z...@huawei.com>; 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>
> 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 <m...@redhat.com>
> > Suggested-by: Igor Mammedov <imamm...@redhat.com>
> > Suggested-by: Jason Wang <jasow...@redhat.com>
> > Signed-off-by: Jay Zhou <jianjay.z...@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;
> > }
> > }
