Re: [Qemu-devel] [PATCH v2] tap: close fd conditionally when error occured

[Jason Wang <jasowang@redhat.com>]

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
>>
>>
>>
>>
>> .
>>
>