[Qemu-devel] [PATCH v2 00/23] vhost-user reconnect fixes

[Qemu-devel] [PATCH v2 00/23] vhost-user reconnect fixes

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Hi,

Since 'vhost-user: simple reconnection support' was merged, it is
possible to disconnect and reconnect a vhost-user backend. However,
many code paths in qemu may trigger assert() when the backend is
disconnected.

Some assert() could simply be replaced by error_report() or silently
fail since they are recoverable cases. Some missing error checks can
also help prevent later issues. In many cases, the code assumes
get_vhost_net() will be non-NULL after a succesful connection, so I
changed it to stay after a disconnect until the new connection comes
(as suggested by Michael). There are also code paths that are wrong,
see "don't assume opaque is a fd" patch for an example.

Since there is feature checks on reconnection, qemu should wait for
the initial connection feature negotiation to complete. The test added
demonstrates this.

For convenience, the series is also available on:
https://github.com/elmarco/qemu, branch vhost-user-reconnect

v1->v2:
- some patch ordering: minor fix, close(fd) fix,
  assert/fprintf->error_report, check and return error,
  vhost_dev_cleanup() fixes, keep vhost_net after a disconnect, wait
  until connection is ready
- merge read/write error checks
- do not rely on link state to check vhost-user init completed

Marc-André Lureau (23):
  misc: indentation
  vhost-user: minor simplification
  vhost: don't assume opaque is a fd, use backend cleanup
  vhost: make vhost_log_put() idempotent
  vhost: call vhost_log_put() on cleanup
  vhost: add vhost device only after all success
  vhost: make vhost_dev_cleanup() idempotent
  vhost-net: always call vhost_dev_cleanup() on failure
  vhost: fix calling vhost_dev_cleanup() after vhost_dev_init()
  vhost: change some assert() for error_report() or silent fail
  vhost: use error_report() instead of fprintf(stderr,...)
  vhost-user: check qemu_chr_fe_set_msgfds() return value
  vhost-user: check vhost_user_{read,write}() return value
  qemu-char: check socket is actually connected
  vhost-user: keep vhost_net after a disconnection
  Revert "vhost-net: do not crash if backend is not present"
  get_vhost_net() should be != null after vhost_user_init
  vhost-net: success if backend has no ops->vhost_migration_done
  vhost: add assert() to check runtime behaviour
  char: add chr_wait_connected callback
  char: add and use tcp_chr_wait_connected
  vhost-user: wait until backend init is completed
  tests: add /vhost-user/connect-fail test

 hw/net/vhost_net.c      |  19 +++-----
 hw/virtio/vhost-user.c  |  54 +++++++++++++++--------
 hw/virtio/vhost.c       | 114 +++++++++++++++++++++++++++---------------------
 include/sysemu/char.h   |   8 ++++
 net/tap.c               |   1 +
 net/vhost-user.c        |  38 +++++++++-------
 qemu-char.c             |  82 ++++++++++++++++++++++++----------
 tests/vhost-user-test.c |  37 ++++++++++++++++
 8 files changed, 235 insertions(+), 118 deletions(-)

-- 
2.9.0

[Qemu-devel] [PATCH v2 01/23] misc: indentation

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 50f4dcd..3f1fecc 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -313,7 +313,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
          * properly.
          */
         if (net->nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
-                dev->use_guest_notifier_mask = false;
+            dev->use_guest_notifier_mask = false;
         }
      }
 
-- 
2.9.0

Re: [Qemu-devel] [PATCH v2 01/23] misc: indentation

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

On 06/24/2016 07:50 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/net/vhost_net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 50f4dcd..3f1fecc 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -313,7 +313,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState *ncs,
>           * properly.
>           */
>          if (net->nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
> -                dev->use_guest_notifier_mask = false;
> +            dev->use_guest_notifier_mask = false;
>          }
>       }
>  
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH v2 02/23] vhost-user: minor simplification

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/vhost-user.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/vhost-user.c b/net/vhost-user.c
index d72ce9b..392f982 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -310,7 +310,6 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
 {
     const char *name = opaque;
     const char *driver, *netdev;
-    const char virtio_name[] = "virtio-net-";
 
     driver = qemu_opt_get(opts, "driver");
     netdev = qemu_opt_get(opts, "netdev");
@@ -320,7 +319,7 @@ static int net_vhost_check_net(void *opaque, QemuOpts *opts, Error **errp)
     }
 
     if (strcmp(netdev, name) == 0 &&
-        strncmp(driver, virtio_name, strlen(virtio_name)) != 0) {
+        !g_str_has_prefix(driver, "virtio-net-")) {
         error_setg(errp, "vhost-user requires frontend driver virtio-net-*");
         return -1;
     }
-- 
2.9.0

[Qemu-devel] [PATCH v2 03/23] vhost: don't assume opaque is a fd, use backend cleanup

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 81cc5b0..dfcd256 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -997,21 +997,19 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
 
     hdev->migration_blocker = NULL;
 
-    if (vhost_set_backend_type(hdev, backend_type) < 0) {
-        close((uintptr_t)opaque);
-        return -1;
-    }
+    r = vhost_set_backend_type(hdev, backend_type);
+    assert(r >= 0);
 
-    if (hdev->vhost_ops->vhost_backend_init(hdev, opaque) < 0) {
-        close((uintptr_t)opaque);
-        return -errno;
+    r = hdev->vhost_ops->vhost_backend_init(hdev, opaque);
+    if (r < 0) {
+        goto fail;
     }
 
     if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
         fprintf(stderr, "vhost backend memory slots limit is less"
                 " than current number of present memory slots\n");
-        close((uintptr_t)opaque);
-        return -1;
+        r = -1;
+        goto fail;
     }
     QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
 
-- 
2.9.0

[Qemu-devel] [PATCH v2 04/23] vhost: make vhost_log_put() idempotent

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Clear dev->log* when calling vhost_log_put()

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index dfcd256..82290a3 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -362,6 +362,8 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
     if (!log) {
         return;
     }
+    dev->log = NULL;
+    dev->log_size = 0;
 
     --log->refcnt;
     if (log->refcnt == 0) {
@@ -710,8 +712,6 @@ static int vhost_migration_log(MemoryListener *listener, int enable)
             return r;
         }
         vhost_log_put(dev, false);
-        dev->log = NULL;
-        dev->log_size = 0;
     } else {
         vhost_dev_log_resize(dev, vhost_get_log_size(dev));
         r = vhost_dev_set_log(dev, true);
@@ -1289,7 +1289,4 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
 
     vhost_log_put(hdev, true);
     hdev->started = false;
-    hdev->log = NULL;
-    hdev->log_size = 0;
 }
-
-- 
2.9.0

[Qemu-devel] [PATCH v2 05/23] vhost: call vhost_log_put() on cleanup

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Make sure the log is being released on cleanup and the structure
cleared.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 82290a3..ce930cb 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1096,6 +1096,7 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
     g_free(hdev->mem);
     g_free(hdev->mem_sections);
     hdev->vhost_ops->vhost_backend_cleanup(hdev);
+    vhost_log_put(hdev, false);
     QLIST_REMOVE(hdev, entry);
 }
 
-- 
2.9.0

[Qemu-devel] [PATCH v2 06/23] vhost: add vhost device only after all success

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This helps following vhost_dev_cleanup() patch to check if the device
was added before removing it from the list.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index ce930cb..9c8c9f8 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1011,7 +1011,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
         r = -1;
         goto fail;
     }
-    QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
 
     r = hdev->vhost_ops->vhost_set_owner(hdev);
     if (r < 0) {
@@ -1070,6 +1069,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     hdev->started = false;
     hdev->memory_changed = false;
     memory_listener_register(&hdev->memory_listener, &address_space_memory);
+    QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
     return 0;
 fail_vq:
     while (--i >= 0) {
-- 
2.9.0

[Qemu-devel] [PATCH v2 07/23] vhost: make vhost_dev_cleanup() idempotent

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

May be called on multiple code path, so it's easier to make it safe in
the first place.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 9c8c9f8..1fdf109 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1085,19 +1085,27 @@ fail:
 void vhost_dev_cleanup(struct vhost_dev *hdev)
 {
     int i;
+
     for (i = 0; i < hdev->nvqs; ++i) {
         vhost_virtqueue_cleanup(hdev->vqs + i);
     }
-    memory_listener_unregister(&hdev->memory_listener);
+    if (hdev->mem) {
+        /* those are only safe after successful init */
+        memory_listener_unregister(&hdev->memory_listener);
+        QLIST_REMOVE(hdev, entry);
+    }
     if (hdev->migration_blocker) {
         migrate_del_blocker(hdev->migration_blocker);
         error_free(hdev->migration_blocker);
     }
     g_free(hdev->mem);
     g_free(hdev->mem_sections);
-    hdev->vhost_ops->vhost_backend_cleanup(hdev);
+    if (hdev->vhost_ops) {
+        hdev->vhost_ops->vhost_backend_cleanup(hdev);
+    }
     vhost_log_put(hdev, false);
-    QLIST_REMOVE(hdev, entry);
+
+    memset(hdev, 0, sizeof(struct vhost_dev));
 }
 
 /* Stop processing guest IO notifications in qemu.
-- 
2.9.0

[Qemu-devel] [PATCH v2 08/23] vhost-net: always call vhost_dev_cleanup() on failure

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

vhost_dev_init() may need to be cleaned up on failure too. Just call
vhost_dev_cleanup() in all cases. But first zero-alloc the struct to
avoid the garbage.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 3f1fecc..1da05d0 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -140,7 +140,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
 {
     int r;
     bool backend_kernel = options->backend_type == VHOST_BACKEND_TYPE_KERNEL;
-    struct vhost_net *net = g_malloc(sizeof *net);
+    struct vhost_net *net = g_new0(struct vhost_net, 1);
     uint64_t features = 0;
 
     if (!options->net_backend) {
@@ -185,7 +185,6 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
             fprintf(stderr, "vhost lacks feature mask %" PRIu64
                    " for backend\n",
                    (uint64_t)(~net->dev.features & net->dev.backend_features));
-            vhost_dev_cleanup(&net->dev);
             goto fail;
         }
     }
@@ -197,7 +196,6 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
             fprintf(stderr, "vhost lacks feature mask %" PRIu64
                     " for backend\n",
                     (uint64_t)(~net->dev.features & features));
-            vhost_dev_cleanup(&net->dev);
             goto fail;
         }
     }
@@ -205,7 +203,9 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
     vhost_net_ack_features(net, features);
 
     return net;
+
 fail:
+    vhost_dev_cleanup(&net->dev);
     g_free(net);
     return NULL;
 }
-- 
2.9.0

[Qemu-devel] [PATCH v2 09/23] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init()

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

vhost_net_init() calls vhost_dev_init() and in case of failure, calls
vhost_dev_cleanup() directly. However, the structure is already
partially cleaned on error. Calling vhost_dev_cleanup() again will call
vhost_virtqueue_cleanup() on already clean queues, and causing potential
double-close. Instead, adjust dev->nvqs and simplify vhost_dev_init()
code to call vhost_virtqueue_cleanup().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 1fdf109..314e0e7 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1025,7 +1025,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     for (i = 0; i < hdev->nvqs; ++i) {
         r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
         if (r < 0) {
-            goto fail_vq;
+            hdev->nvqs = i;
+            goto fail;
         }
     }
     hdev->features = features;
@@ -1071,14 +1072,9 @@ 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);
     return 0;
-fail_vq:
-    while (--i >= 0) {
-        vhost_virtqueue_cleanup(hdev->vqs + i);
-    }
+
 fail:
-    r = -errno;
-    hdev->vhost_ops->vhost_backend_cleanup(hdev);
-    QLIST_REMOVE(hdev, entry);
+    vhost_dev_cleanup(hdev);
     return r;
 }
 
-- 
2.9.0

[Qemu-devel] [PATCH v2 10/23] vhost: change some assert() for error_report() or silent fail

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Calling a vhost operation may fail, especially with disconnectable
backends. Treat some that look harmless as recoverable errors (print
error, or ignore on error code path).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 314e0e7..832fab9 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -400,7 +400,10 @@ static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
     /* inform backend of log switching, this must be done before
        releasing the current log, to ensure no logging is lost */
     r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
-    assert(r >= 0);
+    if (r < 0) {
+        error_report("Failed to change backend log");
+    }
+
     vhost_log_put(dev, true);
     dev->log = log;
     dev->log_size = size;
@@ -567,7 +570,9 @@ static void vhost_commit(MemoryListener *listener)
 
     if (!dev->log_enabled) {
         r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
-        assert(r >= 0);
+        if (r < 0) {
+            error_report("Failed to set mem table");
+        }
         dev->memory_changed = false;
         return;
     }
@@ -580,7 +585,9 @@ static void vhost_commit(MemoryListener *listener)
         vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
     }
     r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
-    assert(r >= 0);
+    if (r < 0) {
+        error_report("Failed to set mem table");
+    }
     /* To log less, can only decrease log size after table update. */
     if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
         vhost_dev_log_resize(dev, log_size);
@@ -649,6 +656,7 @@ static int vhost_virtqueue_set_addr(struct vhost_dev *dev,
     };
     int r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr);
     if (r < 0) {
+        error_report("Failed to set vring addr");
         return -errno;
     }
     return 0;
@@ -662,12 +670,15 @@ static int vhost_dev_set_features(struct vhost_dev *dev, bool enable_log)
         features |= 0x1ULL << VHOST_F_LOG_ALL;
     }
     r = dev->vhost_ops->vhost_set_features(dev, features);
+    if (r < 0) {
+        error_report("Failed to set features");
+    }
     return r < 0 ? -errno : 0;
 }
 
 static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
 {
-    int r, t, i, idx;
+    int r, i, idx;
     r = vhost_dev_set_features(dev, enable_log);
     if (r < 0) {
         goto err_features;
@@ -684,12 +695,10 @@ static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
 err_vq:
     for (; i >= 0; --i) {
         idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
-        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
-                                     dev->log_enabled);
-        assert(t >= 0);
+        vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
+                                 dev->log_enabled);
     }
-    t = vhost_dev_set_features(dev, dev->log_enabled);
-    assert(t >= 0);
+    vhost_dev_set_features(dev, dev->log_enabled);
 err_features:
     return r;
 }
@@ -937,7 +946,6 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
         }
     }
 
-    assert (r >= 0);
     cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev, idx),
                               0, virtio_queue_get_ring_size(vdev, idx));
     cpu_physical_memory_unmap(vq->used, virtio_queue_get_used_size(vdev, idx),
@@ -1190,7 +1198,9 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
 
     file.index = hdev->vhost_ops->vhost_get_vq_index(hdev, n);
     r = hdev->vhost_ops->vhost_set_vring_call(hdev, &file);
-    assert(r >= 0);
+    if (r < 0) {
+        error_report("Failed to set vring call");
+    }
 }
 
 uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
-- 
2.9.0

[Qemu-devel] [PATCH v2 11/23] vhost: use error_report() instead of fprintf(stderr, ...)

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Let's use qemu proper error reporting API.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 832fab9..e4eb97e 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -427,11 +427,11 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
         l = vq->ring_size;
         p = cpu_physical_memory_map(vq->ring_phys, &l, 1);
         if (!p || l != vq->ring_size) {
-            fprintf(stderr, "Unable to map ring buffer for ring %d\n", i);
+            error_report("Unable to map ring buffer for ring %d", i);
             r = -ENOMEM;
         }
         if (p != vq->ring) {
-            fprintf(stderr, "Ring buffer relocated for ring %d\n", i);
+            error_report("Ring buffer relocated for ring %d", i);
             r = -EBUSY;
         }
         cpu_physical_memory_unmap(p, l, 0, 0);
@@ -928,8 +928,7 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
 
     r = dev->vhost_ops->vhost_get_vring_base(dev, &state);
     if (r < 0) {
-        fprintf(stderr, "vhost VQ %d ring restore failed: %d\n", idx, r);
-        fflush(stderr);
+        error_report("vhost VQ %d ring restore failed: %d", idx, r);
     }
     virtio_queue_set_last_avail_idx(vdev, idx, state.num);
     virtio_queue_invalidate_signalled_used(vdev, idx);
@@ -1014,8 +1013,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     }
 
     if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
-        fprintf(stderr, "vhost backend memory slots limit is less"
-                " than current number of present memory slots\n");
+        error_report("vhost backend memory slots limit is less"
+                " than current number of present memory slots");
         r = -1;
         goto fail;
     }
@@ -1122,7 +1121,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
     int i, r, e;
     if (!k->set_host_notifier) {
-        fprintf(stderr, "binding does not support host notifiers\n");
+        error_report("binding does not support host notifiers");
         r = -ENOSYS;
         goto fail;
     }
@@ -1130,7 +1129,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
     for (i = 0; i < hdev->nvqs; ++i) {
         r = k->set_host_notifier(qbus->parent, hdev->vq_index + i, true);
         if (r < 0) {
-            fprintf(stderr, "vhost VQ %d notifier binding failed: %d\n", i, -r);
+            error_report("vhost VQ %d notifier binding failed: %d", i, -r);
             goto fail_vq;
         }
     }
@@ -1140,8 +1139,7 @@ fail_vq:
     while (--i >= 0) {
         e = k->set_host_notifier(qbus->parent, hdev->vq_index + i, false);
         if (e < 0) {
-            fprintf(stderr, "vhost VQ %d notifier cleanup error: %d\n", i, -r);
-            fflush(stderr);
+            error_report("vhost VQ %d notifier cleanup error: %d", i, -r);
         }
         assert (e >= 0);
     }
@@ -1164,8 +1162,7 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
     for (i = 0; i < hdev->nvqs; ++i) {
         r = k->set_host_notifier(qbus->parent, hdev->vq_index + i, false);
         if (r < 0) {
-            fprintf(stderr, "vhost VQ %d notifier cleanup failed: %d\n", i, -r);
-            fflush(stderr);
+            error_report("vhost VQ %d notifier cleanup failed: %d", i, -r);
         }
         assert (r >= 0);
     }
-- 
2.9.0

[Qemu-devel] [PATCH v2 12/23] vhost-user: check qemu_chr_fe_set_msgfds() return value

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Check return value, and drop the unnecessary 'if' check for fd_num.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost-user.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 495e09f..5dae496 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -187,8 +187,8 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
         return 0;
     }
 
-    if (fd_num) {
-        qemu_chr_fe_set_msgfds(chr, fds, fd_num);
+    if (qemu_chr_fe_set_msgfds(chr, fds, fd_num) < 0) {
+        return -1;
     }
 
     return qemu_chr_fe_write_all(chr, (const uint8_t *) msg, size) == size ?
-- 
2.9.0

[Qemu-devel] [PATCH v2 13/23] vhost-user: check vhost_user_{read, write}() return value

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

More error checking to break code flow and report appropriate errors.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost-user.c | 50 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 5dae496..819481d 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -214,12 +214,14 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
         fds[fd_num++] = log->fd;
     }
 
-    vhost_user_write(dev, &msg, fds, fd_num);
+    if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
+        return -1;
+    }
 
     if (shmfd) {
         msg.size = 0;
         if (vhost_user_read(dev, &msg) < 0) {
-            return 0;
+            return -1;
         }
 
         if (msg.request != VHOST_USER_SET_LOG_BASE) {
@@ -275,7 +277,9 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
     msg.size += sizeof(msg.payload.memory.padding);
     msg.size += fd_num * sizeof(VhostUserMemoryRegion);
 
-    vhost_user_write(dev, &msg, fds, fd_num);
+    if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -290,7 +294,9 @@ static int vhost_user_set_vring_addr(struct vhost_dev *dev,
         .size = sizeof(msg.payload.addr),
     };
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -313,7 +319,9 @@ static int vhost_set_vring(struct vhost_dev *dev,
         .size = sizeof(msg.payload.state),
     };
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -360,10 +368,12 @@ static int vhost_user_get_vring_base(struct vhost_dev *dev,
         .size = sizeof(msg.payload.state),
     };
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     if (vhost_user_read(dev, &msg) < 0) {
-        return 0;
+        return -1;
     }
 
     if (msg.request != VHOST_USER_GET_VRING_BASE) {
@@ -401,7 +411,9 @@ static int vhost_set_vring_file(struct vhost_dev *dev,
         msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK;
     }
 
-    vhost_user_write(dev, &msg, fds, fd_num);
+    if (vhost_user_write(dev, &msg, fds, fd_num) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -427,7 +439,9 @@ static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
         .size = sizeof(msg.payload.u64),
     };
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -455,10 +469,12 @@ static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64)
         return 0;
     }
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     if (vhost_user_read(dev, &msg) < 0) {
-        return 0;
+        return -1;
     }
 
     if (msg.request != request) {
@@ -489,7 +505,9 @@ static int vhost_user_set_owner(struct vhost_dev *dev)
         .flags = VHOST_USER_VERSION,
     };
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -501,7 +519,9 @@ static int vhost_user_reset_device(struct vhost_dev *dev)
         .flags = VHOST_USER_VERSION,
     };
 
-    vhost_user_write(dev, &msg, NULL, 0);
+    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        return -1;
+    }
 
     return 0;
 }
@@ -588,7 +608,6 @@ static bool vhost_user_requires_shm_log(struct vhost_dev *dev)
 static int vhost_user_migration_done(struct vhost_dev *dev, char* mac_addr)
 {
     VhostUserMsg msg = { 0 };
-    int err;
 
     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
 
@@ -605,8 +624,7 @@ static int vhost_user_migration_done(struct vhost_dev *dev, char* mac_addr)
         memcpy((char *)&msg.payload.u64, mac_addr, 6);
         msg.size = sizeof(msg.payload.u64);
 
-        err = vhost_user_write(dev, &msg, NULL, 0);
-        return err;
+        return vhost_user_write(dev, &msg, NULL, 0);
     }
     return -1;
 }
-- 
2.9.0

[Qemu-devel] [PATCH v2 14/23] qemu-char: check socket is actually connected

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Calling qemu_chr_fe_set_msgfds() on unconnected socket can lead to crash
if s->ioc is NULL.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qemu-char.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 84f49ac..06eaba3 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2761,14 +2761,16 @@ static int tcp_set_msgfds(CharDriverState *chr, int *fds, int num)
 {
     TCPCharDriver *s = chr->opaque;
 
-    if (!qio_channel_has_feature(s->ioc,
-                                 QIO_CHANNEL_FEATURE_FD_PASS)) {
-        return -1;
-    }
     /* clear old pending fd array */
     g_free(s->write_msgfds);
     s->write_msgfds = NULL;
 
+    if (!s->connected ||
+        !qio_channel_has_feature(s->ioc,
+                                 QIO_CHANNEL_FEATURE_FD_PASS)) {
+        return -1;
+    }
+
     if (num) {
         s->write_msgfds = g_new(int, num);
         memcpy(s->write_msgfds, fds, num * sizeof(int));
-- 
2.9.0

[Qemu-devel] [PATCH v2 15/23] vhost-user: keep vhost_net after a disconnection

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Lot of code path assumes get_vhost_net() return non-null. Keep it
after a successful vhost_net_init(). vhost_net_cleanup() will clear the
vhost-dev connection-related data after vhost_user_stop().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c |  1 -
 net/tap.c          |  1 +
 net/vhost-user.c   | 21 ++++++++-------------
 3 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 1da05d0..08c7d1e 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -378,7 +378,6 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
 void vhost_net_cleanup(struct vhost_net *net)
 {
     vhost_dev_cleanup(&net->dev);
-    g_free(net);
 }
 
 int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
diff --git a/net/tap.c b/net/tap.c
index 49817c7..31726e7 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -299,6 +299,7 @@ static void tap_cleanup(NetClientState *nc)
 
     if (s->vhost_net) {
         vhost_net_cleanup(s->vhost_net);
+        g_free(s->vhost_net);
         s->vhost_net = NULL;
     }
 
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 392f982..4c7702f 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -45,11 +45,6 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc)
     return s->acked_features;
 }
 
-static int vhost_user_running(VhostUserState *s)
-{
-    return (s->vhost_net) ? 1 : 0;
-}
-
 static void vhost_user_stop(int queues, NetClientState *ncs[])
 {
     VhostUserState *s;
@@ -59,15 +54,11 @@ static void vhost_user_stop(int queues, NetClientState *ncs[])
         assert (ncs[i]->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
 
         s = DO_UPCAST(VhostUserState, nc, ncs[i]);
-        if (!vhost_user_running(s)) {
-            continue;
-        }
 
         if (s->vhost_net) {
             /* save acked features */
-            s->acked_features = vhost_net_get_acked_features(s->vhost_net);
+            s->acked_features |= vhost_net_get_acked_features(s->vhost_net);
             vhost_net_cleanup(s->vhost_net);
-            s->vhost_net = NULL;
         }
     }
 }
@@ -85,12 +76,15 @@ static int vhost_user_start(int queues, NetClientState *ncs[])
         assert (ncs[i]->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER);
 
         s = DO_UPCAST(VhostUserState, nc, ncs[i]);
-        if (vhost_user_running(s)) {
-            continue;
-        }
 
         options.net_backend = ncs[i];
         options.opaque      = s->chr;
+
+        if (s->vhost_net) {
+            vhost_net_cleanup(s->vhost_net);
+            g_free(s->vhost_net);
+        }
+
         s->vhost_net = vhost_net_init(&options);
         if (!s->vhost_net) {
             error_report("failed to init vhost_net for queue %d", i);
@@ -149,6 +143,7 @@ static void vhost_user_cleanup(NetClientState *nc)
 
     if (s->vhost_net) {
         vhost_net_cleanup(s->vhost_net);
+        g_free(s->vhost_net);
         s->vhost_net = NULL;
     }
 
-- 
2.9.0

[Qemu-devel] [PATCH v2 16/23] Revert "vhost-net: do not crash if backend is not present"

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Now that get_vhost_net() returns non-null after a successful
vhost_net_init(), we no longer need to check this case.

This reverts commit ecd34898596c60f79886061618dd7e01001113ad.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 08c7d1e..f1dd367 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -428,15 +428,10 @@ VHostNetState *get_vhost_net(NetClientState *nc)
 int vhost_set_vring_enable(NetClientState *nc, int enable)
 {
     VHostNetState *net = get_vhost_net(nc);
-    const VhostOps *vhost_ops;
+    const VhostOps *vhost_ops = net->dev.vhost_ops;
 
     nc->vring_enable = enable;
 
-    if (!net) {
-        return 0;
-    }
-
-    vhost_ops = net->dev.vhost_ops;
     if (vhost_ops->vhost_set_vring_enable) {
         return vhost_ops->vhost_set_vring_enable(&net->dev, enable);
     }
-- 
2.9.0

[Qemu-devel] [PATCH v2 17/23] get_vhost_net() should be != null after vhost_user_init

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c | 1 +
 net/vhost-user.c   | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index f1dd367..22ea653 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -417,6 +417,7 @@ VHostNetState *get_vhost_net(NetClientState *nc)
         break;
     case NET_CLIENT_OPTIONS_KIND_VHOST_USER:
         vhost_net = vhost_user_get_vhost_net(nc);
+        assert(vhost_net != NULL);
         break;
     default:
         break;
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 4c7702f..95ed2d2 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -250,6 +250,8 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
 
     qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, nc[0].name);
 
+    assert(s->vhost_net != NULL);
+
     return 0;
 }
 
-- 
2.9.0

[Qemu-devel] [PATCH v2 18/23] vhost-net: success if backend has no ops->vhost_migration_done

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 22ea653..cc2f68a 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -383,7 +383,7 @@ void vhost_net_cleanup(struct vhost_net *net)
 int vhost_net_notify_migration_done(struct vhost_net *net, char* mac_addr)
 {
     const VhostOps *vhost_ops = net->dev.vhost_ops;
-    int r = -1;
+    int r = 0;
 
     if (vhost_ops->vhost_migration_done) {
         r = vhost_ops->vhost_migration_done(&net->dev, mac_addr);
-- 
2.9.0

[Qemu-devel] [PATCH v2 19/23] vhost: add assert() to check runtime behaviour

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

All these functions must be called only after the backend is connected.
They are called from virtio-net.c, after either virtio or link status
change.

The check for nc->peer->link_down should ensure vhost_net_{start,stop}()
are always called between vhost_user_{start,stop}().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/virtio/vhost.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e4eb97e..e34bd39 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1186,6 +1186,9 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
     int r, index = n - hdev->vq_index;
     struct vhost_vring_file file;
 
+    /* should only be called after backend is connected */
+    assert(hdev->vhost_ops != NULL);
+
     if (mask) {
         assert(vdev->use_guest_notifier_mask);
         file.fd = event_notifier_get_fd(&hdev->vqs[index].masked_notifier);
@@ -1232,6 +1235,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
 {
     int i, r;
 
+    /* should only be called after backend is connected */
+    assert(hdev->vhost_ops != NULL);
+
     hdev->started = true;
 
     r = vhost_dev_set_features(hdev, hdev->log_enabled);
@@ -1292,6 +1298,9 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
 {
     int i;
 
+    /* should only be called after backend is connected */
+    assert(hdev->vhost_ops != NULL);
+
     for (i = 0; i < hdev->nvqs; ++i) {
         vhost_virtqueue_stop(hdev,
                              vdev,
-- 
2.9.0

[Qemu-devel] [PATCH v2 20/23] char: add chr_wait_connected callback

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

A function to wait on the backend to be connected, to be used in the
following patches.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/sysemu/char.h | 8 ++++++++
 qemu-char.c           | 9 +++++++++
 2 files changed, 17 insertions(+)

diff --git a/include/sysemu/char.h b/include/sysemu/char.h
index 1eb2d0f..0e3c35f 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -70,6 +70,7 @@ struct CharDriverState {
     int (*get_msgfds)(struct CharDriverState *s, int* fds, int num);
     int (*set_msgfds)(struct CharDriverState *s, int *fds, int num);
     int (*chr_add_client)(struct CharDriverState *chr, int fd);
+    int (*chr_wait_connected)(struct CharDriverState *chr, Error **errp);
     IOEventHandler *chr_event;
     IOCanReadHandler *chr_can_read;
     IOReadHandler *chr_read;
@@ -151,6 +152,13 @@ CharDriverState *qemu_chr_new(const char *label, const char *filename,
  */
 void qemu_chr_disconnect(CharDriverState *chr);
 
+/**
+ * @qemu_chr_wait_connected:
+ *
+ * Wait for characted backend to be connected.
+ */
+int qemu_chr_wait_connected(CharDriverState *chr, Error **errp);
+
 /**
  * @qemu_chr_new_noreplay:
  *
diff --git a/qemu-char.c b/qemu-char.c
index 06eaba3..a0e7736 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3140,6 +3140,15 @@ static gboolean tcp_chr_accept(QIOChannel *channel,
     return TRUE;
 }
 
+int qemu_chr_wait_connected(CharDriverState *chr, Error **errp)
+{
+    if (chr->chr_wait_connected) {
+        return chr->chr_wait_connected(chr, errp);
+    }
+
+    return 0;
+}
+
 static void tcp_chr_close(CharDriverState *chr)
 {
     TCPCharDriver *s = chr->opaque;
-- 
2.9.0

[Qemu-devel] [PATCH v2 21/23] char: add and use tcp_chr_wait_connected

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Add a chr_wait_connected for the tcp backend, and use it in the
open_socket() function.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qemu-char.c | 63 ++++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 44 insertions(+), 19 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index a0e7736..dca859d 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3140,6 +3140,32 @@ static gboolean tcp_chr_accept(QIOChannel *channel,
     return TRUE;
 }
 
+static int tcp_chr_wait_connected(CharDriverState *chr, Error **errp)
+{
+    TCPCharDriver *s = chr->opaque;
+    QIOChannelSocket *sioc;
+
+    while (!s->connected) {
+        if (s->is_listen) {
+            fprintf(stderr, "QEMU waiting for connection on: %s\n",
+                    chr->filename);
+            qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), true, NULL);
+            tcp_chr_accept(QIO_CHANNEL(s->listen_ioc), G_IO_IN, chr);
+            qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), false, NULL);
+        } else {
+            sioc = qio_channel_socket_new();
+            if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
+                object_unref(OBJECT(sioc));
+                return -1;
+            }
+            tcp_chr_new_client(chr, sioc);
+            object_unref(OBJECT(sioc));
+        }
+    }
+
+    return 0;
+}
+
 int qemu_chr_wait_connected(CharDriverState *chr, Error **errp)
 {
     if (chr->chr_wait_connected) {
@@ -4403,6 +4429,7 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
     qapi_copy_SocketAddress(&s->addr, sock->addr);
 
     chr->opaque = s;
+    chr->chr_wait_connected = tcp_chr_wait_connected;
     chr->chr_write = tcp_chr_write;
     chr->chr_sync_read = tcp_chr_sync_read;
     chr->chr_close = tcp_chr_close;
@@ -4426,32 +4453,30 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
         s->reconnect_time = reconnect;
     }
 
-    sioc = qio_channel_socket_new();
     if (s->reconnect_time) {
+        sioc = qio_channel_socket_new();
         qio_channel_socket_connect_async(sioc, s->addr,
                                          qemu_chr_socket_connected,
                                          chr, NULL);
-    } else if (s->is_listen) {
-        if (qio_channel_socket_listen_sync(sioc, s->addr, errp) < 0) {
-            goto error;
-        }
-        s->listen_ioc = sioc;
-        if (is_waitconnect) {
-            fprintf(stderr, "QEMU waiting for connection on: %s\n",
-                    chr->filename);
-            tcp_chr_accept(QIO_CHANNEL(s->listen_ioc), G_IO_IN, chr);
-        }
-        qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), false, NULL);
-        if (!s->ioc) {
-            s->listen_tag = qio_channel_add_watch(
-                QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
-        }
     } else {
-        if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
+        if (s->is_listen) {
+            sioc = qio_channel_socket_new();
+            if (qio_channel_socket_listen_sync(sioc, s->addr, errp) < 0) {
+                goto error;
+            }
+            s->listen_ioc = sioc;
+            if (is_waitconnect &&
+                qemu_chr_wait_connected(chr, errp) < 0) {
+                goto error;
+            }
+            if (!s->ioc) {
+                s->listen_tag = qio_channel_add_watch(
+                    QIO_CHANNEL(s->listen_ioc), G_IO_IN,
+                    tcp_chr_accept, chr, NULL);
+            }
+        } else if (qemu_chr_wait_connected(chr, errp) < 0) {
             goto error;
         }
-        tcp_chr_new_client(chr, sioc);
-        object_unref(OBJECT(sioc));
     }
 
     return chr;
-- 
2.9.0

[Qemu-devel] [PATCH v2 22/23] vhost-user: wait until backend init is completed

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

The chardev waits for an initial connection before starting qemu,
vhost-user wants the backend negotiation to be completed. vhost-user is
started in the net_vhost_user_event callback, which is synchronously
called after the socket is connected.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 net/vhost-user.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/net/vhost-user.c b/net/vhost-user.c
index 95ed2d2..4badd9e 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -24,6 +24,7 @@ typedef struct VhostUserState {
     VHostNetState *vhost_net;
     int watch;
     uint64_t acked_features;
+    bool started;
 } VhostUserState;
 
 typedef struct VhostUserChardevProps {
@@ -211,6 +212,7 @@ static void net_vhost_user_event(void *opaque, int event)
             return;
         }
         qmp_set_link(name, true, &err);
+        s->started = true;
         break;
     case CHR_EVENT_CLOSED:
         qmp_set_link(name, false, &err);
@@ -248,7 +250,15 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
         s->chr = chr;
     }
 
-    qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, nc[0].name);
+    do {
+        Error *err = NULL;
+        if (qemu_chr_wait_connected(chr, &err) < 0) {
+            error_report_err(err);
+            return -1;
+        }
+        qemu_chr_add_handlers(chr, NULL, NULL,
+                              net_vhost_user_event, nc[0].name);
+    } while (!s->started);
 
     assert(s->vhost_net != NULL);
 
-- 
2.9.0

Re: [Qemu-devel] [PATCH v2 22/23] vhost-user: wait until backend init is completed

[Yuanhan Liu]

On Fri, Jun 24, 2016 at 03:51:09PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> The chardev waits for an initial connection before starting qemu,
> vhost-user wants the backend negotiation to be completed. vhost-user is
> started in the net_vhost_user_event callback, which is synchronously
> called after the socket is connected.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  net/vhost-user.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 95ed2d2..4badd9e 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -24,6 +24,7 @@ typedef struct VhostUserState {
>      VHostNetState *vhost_net;
>      int watch;
>      uint64_t acked_features;
> +    bool started;
>  } VhostUserState;
>  
>  typedef struct VhostUserChardevProps {
> @@ -211,6 +212,7 @@ static void net_vhost_user_event(void *opaque, int event)
>              return;
>          }
>          qmp_set_link(name, true, &err);
> +        s->started = true;
>          break;
>      case CHR_EVENT_CLOSED:
>          qmp_set_link(name, false, &err);
> @@ -248,7 +250,15 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
>          s->chr = chr;
>      }
>  
> -    qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, nc[0].name);
> +    do {
> +        Error *err = NULL;
> +        if (qemu_chr_wait_connected(chr, &err) < 0) {
> +            error_report_err(err);
> +            return -1;
> +        }
> +        qemu_chr_add_handlers(chr, NULL, NULL,
> +                              net_vhost_user_event, nc[0].name);
> +    } while (!s->started);


I haven't looked at your patchset carefully yet, but I did a quick test,
and showed that above code piece just breaks vhost-user: it's a dead loop
that vhost-user net will be initiated again and again.

The dead loop is due to, we check "s->started" corresponding to last nc,
while we set "s->started" corresponding to the first nc.

	--yliu

Re: [Qemu-devel] [PATCH v2 22/23] vhost-user: wait until backend init is completed

[Marc-André Lureau]

Hi

----- Original Message -----
> On Fri, Jun 24, 2016 at 03:51:09PM +0200, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > The chardev waits for an initial connection before starting qemu,
> > vhost-user wants the backend negotiation to be completed. vhost-user is
> > started in the net_vhost_user_event callback, which is synchronously
> > called after the socket is connected.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  net/vhost-user.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > index 95ed2d2..4badd9e 100644
> > --- a/net/vhost-user.c
> > +++ b/net/vhost-user.c
> > @@ -24,6 +24,7 @@ typedef struct VhostUserState {
> >      VHostNetState *vhost_net;
> >      int watch;
> >      uint64_t acked_features;
> > +    bool started;
> >  } VhostUserState;
> >  
> >  typedef struct VhostUserChardevProps {
> > @@ -211,6 +212,7 @@ static void net_vhost_user_event(void *opaque, int
> > event)
> >              return;
> >          }
> >          qmp_set_link(name, true, &err);
> > +        s->started = true;
> >          break;
> >      case CHR_EVENT_CLOSED:
> >          qmp_set_link(name, false, &err);
> > @@ -248,7 +250,15 @@ static int net_vhost_user_init(NetClientState *peer,
> > const char *device,
> >          s->chr = chr;
> >      }
> >  
> > -    qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event,
> > nc[0].name);
> > +    do {
> > +        Error *err = NULL;
> > +        if (qemu_chr_wait_connected(chr, &err) < 0) {
> > +            error_report_err(err);
> > +            return -1;
> > +        }
> > +        qemu_chr_add_handlers(chr, NULL, NULL,
> > +                              net_vhost_user_event, nc[0].name);
> > +    } while (!s->started);
> 
> 
> I haven't looked at your patchset carefully yet, but I did a quick test,
> and showed that above code piece just breaks vhost-user: it's a dead loop
> that vhost-user net will be initiated again and again.

Interesting, thanks a lot for testing!

The vhost-user-test works just fine, as well as vhost-user-bridge.

> The dead loop is due to, we check "s->started" corresponding to last nc,
> while we set "s->started" corresponding to the first nc.

I see, that makes sense with multiqueue, I'll update the code. It would be really nice to have a basic multiqueue test in vhost-user-test.

thanks

Re: [Qemu-devel] [PATCH v2 22/23] vhost-user: wait until backend init is completed

[Yuanhan Liu]

On Thu, Jun 30, 2016 at 05:22:43AM -0400, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > On Fri, Jun 24, 2016 at 03:51:09PM +0200, marcandre.lureau@redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > 
> > > The chardev waits for an initial connection before starting qemu,
> > > vhost-user wants the backend negotiation to be completed. vhost-user is
> > > started in the net_vhost_user_event callback, which is synchronously
> > > called after the socket is connected.
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  net/vhost-user.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > > index 95ed2d2..4badd9e 100644
> > > --- a/net/vhost-user.c
> > > +++ b/net/vhost-user.c
> > > @@ -24,6 +24,7 @@ typedef struct VhostUserState {
> > >      VHostNetState *vhost_net;
> > >      int watch;
> > >      uint64_t acked_features;
> > > +    bool started;
> > >  } VhostUserState;
> > >  
> > >  typedef struct VhostUserChardevProps {
> > > @@ -211,6 +212,7 @@ static void net_vhost_user_event(void *opaque, int
> > > event)
> > >              return;
> > >          }
> > >          qmp_set_link(name, true, &err);
> > > +        s->started = true;
> > >          break;
> > >      case CHR_EVENT_CLOSED:
> > >          qmp_set_link(name, false, &err);
> > > @@ -248,7 +250,15 @@ static int net_vhost_user_init(NetClientState *peer,
> > > const char *device,
> > >          s->chr = chr;
> > >      }
> > >  
> > > -    qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event,
> > > nc[0].name);
> > > +    do {
> > > +        Error *err = NULL;
> > > +        if (qemu_chr_wait_connected(chr, &err) < 0) {
> > > +            error_report_err(err);
> > > +            return -1;
> > > +        }
> > > +        qemu_chr_add_handlers(chr, NULL, NULL,
> > > +                              net_vhost_user_event, nc[0].name);
> > > +    } while (!s->started);
> > 
> > 
> > I haven't looked at your patchset carefully yet, but I did a quick test,
> > and showed that above code piece just breaks vhost-user: it's a dead loop
> > that vhost-user net will be initiated again and again.
> 
> Interesting, thanks a lot for testing!
> 
> The vhost-user-test works just fine, as well as vhost-user-bridge.
> 
> > The dead loop is due to, we check "s->started" corresponding to last nc,
> > while we set "s->started" corresponding to the first nc.
> 
> I see, that makes sense with multiqueue, I'll update the code. It would be really nice to have a basic multiqueue test in vhost-user-test.

My bad. It was actually my task to add the multiqueue test: it was
postponed, or even forgotten :(. If you don't mind, feel free to
add it. If you have not time for that, I will spend some time to
fix what I left.

	--yliu

[Qemu-devel] [PATCH v2 23/23] tests: add /vhost-user/connect-fail test

[marcandre.lureau]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Check early connection failure and resume.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/vhost-user-test.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 8b2164b..7fe12e6 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -131,6 +131,7 @@ typedef struct TestServer {
     GCond data_cond;
     int log_fd;
     uint64_t rings;
+    bool test_fail;
 } TestServer;
 
 #if !GLIB_CHECK_VERSION(2, 32, 0)
@@ -234,6 +235,12 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
     uint8_t *p = (uint8_t *) &msg;
     int fd;
 
+    if (s->test_fail) {
+        qemu_chr_disconnect(chr);
+        /* now switch to non-failure */
+        s->test_fail = false;
+    }
+
     if (size != VHOST_USER_HDR_SIZE) {
         g_test_message("Wrong message size received %d\n", size);
         return;
@@ -697,6 +704,33 @@ static void test_reconnect(void)
     g_test_trap_subprocess(path, 0, 0);
     g_test_trap_assert_passed();
 }
+
+static void test_connect_fail_subprocess(void)
+{
+    TestServer *s = test_server_new("reconnect");
+    char *cmd;
+
+    s->test_fail = true;
+    g_thread_new("connect", connect_thread, s);
+    cmd = GET_QEMU_CMDE(s, 2, ",server", "");
+    qtest_start(cmd);
+    g_free(cmd);
+
+    wait_for_fds(s);
+    wait_for_rings_started(s, 2);
+
+    qtest_end();
+    test_server_free(s);
+}
+
+static void test_connect_fail(void)
+{
+    gchar *path = g_strdup_printf("/%s/vhost-user/connect-fail/subprocess",
+                                  qtest_get_arch());
+    g_test_trap_subprocess(path, 0, 0);
+    g_test_trap_assert_passed();
+}
+
 #endif
 
 int main(int argc, char **argv)
@@ -747,6 +781,9 @@ int main(int argc, char **argv)
     qtest_add_func("/vhost-user/reconnect/subprocess",
                    test_reconnect_subprocess);
     qtest_add_func("/vhost-user/reconnect", test_reconnect);
+    qtest_add_func("/vhost-user/connect-fail/subprocess",
+                   test_connect_fail_subprocess);
+    qtest_add_func("/vhost-user/connect-fail", test_connect_fail);
 #endif
 
     ret = g_test_run();
-- 
2.9.0