Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to initialize

[Jason Wang <jasowang@redhat.com>]

On 2018年01月10日 12:18, Zhoujian (jay) wrote:
> Sorry about missing to cc Jason.
>
>
> Close the fd of the tap unconditionally when netdev_add tap,id=net0,vhost=on failed
> in net_init_tap_one() will make the followed up device_add virtio-net-pci,netdev=net0
> failed too, which prints:
>
>      TUNSETOFFLOAD ioctl() failed: Bad file descriptor TUNSETOFFLOAD
>      ioctl() failed: Bad file descriptor
>
> This patch makes the followed up device_add be able to fall back to userspace virtio
> when netdev_add failed with vhost turning on but vhost force flag does not set.
>
> Here is the original discussion:
> https://lists.gnu.org/archive/html/qemu-devel/2017-12/msg05205.html
>
>
> This is a RFC version, if I'm wrong, please let me know, thanks!
>
> PS: there're two places updated, see below.
>
>
>> -----Original Message-----
>> From: Zhoujian (jay)
>> Sent: Wednesday, January 10, 2018 12:40 AM
>> To: qemu-devel@nongnu.org
>> Cc: mst@redhat.com; imammedo@redhat.com; Huangweidong (C)
>> <weidong.huang@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>; Gonglei
>> (Arei) <arei.gonglei@huawei.com>; Liuzhe (Ahriy, Euler) <liuzhe13@huawei.com>;
>> Zhoujian (jay) <jianjay.zhou@huawei.com>
>> Subject: [PATCH v5 2/4][RFC] tap: do not close fd if only vhost failed to
>> initialize
>>
>> Making the followed up device_add to fall back to userspace virtio when
>> netdev_add fails if vhost force flag does not set.
>>
>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
>> ---
>>   net/tap.c | 25 ++++++++++++++++++-------
>>   1 file changed, 18 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/tap.c b/net/tap.c
>> index 979e622..03f226f 100644
>> --- a/net/tap.c
>> +++ b/net/tap.c
>> @@ -642,7 +642,8 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
>> NetClientState *peer,
>>                                const char *model, const char *name,
>>                                const char *ifname, const char *script,
>>                                const char *downscript, const char *vhostfdname,
>> -                             int vnet_hdr, int fd, Error **errp)
>> +                             int vnet_hdr, int fd, Error **errp,
>> +                             bool *vhost_init_failed)
>>   {
>>       Error *err = NULL;
>>       TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr); @@ -
>> 687,6 +688,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
>> NetClientState *peer,
>>               vhostfd = monitor_fd_param(cur_mon, vhostfdname, &err);
>>               if (vhostfd == -1) {
>>                   error_propagate(errp, err);
>> +                *vhost_init_failed = true;
>>                   return;
>>               }
>>           } else {
>> @@ -694,6 +696,7 @@ static void net_init_tap_one(const NetdevTapOptions *tap,
>> NetClientState *peer,
>>               if (vhostfd < 0) {
>>                   error_setg_errno(errp, errno,
>>                                    "tap: open vhost char device failed");
>> +                *vhost_init_failed = true;
>>                   return;
>>               }
>>               fcntl(vhostfd, F_SETFL, O_NONBLOCK); @@ -704,6 +707,7 @@ static
>> void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>>           if (!s->vhost_net) {
>>               error_setg(errp,
>>                          "vhost-net requested but could not be initialized");
>> +            *vhost_init_failed = true;

Why not simply check s->vhost_net after call net_init_tap_one()?

Thanks

>>               return;
>>           }
>>       } else if (vhostfdname) {
>> @@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>       Error *err = NULL;
>>       const char *vhostfdname;
>>       char ifname[128];
>> +    bool vhost_init_failed = false;
>>
>>       assert(netdev->type == NET_CLIENT_DRIVER_TAP);
>>       tap = &netdev->u.tap;
>> @@ -783,7 +788,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>
>>           net_init_tap_one(tap, peer, "tap", name, NULL,
>>                            script, downscript,
>> -                         vhostfdname, vnet_hdr, fd, &err);
>> +                         vhostfdname, vnet_hdr, fd, &err,
>> + &vhost_init_failed);
>>           if (err) {
>>               error_propagate(errp, err);
>>               return -1;
>> @@ -835,7 +840,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>>               net_init_tap_one(tap, peer, "tap", name, ifname,
>>                                script, downscript,
>>                                tap->has_vhostfds ? vhost_fds[i] : NULL,
>> -                             vnet_hdr, fd, &err);
>> +                             vnet_hdr, fd, &err, &vhost_init_failed);
>>               if (err) {
>>                   error_propagate(errp, err);
>>                   goto free_fail;
>> @@ -874,10 +879,12 @@ free_fail:
>>
>>           net_init_tap_one(tap, peer, "bridge", name, ifname,
>>                            script, downscript, vhostfdname,
>> -                         vnet_hdr, fd, &err);
>> +                         vnet_hdr, fd, &err, &vhost_init_failed);
>>           if (err) {
>>               error_propagate(errp, err);
>> -            close(fd);
>> +            if (tap->has_vhostforce && tap->vhostforce && vhost_init_failed)
>> {
>> +                close(fd);
>> +            }
> This should be:
>          
>          if (!vhost_init_failed || (tap->has_vhostforce && tap->vhostforce)) {
>              close(fd);
>          }
>
>>               return -1;
>>           }
>>       } else {
>> @@ -913,10 +920,14 @@ free_fail:
>>               net_init_tap_one(tap, peer, "tap", name, ifname,
>>                                i >= 1 ? "no" : script,
>>                                i >= 1 ? "no" : downscript,
>> -                             vhostfdname, vnet_hdr, fd, &err);
>> +                             vhostfdname, vnet_hdr, fd, &err,
>> +                             &vhost_init_failed);
>>               if (err) {
>>                   error_propagate(errp, err);
>> -                close(fd);
>> +                if (tap->has_vhostforce && tap->vhostforce &&
>> +                        vhost_init_failed) {
>> +                    close(fd);
>> +                }
> Same here, this should be:
>
>           if (!vhost_init_failed || (tap->has_vhostforce && tap->vhostforce)) {
>               close(fd);
>           }
>
>>                   return -1;
>>               }
>>           }
>> --
>> 1.8.3.1
>>
>