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

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

[Jay Zhou]

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;
             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);
+            }
             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);
+                }
                 return -1;
             }
         }
-- 
1.8.3.1

[Qemu-devel] [PATCH v5 3/4] vhost: fix memslot limit check

[Jay Zhou]

Since used_memslots will be updated to the actual value after
registering memory listener for the first time, move the
memslots limit checking to the right place.

Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
---
 hw/virtio/vhost.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e4290ce..69b3599 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1251,13 +1251,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
         goto fail;
     }
 
-    if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
-        error_report("vhost backend memory slots limit is less"
-                " than current number of present memory slots");
-        r = -1;
-        goto fail;
-    }
-
     r = hdev->vhost_ops->vhost_set_owner(hdev);
     if (r < 0) {
         VHOST_OPS_DEBUG("vhost_set_owner failed");
@@ -1339,6 +1332,18 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     hdev->memory_changed = false;
     memory_listener_register(&hdev->memory_listener, &address_space_memory);
     QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
+
+    if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
+        error_report("vhost backend memory slots limit is less"
+                " than current number of present memory slots");
+        r = -1;
+        if (busyloop_timeout) {
+            goto fail_busyloop;
+        } else {
+            goto fail;
+        }
+    }
+
     return 0;
 
 fail_busyloop:
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v5 3/4] vhost: fix memslot limit check

[Igor Mammedov]

On Wed, 10 Jan 2018 00:40:12 +0800
Jay Zhou <jianjay.zhou@huawei.com> wrote:

> Since used_memslots will be updated to the actual value after
> registering memory listener for the first time, move the
> memslots limit checking to the right place.
> 
> Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/virtio/vhost.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index e4290ce..69b3599 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1251,13 +1251,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>          goto fail;
>      }
>  
> -    if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
> -        error_report("vhost backend memory slots limit is less"
> -                " than current number of present memory slots");
> -        r = -1;
> -        goto fail;
> -    }
> -
>      r = hdev->vhost_ops->vhost_set_owner(hdev);
>      if (r < 0) {
>          VHOST_OPS_DEBUG("vhost_set_owner failed");
> @@ -1339,6 +1332,18 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>      hdev->memory_changed = false;
>      memory_listener_register(&hdev->memory_listener, &address_space_memory);
>      QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
> +
> +    if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
> +        error_report("vhost backend memory slots limit is less"
> +                " than current number of present memory slots");
> +        r = -1;
> +        if (busyloop_timeout) {
> +            goto fail_busyloop;
> +        } else {
> +            goto fail;
> +        }
> +    }
> +
>      return 0;
>  
>  fail_busyloop:

[Qemu-devel] [PATCH v5 4/4] vhost: used_memslots refactoring

[Jay Zhou]

Used_memslots is shared by vhost kernel and user, it is equal to
dev->mem->nregions, which is correct for vhost kernel, but not for
vhost user, the latter one uses memory regions that have file
descriptor. E.g. a VM has a vhost-user NIC and 8(vhost user memslot
upper limit) memory slots, it will be failed to hotplug a new DIMM
device since vhost_has_free_slot() finds no free slot left. It
should be successful if only part of memory slots have file
descriptor, so setting used memslots for vhost-user and
vhost-kernel respectively.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Jay Zhou <jianjay.zhou@huawei.com>
Signed-off-by: Liuzhe <liuzhe13@huawei.com>
---
 hw/virtio/vhost-backend.c         | 15 +++++++-
 hw/virtio/vhost-user.c            | 77 ++++++++++++++++++++++++++-------------
 hw/virtio/vhost.c                 | 13 +++----
 include/hw/virtio/vhost-backend.h |  6 ++-
 4 files changed, 75 insertions(+), 36 deletions(-)

diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
index 7f09efa..59def69 100644
--- a/hw/virtio/vhost-backend.c
+++ b/hw/virtio/vhost-backend.c
@@ -15,6 +15,8 @@
 #include "hw/virtio/vhost-backend.h"
 #include "qemu/error-report.h"
 
+static unsigned int vhost_kernel_used_memslots;
+
 static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request,
                              void *arg)
 {
@@ -62,6 +64,11 @@ static int vhost_kernel_memslots_limit(struct vhost_dev *dev)
     return limit;
 }
 
+static bool vhost_kernel_has_free_memslots(struct vhost_dev *dev)
+{
+    return vhost_kernel_used_memslots < vhost_kernel_memslots_limit(dev);
+}
+
 static int vhost_kernel_net_set_backend(struct vhost_dev *dev,
                                         struct vhost_vring_file *file)
 {
@@ -233,11 +240,16 @@ static void vhost_kernel_set_iotlb_callback(struct vhost_dev *dev,
         qemu_set_fd_handler((uintptr_t)dev->opaque, NULL, NULL, NULL);
 }
 
+static void vhost_kernel_set_used_memslots(struct vhost_dev *dev)
+{
+    vhost_kernel_used_memslots = dev->mem->nregions;
+}
+
 static const VhostOps kernel_ops = {
         .backend_type = VHOST_BACKEND_TYPE_KERNEL,
         .vhost_backend_init = vhost_kernel_init,
         .vhost_backend_cleanup = vhost_kernel_cleanup,
-        .vhost_backend_memslots_limit = vhost_kernel_memslots_limit,
+        .vhost_backend_has_free_memslots = vhost_kernel_has_free_memslots,
         .vhost_net_set_backend = vhost_kernel_net_set_backend,
         .vhost_scsi_set_endpoint = vhost_kernel_scsi_set_endpoint,
         .vhost_scsi_clear_endpoint = vhost_kernel_scsi_clear_endpoint,
@@ -264,6 +276,7 @@ static const VhostOps kernel_ops = {
 #endif /* CONFIG_VHOST_VSOCK */
         .vhost_set_iotlb_callback = vhost_kernel_set_iotlb_callback,
         .vhost_send_device_iotlb_msg = vhost_kernel_send_device_iotlb_msg,
+        .vhost_set_used_memslots = vhost_kernel_set_used_memslots,
 };
 
 int vhost_set_backend_type(struct vhost_dev *dev, VhostBackendType backend_type)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 8500562..11c7d52 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -122,6 +122,8 @@ static VhostUserMsg m __attribute__ ((unused));
 /* The version of the protocol we support */
 #define VHOST_USER_VERSION    (0x1)
 
+static bool vhost_user_free_memslots = true;
+
 struct vhost_user {
     CharBackend *chr;
     int slave_fd;
@@ -289,12 +291,43 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
     return 0;
 }
 
+static int vhost_user_prepare_msg(struct vhost_dev *dev, VhostUserMemory *mem,
+                                  int *fds)
+{
+    int i, fd;
+
+    vhost_user_free_memslots = true;
+    for (i = 0, mem->nregions = 0; i < dev->mem->nregions; ++i) {
+        struct vhost_memory_region *reg = dev->mem->regions + i;
+        ram_addr_t offset;
+        MemoryRegion *mr;
+
+        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
+        mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
+                                     &offset);
+        fd = memory_region_get_fd(mr);
+        if (fd > 0) {
+            if (mem->nregions == VHOST_MEMORY_MAX_NREGIONS) {
+                vhost_user_free_memslots = false;
+                return -1;
+            }
+
+            mem->regions[mem->nregions].userspace_addr = reg->userspace_addr;
+            mem->regions[mem->nregions].memory_size = reg->memory_size;
+            mem->regions[mem->nregions].guest_phys_addr = reg->guest_phys_addr;
+            mem->regions[mem->nregions].mmap_offset = offset;
+            fds[mem->nregions++] = fd;
+        }
+    }
+
+    return 0;
+}
+
 static int vhost_user_set_mem_table(struct vhost_dev *dev,
                                     struct vhost_memory *mem)
 {
     int fds[VHOST_MEMORY_MAX_NREGIONS];
-    int i, fd;
-    size_t fd_num = 0;
+    size_t fd_num;
     bool reply_supported = virtio_has_feature(dev->protocol_features,
                                               VHOST_USER_PROTOCOL_F_REPLY_ACK);
 
@@ -307,29 +340,12 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
         msg.flags |= VHOST_USER_NEED_REPLY_MASK;
     }
 
-    for (i = 0; i < dev->mem->nregions; ++i) {
-        struct vhost_memory_region *reg = dev->mem->regions + i;
-        ram_addr_t offset;
-        MemoryRegion *mr;
-
-        assert((uintptr_t)reg->userspace_addr == reg->userspace_addr);
-        mr = memory_region_from_host((void *)(uintptr_t)reg->userspace_addr,
-                                     &offset);
-        fd = memory_region_get_fd(mr);
-        if (fd > 0) {
-            if (fd_num == VHOST_MEMORY_MAX_NREGIONS) {
-                error_report("Failed preparing vhost-user memory table msg");
-                return -1;
-            }
-            msg.payload.memory.regions[fd_num].userspace_addr = reg->userspace_addr;
-            msg.payload.memory.regions[fd_num].memory_size  = reg->memory_size;
-            msg.payload.memory.regions[fd_num].guest_phys_addr = reg->guest_phys_addr;
-            msg.payload.memory.regions[fd_num].mmap_offset = offset;
-            fds[fd_num++] = fd;
-        }
+    if (vhost_user_prepare_msg(dev, &msg.payload.memory, fds) < 0) {
+        error_report("Failed preparing vhost-user memory table msg");
+        return -1;
     }
 
-    msg.payload.memory.nregions = fd_num;
+    fd_num = msg.payload.memory.nregions;
 
     if (!fd_num) {
         error_report("Failed initializing vhost-user memory map, "
@@ -818,9 +834,9 @@ static int vhost_user_get_vq_index(struct vhost_dev *dev, int idx)
     return idx;
 }
 
-static int vhost_user_memslots_limit(struct vhost_dev *dev)
+static bool vhost_user_has_free_memslots(struct vhost_dev *dev)
 {
-    return VHOST_MEMORY_MAX_NREGIONS;
+    return vhost_user_free_memslots;
 }
 
 static bool vhost_user_requires_shm_log(struct vhost_dev *dev)
@@ -925,11 +941,19 @@ static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
     /* No-op as the receive channel is not dedicated to IOTLB messages. */
 }
 
+static void vhost_user_set_used_memslots(struct vhost_dev *dev)
+{
+    int fds[VHOST_MEMORY_MAX_NREGIONS];
+    VhostUserMsg msg;
+
+    vhost_user_prepare_msg(dev, &msg.payload.memory, fds);
+}
+
 const VhostOps user_ops = {
         .backend_type = VHOST_BACKEND_TYPE_USER,
         .vhost_backend_init = vhost_user_init,
         .vhost_backend_cleanup = vhost_user_cleanup,
-        .vhost_backend_memslots_limit = vhost_user_memslots_limit,
+        .vhost_backend_has_free_memslots = vhost_user_has_free_memslots,
         .vhost_set_log_base = vhost_user_set_log_base,
         .vhost_set_mem_table = vhost_user_set_mem_table,
         .vhost_set_vring_addr = vhost_user_set_vring_addr,
@@ -951,4 +975,5 @@ const VhostOps user_ops = {
         .vhost_net_set_mtu = vhost_user_net_set_mtu,
         .vhost_set_iotlb_callback = vhost_user_set_iotlb_callback,
         .vhost_send_device_iotlb_msg = vhost_user_send_device_iotlb_msg,
+        .vhost_set_used_memslots = vhost_user_set_used_memslots,
 };
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 69b3599..f970ab7 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -43,20 +43,19 @@
 static struct vhost_log *vhost_log;
 static struct vhost_log *vhost_log_shm;
 
-static unsigned int used_memslots;
 static QLIST_HEAD(, vhost_dev) vhost_devices =
     QLIST_HEAD_INITIALIZER(vhost_devices);
 
 bool vhost_has_free_slot(void)
 {
-    unsigned int slots_limit = ~0U;
     struct vhost_dev *hdev;
 
     QLIST_FOREACH(hdev, &vhost_devices, entry) {
-        unsigned int r = hdev->vhost_ops->vhost_backend_memslots_limit(hdev);
-        slots_limit = MIN(slots_limit, r);
+        if (!hdev->vhost_ops->vhost_backend_has_free_memslots(hdev)) {
+            return false;
+        }
     }
-    return slots_limit > used_memslots;
+    return true;
 }
 
 static void vhost_dev_sync_region(struct vhost_dev *dev,
@@ -606,7 +605,7 @@ static void vhost_set_memory(MemoryListener *listener,
     dev->mem_changed_start_addr = MIN(dev->mem_changed_start_addr, start_addr);
     dev->mem_changed_end_addr = MAX(dev->mem_changed_end_addr, start_addr + size - 1);
     dev->memory_changed = true;
-    used_memslots = dev->mem->nregions;
+    dev->vhost_ops->vhost_set_used_memslots(dev);
 }
 
 static bool vhost_section(MemoryRegionSection *section)
@@ -1333,7 +1332,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     memory_listener_register(&hdev->memory_listener, &address_space_memory);
     QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
 
-    if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
+    if (!hdev->vhost_ops->vhost_backend_has_free_memslots(hdev)) {
         error_report("vhost backend memory slots limit is less"
                 " than current number of present memory slots");
         r = -1;
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index a7a5f22..ea01d5f 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -31,7 +31,7 @@ struct vhost_iotlb_msg;
 
 typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
 typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
-typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev);
+typedef bool (*vhost_backend_has_free_memslots)(struct vhost_dev *dev);
 
 typedef int (*vhost_net_set_backend_op)(struct vhost_dev *dev,
                                 struct vhost_vring_file *file);
@@ -84,12 +84,13 @@ typedef void (*vhost_set_iotlb_callback_op)(struct vhost_dev *dev,
                                            int enabled);
 typedef int (*vhost_send_device_iotlb_msg_op)(struct vhost_dev *dev,
                                               struct vhost_iotlb_msg *imsg);
+typedef void (*vhost_set_used_memslots_op)(struct vhost_dev *dev);
 
 typedef struct VhostOps {
     VhostBackendType backend_type;
     vhost_backend_init vhost_backend_init;
     vhost_backend_cleanup vhost_backend_cleanup;
-    vhost_backend_memslots_limit vhost_backend_memslots_limit;
+    vhost_backend_has_free_memslots vhost_backend_has_free_memslots;
     vhost_net_set_backend_op vhost_net_set_backend;
     vhost_net_set_mtu_op vhost_net_set_mtu;
     vhost_scsi_set_endpoint_op vhost_scsi_set_endpoint;
@@ -118,6 +119,7 @@ typedef struct VhostOps {
     vhost_vsock_set_running_op vhost_vsock_set_running;
     vhost_set_iotlb_callback_op vhost_set_iotlb_callback;
     vhost_send_device_iotlb_msg_op vhost_send_device_iotlb_msg;
+    vhost_set_used_memslots_op vhost_set_used_memslots;
 } VhostOps;
 
 extern const VhostOps user_ops;
-- 
1.8.3.1

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

[Zhoujian (jay)]

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

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

[Jason Wang]

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

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

[Zhoujian (jay)]

> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Wednesday, January 10, 2018 2:02 PM
> To: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org
> Cc: Huangweidong (C) <weidong.huang@huawei.com>; mst@redhat.com; wangxin (U)
> <wangxinxin.wang@huawei.com>; Gonglei (Arei) <arei.gonglei@huawei.com>;
> imammedo@redhat.com; Liuzhe (Ahriy, Euler) <liuzhe13@huawei.com>
> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only
> vhost failed to initialize
> 
> 
> 
> 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()?

s->vhost_net is always NULL if net_init_tap_one() failed, it can't distinguish
failure reasons.

fd should be closed in these two cases:
(1) tap_set_sndbuf() failed, regardless of whether having vhost or vhostforce flag
(2) with vhostforce flag

fd should not be closed if tap_set_sndbuf() succeeded and vhost init failed without
vhostforce flag.

Regards,
Jay

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

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

[Jason Wang]

On 2018年01月10日 15:39, Zhoujian (jay) wrote:
>>>> +            *vhost_init_failed = true;
>> Why not simply check s->vhost_net after call net_init_tap_one()?
> s->vhost_net is always NULL if net_init_tap_one() failed, it can't distinguish
> failure reasons.

On which condition net_init_tap_one() fail but s->vhost_net is set?

Thanks

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

[Zhoujian (jay)]

Hi Jason,

> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Thursday, January 11, 2018 11:35 AM
> To: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org
> Cc: Huangweidong (C) <weidong.huang@huawei.com>; mst@redhat.com; wangxin (U)
> <wangxinxin.wang@huawei.com>; Gonglei (Arei) <arei.gonglei@huawei.com>;
> imammedo@redhat.com; Liuzhe (Ahriy, Euler) <liuzhe13@huawei.com>
> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only
> vhost failed to initialize
> 
> 
> 
> On 2018年01月10日 15:39, Zhoujian (jay) wrote:
> >>>> +            *vhost_init_failed = true;
> >> Why not simply check s->vhost_net after call net_init_tap_one()?
> > s->vhost_net is always NULL if net_init_tap_one() failed, it can't
> distinguish
> > failure reasons.
> 
> On which condition net_init_tap_one() fail but s->vhost_net is set?

No, it does not exist, so we could not use s->vhost_net to decide
whether close the fd of tap when error occurred.


Maybe the patch below will be much better to understand, please have a look.

diff --git a/net/tap.c b/net/tap.c
index 979e622..a5c5111 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 *error_is_set_sndbuf)
 {
     Error *err = NULL;
     TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
@@ -651,6 +652,9 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
     tap_set_sndbuf(s->fd, tap, &err);
     if (err) {
         error_propagate(errp, err);
+        if (error_is_set_sndbuf) {
+            *error_is_set_sndbuf = true;
+        }
         return;
     }
 
@@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
     Error *err = NULL;
     const char *vhostfdname;
     char ifname[128];
+    bool error_is_set_sndbuf = 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, NULL);
         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, NULL);
             if (err) {
                 error_propagate(errp, err);
                 goto free_fail;
@@ -874,10 +879,13 @@ free_fail:
 
         net_init_tap_one(tap, peer, "bridge", name, ifname,
                          script, downscript, vhostfdname,
-                         vnet_hdr, fd, &err);
+                         vnet_hdr, fd, &err, &error_is_set_sndbuf);
         if (err) {
             error_propagate(errp, err);
-            close(fd);
+            if (error_is_set_sndbuf || (tap->has_vhostforce &&
+                                        tap->vhostforce)) {
+                close(fd);
+            }
             return -1;
         }
     } else {
@@ -913,10 +921,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,
+                             &error_is_set_sndbuf);
             if (err) {
                 error_propagate(errp, err);
-                close(fd);
+                if (error_is_set_sndbuf || (tap->has_vhostforce &&
+                                            tap->vhostforce)) {
+                    close(fd);
+                }
                 return -1;
             }
         }



PS: I think I do not express the meaning clearly... I can express it in
Chinese in private if necessary

Regards,
Jay

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

[Jason Wang]

On 2018年01月11日 11:54, Zhoujian (jay) wrote:
> Hi Jason,
>
>> -----Original Message-----
>> From: Jason Wang [mailto:jasowang@redhat.com]
>> Sent: Thursday, January 11, 2018 11:35 AM
>> To: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org
>> Cc: Huangweidong (C) <weidong.huang@huawei.com>; mst@redhat.com; wangxin (U)
>> <wangxinxin.wang@huawei.com>; Gonglei (Arei) <arei.gonglei@huawei.com>;
>> imammedo@redhat.com; Liuzhe (Ahriy, Euler) <liuzhe13@huawei.com>
>> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only
>> vhost failed to initialize
>>
>>
>>
>> On 2018年01月10日 15:39, Zhoujian (jay) wrote:
>>>>>> +            *vhost_init_failed = true;
>>>> Why not simply check s->vhost_net after call net_init_tap_one()?
>>> s->vhost_net is always NULL if net_init_tap_one() failed, it can't
>> distinguish
>>> failure reasons.
>> On which condition net_init_tap_one() fail but s->vhost_net is set?
> No, it does not exist, so we could not use s->vhost_net to decide
> whether close the fd of tap when error occurred.
>
>
> Maybe the patch below will be much better to understand, please have a look.
>
> diff --git a/net/tap.c b/net/tap.c
> index 979e622..a5c5111 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 *error_is_set_sndbuf)
>   {
>       Error *err = NULL;
>       TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
> @@ -651,6 +652,9 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
>       tap_set_sndbuf(s->fd, tap, &err);
>       if (err) {
>           error_propagate(errp, err);
> +        if (error_is_set_sndbuf) {
> +            *error_is_set_sndbuf = true;
> +        }
>           return;
>       }
>   
> @@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
>       Error *err = NULL;
>       const char *vhostfdname;
>       char ifname[128];
> +    bool error_is_set_sndbuf = 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, NULL);
>           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, NULL);
>               if (err) {
>                   error_propagate(errp, err);
>                   goto free_fail;
> @@ -874,10 +879,13 @@ free_fail:
>   
>           net_init_tap_one(tap, peer, "bridge", name, ifname,
>                            script, downscript, vhostfdname,
> -                         vnet_hdr, fd, &err);
> +                         vnet_hdr, fd, &err, &error_is_set_sndbuf);
>           if (err) {
>               error_propagate(errp, err);
> -            close(fd);
> +            if (error_is_set_sndbuf || (tap->has_vhostforce &&
> +                                        tap->vhostforce)) {
> +                close(fd);
> +            }
>               return -1;
>           }
>       } else {
> @@ -913,10 +921,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,
> +                             &error_is_set_sndbuf);
>               if (err) {
>                   error_propagate(errp, err);
> -                close(fd);
> +                if (error_is_set_sndbuf || (tap->has_vhostforce &&
> +                                            tap->vhostforce)) {
> +                    close(fd);
> +                }
>                   return -1;
>               }
>           }
>
>
>
> PS: I think I do not express the meaning clearly... I can express it in
> Chinese in private if necessary
>
> Regards,
> Jay

That's kind of ugly.

I would suggest do all the correct thing in net_tap_init_one().

Thanks

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

[Zhoujian (jay)]

> -----Original Message-----
> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Thursday, January 11, 2018 6:30 PM
> To: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org
> Cc: Huangweidong (C) <weidong.huang@huawei.com>; mst@redhat.com; wangxin (U)
> <wangxinxin.wang@huawei.com>; Gonglei (Arei) <arei.gonglei@huawei.com>;
> imammedo@redhat.com; Liuzhe (Ahriy, Euler) <liuzhe13@huawei.com>
> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if only
> vhost failed to initialize
> 
> 
> 
> On 2018年01月11日 11:54, Zhoujian (jay) wrote:
> > Hi Jason,
> >
> >> -----Original Message-----
> >> From: Jason Wang [mailto:jasowang@redhat.com]
> >> Sent: Thursday, January 11, 2018 11:35 AM
> >> To: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org
> >> Cc: Huangweidong (C) <weidong.huang@huawei.com>; mst@redhat.com;
> >> wangxin (U) <wangxinxin.wang@huawei.com>; Gonglei (Arei)
> >> <arei.gonglei@huawei.com>; imammedo@redhat.com; Liuzhe (Ahriy, Euler)
> >> <liuzhe13@huawei.com>
> >> Subject: Re: [Qemu-devel] [PATCH v5 2/4][RFC] tap: do not close fd if
> >> only vhost failed to initialize
> >>
> >>
> >>
> >> On 2018年01月10日 15:39, Zhoujian (jay) wrote:
> >>>>>> +            *vhost_init_failed = true;
> >>>> Why not simply check s->vhost_net after call net_init_tap_one()?
> >>> s->vhost_net is always NULL if net_init_tap_one() failed, it can't
> >> distinguish
> >>> failure reasons.
> >> On which condition net_init_tap_one() fail but s->vhost_net is set?
> > No, it does not exist, so we could not use s->vhost_net to decide
> > whether close the fd of tap when error occurred.
> >
> >
> > Maybe the patch below will be much better to understand, please have a look.
> >
> > diff --git a/net/tap.c b/net/tap.c
> > index 979e622..a5c5111 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 *error_is_set_sndbuf)
> >   {
> >       Error *err = NULL;
> >       TAPState *s = net_tap_fd_init(peer, model, name, fd, vnet_hdr);
> > @@ -651,6 +652,9 @@ static void net_init_tap_one(const NetdevTapOptions
> *tap, NetClientState *peer,
> >       tap_set_sndbuf(s->fd, tap, &err);
> >       if (err) {
> >           error_propagate(errp, err);
> > +        if (error_is_set_sndbuf) {
> > +            *error_is_set_sndbuf = true;
> > +        }
> >           return;
> >       }
> >
> > @@ -748,6 +752,7 @@ int net_init_tap(const Netdev *netdev, const char *name,
> >       Error *err = NULL;
> >       const char *vhostfdname;
> >       char ifname[128];
> > +    bool error_is_set_sndbuf = 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, NULL);
> >           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, NULL);
> >               if (err) {
> >                   error_propagate(errp, err);
> >                   goto free_fail;
> > @@ -874,10 +879,13 @@ free_fail:
> >
> >           net_init_tap_one(tap, peer, "bridge", name, ifname,
> >                            script, downscript, vhostfdname,
> > -                         vnet_hdr, fd, &err);
> > +                         vnet_hdr, fd, &err, &error_is_set_sndbuf);
> >           if (err) {
> >               error_propagate(errp, err);
> > -            close(fd);
> > +            if (error_is_set_sndbuf || (tap->has_vhostforce &&
> > +                                        tap->vhostforce)) {
> > +                close(fd);
> > +            }
> >               return -1;
> >           }
> >       } else {
> > @@ -913,10 +921,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,
> > +                             &error_is_set_sndbuf);
> >               if (err) {
> >                   error_propagate(errp, err);
> > -                close(fd);
> > +                if (error_is_set_sndbuf || (tap->has_vhostforce &&
> > +                                            tap->vhostforce)) {
> > +                    close(fd);
> > +                }
> >                   return -1;
> >               }
> >           }
> >
> >
> >
> > PS: I think I do not express the meaning clearly... I can express it
> > in Chinese in private if necessary
> >
> > Regards,
> > Jay
> 
> That's kind of ugly.

Oops...

> 
> I would suggest do all the correct thing in net_tap_init_one().

Yes, you're right, we could do it better, :)

How about this one(I have done some tests, it works):

---
diff --git a/net/tap.c b/net/tap.c
index 979e622..3ed72eb 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -651,6 +651,9 @@ static void net_init_tap_one(const NetdevTapOptions *tap, NetClientState *peer,
     tap_set_sndbuf(s->fd, tap, &err);
     if (err) {
         error_propagate(errp, err);
+        if (!tap->has_fd && !tap->has_fds) {
+            close(fd);
+        }
         return;
     }
 
@@ -687,14 +690,14 @@ 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);
-                return;
+                goto cleanup;
             }
         } else {
             vhostfd = open("/dev/vhost-net", O_RDWR);
             if (vhostfd < 0) {
                 error_setg_errno(errp, errno,
                                  "tap: open vhost char device failed");
-                return;
+                goto cleanup;
             }
             fcntl(vhostfd, F_SETFL, O_NONBLOCK);
         }
@@ -704,11 +707,18 @@ 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");
-            return;
+            goto cleanup;
         }
     } else if (vhostfdname) {
         error_setg(errp, "vhostfd(s)= is not valid without vhost");
     }
+
+cleanup:
+    if (!tap->has_fd && !tap->has_fds && tap->has_vhostforce &&
+        tap->vhostforce) {
+        close(fd);
+    }
+    return;
 }
 
 static int get_fds(char *str, char *fds[], int max)
@@ -877,7 +887,6 @@ free_fail:
                          vnet_hdr, fd, &err);
         if (err) {
             error_propagate(errp, err);
-            close(fd);
             return -1;
         }
     } else {
@@ -916,7 +925,6 @@ free_fail:
                              vhostfdname, vnet_hdr, fd, &err);
             if (err) {
                 error_propagate(errp, err);
-                close(fd);
                 return -1;
             }
         }
---

> 
> Thanks