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

[Jay Zhou <jianjay.zhou@huawei.com>]

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