* [Qemu-devel] [PATCH v2] tap: close fd conditionally when error occured
@ 2018-01-26 3:08 Jay Zhou
2018-02-02 3:11 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Jay Zhou @ 2018-01-26 3:08 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 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;
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] tap: close fd conditionally when error occured
2018-01-26 3:08 [Qemu-devel] [PATCH v2] tap: close fd conditionally when error occured Jay Zhou
@ 2018-02-02 3:11 ` Jason Wang
2018-02-02 3:53 ` Jay Zhou
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2018-02-02 3:11 UTC (permalink / raw)
To: Jay Zhou, qemu-devel
Cc: weidong.huang, mst, arei.gonglei, imammedo, wangxinxin.wang
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] tap: close fd conditionally when error occured
2018-02-02 3:11 ` Jason Wang
@ 2018-02-02 3:53 ` Jay Zhou
2018-02-02 6:56 ` Jason Wang
0 siblings, 1 reply; 5+ messages in thread
From: Jay Zhou @ 2018-02-02 3:53 UTC (permalink / raw)
To: Jason Wang, qemu-devel
Cc: weidong.huang, mst, arei.gonglei, imammedo, wangxinxin.wang
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
>
>
>
>
> .
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] tap: close fd conditionally when error occured
2018-02-02 3:53 ` Jay Zhou
@ 2018-02-02 6:56 ` Jason Wang
2018-02-02 8:02 ` Jay Zhou
0 siblings, 1 reply; 5+ messages in thread
From: Jason Wang @ 2018-02-02 6:56 UTC (permalink / raw)
To: Jay Zhou, qemu-devel
Cc: weidong.huang, mst, arei.gonglei, imammedo, wangxinxin.wang
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
>>
>>
>>
>>
>> .
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2] tap: close fd conditionally when error occured
2018-02-02 6:56 ` Jason Wang
@ 2018-02-02 8:02 ` Jay Zhou
0 siblings, 0 replies; 5+ messages in thread
From: Jay Zhou @ 2018-02-02 8:02 UTC (permalink / raw)
To: Jason Wang, qemu-devel
Cc: weidong.huang, mst, arei.gonglei, imammedo, wangxinxin.wang
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-02-02 16:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-26 3:08 [Qemu-devel] [PATCH v2] tap: close fd conditionally when error occured Jay Zhou
2018-02-02 3:11 ` Jason Wang
2018-02-02 3:53 ` Jay Zhou
2018-02-02 6:56 ` Jason Wang
2018-02-02 8:02 ` Jay Zhou
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).