| Message ID | 1516936096-48900-1-git-send-email-jianjay.zhou@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] tap: close fd conditionally when error occured | expand |
On 2018年01月26日 11:08, 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 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 <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 | 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 NetdevTapOptions *tap, NetClientState *peer, > TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr); > int vhostfd; > > - 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=%d", fd); > } else if (tap->has_helper) { > @@ -781,6 +775,12 @@ int net_init_tap(const Netdev *netdev, const char *name, > > vnet_hdr = tap_probe_vnet_hdr(fd); > > + 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; > } > > + 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 = tap_probe_vnet_hdr(fd); > > + 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: > } > } > > + 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 >= 1 ? "no" : script, > i >= 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 net_tap_init_one(), and let it decide? Thanks
Hi Jason, On 2018/2/2 11:11, Jason Wang wrote: > > > On 2018年01月26日 11:08, 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 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 <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 | 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 NetdevTapOptions >> *tap, NetClientState *peer, >> TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr); >> int vhostfd; >> - 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=%d", fd); >> } else if (tap->has_helper) { >> @@ -781,6 +775,12 @@ int net_init_tap(const Netdev *netdev, const char *name, >> vnet_hdr = tap_probe_vnet_hdr(fd); >> + 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; >> } >> + 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 = tap_probe_vnet_hdr(fd); >> + 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: >> } >> } >> + 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 >= 1 ? "no" : script, >> i >= 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 net_tap_init_one(), > and let it decide? I'm a little confused here, as you suggested in version 1: https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg02933.html whether or not to close the fd should let the caller decide, so this is the version 2. If I misunderstood something, please let me know, thanks! Regards, Jay > > Thanks > > > > > . >
On 2018年02月02日 11:53, Jay Zhou wrote: > Hi Jason, > > On 2018/2/2 11:11, Jason Wang wrote: >> >> >> On 2018年01月26日 11:08, 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 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 <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 | 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 >>> NetdevTapOptions >>> *tap, NetClientState *peer, >>> TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr); >>> int vhostfd; >>> - 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=%d", >>> fd); >>> } else if (tap->has_helper) { >>> @@ -781,6 +775,12 @@ int net_init_tap(const Netdev *netdev, const >>> char *name, >>> vnet_hdr = tap_probe_vnet_hdr(fd); >>> + 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; >>> } >>> + 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 = tap_probe_vnet_hdr(fd); >>> + 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: >>> } >>> } >>> + 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 >= 1 ? "no" : script, >>> i >= 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 >> net_tap_init_one(), >> and let it decide? > > I'm a little confused here, as you suggested in version 1: > > https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg02933.html > > whether or not to close the fd should let the caller decide, so this is > the version 2. > > If I misunderstood something, please let me know, thanks! I forgot some context here, sorry. Rethink about this, looks like do it inside net_tap_init_one() is better. Just no need to modify caller and set err only when: - sndbuf set fails - vhost-net open or init fails and vhostforce set And warn when vhost fail but vhostforces does not set? Thanks > > Regards, > Jay > >> >> Thanks >> >> >> >> >> . >> >
Hi Jason, On 2018/2/2 14:56, Jason Wang wrote: > > > On 2018年02月02日 11:53, Jay Zhou wrote: >> Hi Jason, >> >> On 2018/2/2 11:11, Jason Wang wrote: >>> >>> >>> On 2018年01月26日 11:08, 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 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 <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 | 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 NetdevTapOptions >>>> *tap, NetClientState *peer, >>>> TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr); >>>> int vhostfd; >>>> - 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=%d", fd); >>>> } else if (tap->has_helper) { >>>> @@ -781,6 +775,12 @@ int net_init_tap(const Netdev *netdev, const char *name, >>>> vnet_hdr = tap_probe_vnet_hdr(fd); >>>> + 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; >>>> } >>>> + 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 = tap_probe_vnet_hdr(fd); >>>> + 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: >>>> } >>>> } >>>> + 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 >= 1 ? "no" : script, >>>> i >= 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 net_tap_init_one(), >>> and let it decide? >> >> I'm a little confused here, as you suggested in version 1: >> >> https://lists.gnu.org/archive/html/qemu-devel/2018-01/msg02933.html >> >> whether or not to close the fd should let the caller decide, so this is >> the version 2. >> >> If I misunderstood something, please let me know, thanks! > > I forgot some context here, sorry. > > Rethink about this, looks like do it inside net_tap_init_one() is better. Just > no need to modify caller and set err only when: > > - sndbuf set fails > - vhost-net open or init fails and vhostforce set > > And warn when vhost fail but vhostforces does not set? Okay, it seems reasonable and I agree with you. Will prepare for v3 soon. Regards, Jay
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 NetdevTapOptions *tap, NetClientState *peer, TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr); int vhostfd; - 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=%d", fd); } else if (tap->has_helper) { @@ -781,6 +775,12 @@ int net_init_tap(const Netdev *netdev, const char *name, vnet_hdr = tap_probe_vnet_hdr(fd); + 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; } + 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 = tap_probe_vnet_hdr(fd); + 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: } } + 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 >= 1 ? "no" : script, i >= 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; } }
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 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 <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 | 40 ++++++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-)