[Qemu-devel] [PATCH v5 00/31] vhost-user reconnect fixes

[Qemu-devel] [PATCH v5 00/31] vhost-user reconnect fixes

[marcandre.lureau]

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

Hi,

Since 'vhost-user: simple reconnection support' has been 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.

There are also code paths that are wrong, see "don't assume opaque is
a fd" patch for an example. Once those patches are reviewed & merged,
they are good candidates for stable too.

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. The errors are reported up to vhost.c,
as the vhost-user backend alone doesn't handle disconnected state
transparently so far. There are still problematic code paths left
after this series, for example, starting a migration with a
disconnected backend will abort(). It is likely that other problematic
code path exists (vhost_scsi_start failure is fatal, but there are no
vhost-user backend that I know yet).

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

Since there is feature checks on reconnection, qemu should wait for
the initial connection feature negotiation to complete. The test added
demonstrates this. Additionally, a regression was found during v2,
which could have been spotted with a multiqueue test, so add a basic
one that would have exhibited the issue.

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

v5:
- rebased
- use a VHOST_OPS_DEBUG macro to print vhost_ops errors
- replace assert(foo != NULL) with assert(foo)
- add "RFC: vhost: do not update last avail idx"

v4:
- change notify_migration_done() patch to be VHOST_BACKEND_TYPE_USER
  specific, to avoid having to handle the case where the backend
  doesn't implement the callback
- change vhost_dev_cleanup() to assert on empty log, instead of
  adding a call to vhost_log_put()
- made "keep vhost_net after a disconnection" more robust, got rid of
  the acked_features field
- improve commit log, and some patch reorganization for clarity

v3:
- add vhost-user multiqueue test, which would have helped to find the
  following fix
- fix waiting on vhost-user connection with multiqueue (found by
  Yuanhan Liu)
- merge vhost_user_{read,write}() error checking patches
- add error reporting to vhost_user_read() (similar to
  vhost_user_write())
- add a vhost_net_set_backend() wrapper to help with potential crash
- some leak fixes

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 (31):
  misc: indentation
  vhost-user: minor simplification
  vhost: don't assume opaque is a fd, use backend cleanup
  vhost: make vhost_log_put() idempotent
  vhost: assert the log was cleaned up
  vhost: fix cleanup on not fully initialized device
  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: do not assert() on vhost_ops failure
  vhost: add missing VHOST_OPS_DEBUG
  vhost: use error_report() instead of fprintf(stderr,...)
  qemu-char: fix qemu_chr_fe_set_msgfds() crash when disconnected
  vhost-user: call set_msgfds unconditionally
  vhost-user: check qemu_chr_fe_set_msgfds() return value
  vhost-user: check vhost_user_{read,write}() return value
  vhost-user: keep vhost_net after a disconnection
  vhost-user: add get_vhost_net() assertions
  Revert "vhost-net: do not crash if backend is not present"
  vhost-net: vhost_migration_done is vhost-user specific
  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: plug some leaks in virtio-net-test
  tests: fix vhost-user-test leak
  tests: add /vhost-user/connect-fail test
  tests: add a simple /vhost-user/multiqueue test
  vhost-user: add error report in vhost_user_write()
  vhost: add vhost_net_set_backend()
  RFC: vhost: do not update last avail idx on get_vring_base() failure

 hw/net/vhost_net.c        |  34 ++++------
 hw/virtio/vhost-user.c    |  67 ++++++++++++++------
 hw/virtio/vhost.c         | 158 ++++++++++++++++++++++++++++++----------------
 include/hw/virtio/vhost.h |   4 ++
 include/sysemu/char.h     |   8 +++
 net/tap.c                 |   1 +
 net/vhost-user.c          |  59 ++++++++++-------
 qemu-char.c               |  82 +++++++++++++++++-------
 tests/Makefile.include    |   2 +-
 tests/vhost-user-test.c   | 147 +++++++++++++++++++++++++++++++++++++++++-
 tests/virtio-net-test.c   |  12 +++-
 11 files changed, 422 insertions(+), 152 deletions(-)

-- 
2.9.0

[Qemu-devel] [PATCH v5 01/31] 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 f92d3f8..3677a82 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_DRIVER_VHOST_USER) {
-                dev->use_guest_notifier_mask = false;
+            dev->use_guest_notifier_mask = false;
         }
      }
 
-- 
2.9.0

[Qemu-devel] [PATCH v5 02/31] vhost-user: minor simplification

[marcandre.lureau]

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

Shorten the code and make it more clear by using the specialized
function g_str_has_prefix().

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 c4d63e0..2af212b 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -316,7 +316,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");
@@ -326,7 +325,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 v5 03/31] vhost: don't assume opaque is a fd, use backend cleanup

[marcandre.lureau]

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

vhost-dev opaque isn't necessarily an fd, it can be a chardev when using
vhost-user. Goto fail, so vhost_backend_cleanup() is called to handle
backend cleanup appropriately.

vhost_set_backend_type() should never fail, use an assert().

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 ec3abda..429499a 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1019,21 +1019,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 v5 04/31] vhost: make vhost_log_put() idempotent

[marcandre.lureau]

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

Although not strictly required, it is nice to have vhost_log_put()
safely callable multiple times.

Clear dev->log* when calling vhost_log_put() to make the function
idempotent. This also simplifies a bit the caller work.

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 429499a..9bac163 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);
@@ -1328,7 +1328,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 v5 05/31] vhost: assert the log was cleaned up

[marcandre.lureau]

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

Make sure the log was released on cleanup, or it will leak (the
alternative is to call vhost_log_put() unconditionally, but it may hide
some dev state issues).

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 9bac163..8a18f9b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1134,6 +1134,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);
+    assert(!hdev->log);
     QLIST_REMOVE(hdev, entry);
 }
 
-- 
2.9.0

[Qemu-devel] [PATCH v5 06/31] vhost: fix cleanup on not fully initialized device

[marcandre.lureau]

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

If vhost_dev_init() failed, caller may still call vhost_dev_cleanup()
later. However, vhost_dev_cleanup() tries to remove the device from the
list even if it wasn't yet added, which may lead to crashes. Similarly
for the memory listener.

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 8a18f9b..6b988e1 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1033,7 +1033,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) {
@@ -1103,6 +1102,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_busyloop:
     while (--i >= 0) {
@@ -1126,7 +1126,11 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
     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);
@@ -1135,7 +1139,6 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
     g_free(hdev->mem_sections);
     hdev->vhost_ops->vhost_backend_cleanup(hdev);
     assert(!hdev->log);
-    QLIST_REMOVE(hdev, entry);
 }
 
 /* Stop processing guest IO notifications in qemu.
-- 
2.9.0

[Qemu-devel] [PATCH v5 07/31] vhost: make vhost_dev_cleanup() idempotent

[marcandre.lureau]

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

It is called on multiple code path, so make it safe to call several
times (note: I don't remember a reproducer here, but a function called
'cleanup' should probably be idempotent in my book)

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 6b988e1..9400b47 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1123,6 +1123,7 @@ fail:
 void vhost_dev_cleanup(struct vhost_dev *hdev)
 {
     int i;
+
     for (i = 0; i < hdev->nvqs; ++i) {
         vhost_virtqueue_cleanup(hdev->vqs + i);
     }
@@ -1137,8 +1138,12 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
     }
     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);
+    }
     assert(!hdev->log);
+
+    memset(hdev, 0, sizeof(struct vhost_dev));
 }
 
 /* Stop processing guest IO notifications in qemu.
-- 
2.9.0

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

[marcandre.lureau]

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

vhost_dev_init(), calling vhost backend initialization, should be
cleaned up after failure too. Call vhost_dev_cleanup() in all failure
cases. First, it needs to zero-alloc the struct to avoid the initial
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 3677a82..c11f69c 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 v5 09/31] 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 not call vhost_virtqueue_cleanup() but vhost_dev_cleanup()
instead.

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 9400b47..c61302a 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1047,7 +1047,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;
         }
     }
 
@@ -1104,19 +1105,13 @@ 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_busyloop:
     while (--i >= 0) {
         vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0);
     }
-    i = hdev->nvqs;
-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 v5 10/31] vhost: do not assert() on vhost_ops failure

[marcandre.lureau]

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

Calling a vhost operation may fail, for example with disconnected
vhost-user backend, but qemu shouldn't abort in this case.

Log an error instead, except on error and cleanup code paths where it
can be mostly ignored.

Let's use a VHOST_OPS_DEBUG macro to easily disable those messages once
disconnected backend stabilizes.

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index c61302a..324a8bf 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -27,6 +27,18 @@
 #include "hw/virtio/virtio-access.h"
 #include "migration/migration.h"
 
+/* enabled until disconnected backend stabilizes */
+#define _VHOST_DEBUG 1
+
+#ifdef _VHOST_DEBUG
+#define VHOST_OPS_DEBUG(fmt, ...) \
+    do { error_report(fmt ": %s (%d)", ## __VA_ARGS__, \
+                      strerror(errno), errno); } while (0)
+#else
+#define VHOST_OPS_DEBUG(fmt, ...) \
+    do { } while (0)
+#endif
+
 static struct vhost_log *vhost_log;
 static struct vhost_log *vhost_log_shm;
 
@@ -400,7 +412,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) {
+        VHOST_OPS_DEBUG("vhost_set_log_base failed");
+    }
+
     vhost_log_put(dev, true);
     dev->log = log;
     dev->log_size = size;
@@ -567,7 +582,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) {
+            VHOST_OPS_DEBUG("vhost_set_mem_table failed");
+        }
         dev->memory_changed = false;
         return;
     }
@@ -580,7 +597,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) {
+        VHOST_OPS_DEBUG("vhost_set_mem_table failed");
+    }
     /* 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);
@@ -667,7 +686,7 @@ static int vhost_dev_set_features(struct vhost_dev *dev, bool enable_log)
 
 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 +703,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;
 }
@@ -929,15 +946,11 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
      * native as legacy devices expect so by default.
      */
     if (vhost_needs_vring_endian(vdev)) {
-        r = vhost_virtqueue_set_vring_endian_legacy(dev,
-                                                    !virtio_is_big_endian(vdev),
-                                                    vhost_vq_index);
-        if (r < 0) {
-            error_report("failed to reset vring endianness");
-        }
+        vhost_virtqueue_set_vring_endian_legacy(dev,
+                                                !virtio_is_big_endian(vdev),
+                                                vhost_vq_index);
     }
 
-    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),
@@ -1228,7 +1241,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) {
+        VHOST_OPS_DEBUG("vhost_set_vring_call failed");
+    }
 }
 
 uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
-- 
2.9.0

[Qemu-devel] [PATCH v5 11/31] vhost: add missing VHOST_OPS_DEBUG

[marcandre.lureau]

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

Add missing VHOST_OPS_DEBUG() logs, for completeness.

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 324a8bf..a671ac8 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -668,6 +668,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) {
+        VHOST_OPS_DEBUG("vhost_set_vring_addr failed");
         return -errno;
     }
     return 0;
@@ -681,6 +682,9 @@ 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) {
+        VHOST_OPS_DEBUG("vhost_set_features failed");
+    }
     return r < 0 ? -errno : 0;
 }
 
@@ -804,6 +808,7 @@ static int vhost_virtqueue_set_vring_endian_legacy(struct vhost_dev *dev,
         return 0;
     }
 
+    VHOST_OPS_DEBUG("vhost_set_vring_endian failed");
     if (errno == ENOTTY) {
         error_report("vhost does not support cross-endian");
         return -ENOSYS;
@@ -832,12 +837,14 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
     vq->num = state.num = virtio_queue_get_num(vdev, idx);
     r = dev->vhost_ops->vhost_set_vring_num(dev, &state);
     if (r) {
+        VHOST_OPS_DEBUG("vhost_set_vring_num failed");
         return -errno;
     }
 
     state.num = virtio_queue_get_last_avail_idx(vdev, idx);
     r = dev->vhost_ops->vhost_set_vring_base(dev, &state);
     if (r) {
+        VHOST_OPS_DEBUG("vhost_set_vring_base failed");
         return -errno;
     }
 
@@ -889,6 +896,7 @@ static int vhost_virtqueue_start(struct vhost_dev *dev,
     file.fd = event_notifier_get_fd(virtio_queue_get_host_notifier(vvq));
     r = dev->vhost_ops->vhost_set_vring_kick(dev, &file);
     if (r) {
+        VHOST_OPS_DEBUG("vhost_set_vring_kick failed");
         r = -errno;
         goto fail_kick;
     }
@@ -936,8 +944,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);
+        VHOST_OPS_DEBUG("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);
@@ -989,6 +996,7 @@ static int vhost_virtqueue_set_busyloop_timeout(struct vhost_dev *dev,
 
     r = dev->vhost_ops->vhost_set_vring_busyloop_timeout(dev, &state);
     if (r) {
+        VHOST_OPS_DEBUG("vhost_set_vring_busyloop_timeout failed");
         return r;
     }
 
@@ -1010,6 +1018,7 @@ static int vhost_virtqueue_init(struct vhost_dev *dev,
     file.fd = event_notifier_get_fd(&vq->masked_notifier);
     r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
     if (r) {
+        VHOST_OPS_DEBUG("vhost_set_vring_call failed");
         r = -errno;
         goto fail_call;
     }
@@ -1049,11 +1058,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
 
     r = hdev->vhost_ops->vhost_set_owner(hdev);
     if (r < 0) {
+        VHOST_OPS_DEBUG("vhost_set_owner failed");
         goto fail;
     }
 
     r = hdev->vhost_ops->vhost_get_features(hdev, &features);
     if (r < 0) {
+        VHOST_OPS_DEBUG("vhost_get_features failed");
         goto fail;
     }
 
@@ -1286,6 +1297,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
     }
     r = hdev->vhost_ops->vhost_set_mem_table(hdev, hdev->mem);
     if (r < 0) {
+        VHOST_OPS_DEBUG("vhost_set_mem_table failed");
         r = -errno;
         goto fail_mem;
     }
@@ -1310,6 +1322,7 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
                                                 hdev->log_size ? log_base : 0,
                                                 hdev->log);
         if (r < 0) {
+            VHOST_OPS_DEBUG("vhost_set_log_base failed");
             r = -errno;
             goto fail_log;
         }
-- 
2.9.0

[Qemu-devel] [PATCH v5 12/31] 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, this ensures the error is
reported at the right place (stderr or monitor), with a conventional
format.

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index a671ac8..09428bc 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -439,11 +439,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);
@@ -1050,8 +1050,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;
     }
@@ -1174,8 +1174,9 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
     VirtioBusState *vbus = VIRTIO_BUS(qbus);
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
     int i, r, e;
+
     if (!k->ioeventfd_started) {
-        fprintf(stderr, "binding does not support host notifiers\n");
+        error_report("binding does not support host notifiers");
         r = -ENOSYS;
         goto fail;
     }
@@ -1184,7 +1185,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
         r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), 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;
         }
     }
@@ -1195,8 +1196,7 @@ fail_vq:
         e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), 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);
     }
@@ -1218,8 +1218,7 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
         r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), 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 v5 13/31] qemu-char: fix qemu_chr_fe_set_msgfds() crash when disconnected

[marcandre.lureau]

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

Calling qemu_chr_fe_set_msgfds() on unconnected socket leads to crash
since s->ioc is NULL in this case. Return an error earlier instead.

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 e4b8448..1274f50 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -2760,14 +2760,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 v5 14/31] vhost-user: call set_msgfds unconditionally

[marcandre.lureau]

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

It is fine to call set_msgfds() with 0 fd, and ensures any previous fd
array is cleared.

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

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 495e09f..f01b92f 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -187,9 +187,7 @@ 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);
-    }
+    qemu_chr_fe_set_msgfds(chr, fds, fd_num);
 
     return qemu_chr_fe_write_all(chr, (const uint8_t *) msg, size) == size ?
             0 : -1;
-- 
2.9.0

[Qemu-devel] [PATCH v5 15/31] vhost-user: check qemu_chr_fe_set_msgfds() return value

[marcandre.lureau]

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

Check qemu_chr_fe_set_msgfds() for errors, to make sure the message to
be sent is correct.

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

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index f01b92f..5dae496 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -187,7 +187,9 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
         return 0;
     }
 
-    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 ?
             0 : -1;
-- 
2.9.0

[Qemu-devel] [PATCH v5 16/31] vhost-user: check vhost_user_{read, write}() return value

[marcandre.lureau]

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

The vhost-user code is quite inconsistent with error handling. Instead
of ignoring some return values of read/write and silently going on with
invalid state (invalid read for example), break the code flow when the
error happened.

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 v5 17/31] vhost-user: keep vhost_net after a disconnection

[marcandre.lureau]

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

Many code paths assume get_vhost_net() returns non-null.

Keep VhostUserState.vhost_net after a successful vhost_net_init(),
instead of freeing it in vhost_net_cleanup().

VhostUserState.vhost_net is thus freed before after being recreated or
on final vhost_user_cleanup() and there is no need to save the acked
features.

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

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index c11f69c..7b76591 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 40a8c74..6abb962 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -312,6 +312,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 2af212b..60fab9a 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -23,7 +23,6 @@ typedef struct VhostUserState {
     CharDriverState *chr;
     VHostNetState *vhost_net;
     guint watch;
-    uint64_t acked_features;
 } VhostUserState;
 
 typedef struct VhostUserChardevProps {
@@ -42,12 +41,7 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc)
 {
     VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
     assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_USER);
-    return s->acked_features;
-}
-
-static int vhost_user_running(VhostUserState *s)
-{
-    return (s->vhost_net) ? 1 : 0;
+    return s->vhost_net ? vhost_net_get_acked_features(s->vhost_net) : 0;
 }
 
 static void vhost_user_stop(int queues, NetClientState *ncs[])
@@ -59,15 +53,9 @@ static void vhost_user_stop(int queues, NetClientState *ncs[])
         assert(ncs[i]->info->type == NET_CLIENT_DRIVER_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);
             vhost_net_cleanup(s->vhost_net);
-            s->vhost_net = NULL;
         }
     }
 }
@@ -75,6 +63,7 @@ static void vhost_user_stop(int queues, NetClientState *ncs[])
 static int vhost_user_start(int queues, NetClientState *ncs[])
 {
     VhostNetOptions options;
+    struct vhost_net *net = NULL;
     VhostUserState *s;
     int max_queues;
     int i;
@@ -85,33 +74,39 @@ static int vhost_user_start(int queues, NetClientState *ncs[])
         assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER);
 
         s = DO_UPCAST(VhostUserState, nc, ncs[i]);
-        if (vhost_user_running(s)) {
-            continue;
-        }
 
         options.net_backend = ncs[i];
         options.opaque      = s->chr;
         options.busyloop_timeout = 0;
-        s->vhost_net = vhost_net_init(&options);
-        if (!s->vhost_net) {
+        net = vhost_net_init(&options);
+        if (!net) {
             error_report("failed to init vhost_net for queue %d", i);
             goto err;
         }
 
         if (i == 0) {
-            max_queues = vhost_net_get_max_queues(s->vhost_net);
+            max_queues = vhost_net_get_max_queues(net);
             if (queues > max_queues) {
                 error_report("you are asking more queues than supported: %d",
                              max_queues);
                 goto err;
             }
         }
+
+        if (s->vhost_net) {
+            vhost_net_cleanup(s->vhost_net);
+            g_free(s->vhost_net);
+        }
+        s->vhost_net = net;
     }
 
     return 0;
 
 err:
-    vhost_user_stop(i + 1, ncs);
+    if (net) {
+        vhost_net_cleanup(net);
+    }
+    vhost_user_stop(i, ncs);
     return -1;
 }
 
@@ -150,6 +145,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;
     }
     if (s->chr) {
-- 
2.9.0

[Qemu-devel] [PATCH v5 18/31] vhost-user: add get_vhost_net() assertions

[marcandre.lureau]

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

Add a few assertions to be more explicit about the runtime behaviour
after the previous patch: get_vhost_net() is non-null after
net_vhost_user_init().

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 7b76591..4e6495e 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_DRIVER_VHOST_USER:
         vhost_net = vhost_user_get_vhost_net(nc);
+        assert(vhost_net);
         break;
     default:
         break;
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 60fab9a..5e23869 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -257,6 +257,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);
+
     return 0;
 }
 
-- 
2.9.0

[Qemu-devel] [PATCH v5 19/31] 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 4e6495e..54cf015 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -429,15 +429,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 v5 20/31] vhost-net: vhost_migration_done is vhost-user specific

[marcandre.lureau]

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

Either the callback is mandatory to implement, in which case an assert()
is more appropriate, or it's not and we can't tell much whether the
function should fail or not (given it's name, I guess it should silently
success by default). Instead, make the implementation mandatory and
vhost-user specific to be more clear about its usage.

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

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 54cf015..dd41a8e 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -383,13 +383,11 @@ 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;
 
-    if (vhost_ops->vhost_migration_done) {
-        r = vhost_ops->vhost_migration_done(&net->dev, mac_addr);
-    }
+    assert(vhost_ops->backend_type == VHOST_BACKEND_TYPE_USER);
+    assert(vhost_ops->vhost_migration_done);
 
-    return r;
+    return vhost_ops->vhost_migration_done(&net->dev, mac_addr);
 }
 
 bool vhost_net_virtqueue_pending(VHostNetState *net, int idx)
-- 
2.9.0

[Qemu-devel] [PATCH v5 21/31] 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 09428bc..970261b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1242,6 +1242,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);
+
     if (mask) {
         assert(vdev->use_guest_notifier_mask);
         file.fd = event_notifier_get_fd(&hdev->vqs[index].masked_notifier);
@@ -1288,6 +1291,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);
+
     hdev->started = true;
 
     r = vhost_dev_set_features(hdev, hdev->log_enabled);
@@ -1350,6 +1356,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);
+
     for (i = 0; i < hdev->nvqs; ++i) {
         vhost_virtqueue_stop(hdev,
                              vdev,
-- 
2.9.0

[Qemu-devel] [PATCH v5 22/31] 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 0ea9eac..ee7e554 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;
@@ -158,6 +159,13 @@ void qemu_chr_disconnect(CharDriverState *chr);
  */
 void qemu_chr_cleanup(void);
 
+/**
+ * @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 1274f50..6eba615 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3139,6 +3139,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 v5 23/31] 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 6eba615..a50b8fb 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3139,6 +3139,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) {
@@ -4402,6 +4428,7 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
     s->addr = QAPI_CLONE(SocketAddress, 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;
@@ -4425,32 +4452,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 v5 24/31] 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, and
vhost-user should wait for the backend negotiation to be completed
before starting qemu too.

vhost-user is started in the net_vhost_user_event callback, which is
synchronously called after the socket is connected. Use a
VhostUserState.started flag to indicate vhost-user init completed
successfully and qemu can be started.

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

diff --git a/net/vhost-user.c b/net/vhost-user.c
index 5e23869..4867944 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -23,6 +23,7 @@ typedef struct VhostUserState {
     CharDriverState *chr;
     VHostNetState *vhost_net;
     guint watch;
+    bool started;
 } VhostUserState;
 
 typedef struct VhostUserChardevProps {
@@ -218,6 +219,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);
@@ -236,7 +238,7 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
                                const char *name, CharDriverState *chr,
                                int queues)
 {
-    NetClientState *nc;
+    NetClientState *nc, *nc0 = NULL;
     VhostUserState *s;
     int i;
 
@@ -245,6 +247,9 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
 
     for (i = 0; i < queues; i++) {
         nc = qemu_new_net_client(&net_vhost_user_info, peer, device, name);
+        if (!nc0) {
+            nc0 = nc;
+        }
 
         snprintf(nc->info_str, sizeof(nc->info_str), "vhost-user%d to %s",
                  i, chr->label);
@@ -255,7 +260,16 @@ 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);
+    s = DO_UPCAST(VhostUserState, nc, nc0);
+    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, nc0->name);
+    } while (!s->started);
 
     assert(s->vhost_net);
 
-- 
2.9.0

[Qemu-devel] [PATCH v5 25/31] tests: plug some leaks in virtio-net-test

[marcandre.lureau]

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

Found thanks to valgrind.

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

diff --git a/tests/virtio-net-test.c b/tests/virtio-net-test.c
index a34a939..361506f 100644
--- a/tests/virtio-net-test.c
+++ b/tests/virtio-net-test.c
@@ -149,6 +149,7 @@ static void rx_stop_cont_test(const QVirtioBus *bus, QVirtioDevice *dev,
     char test[] = "TEST";
     char buffer[64];
     int len = htonl(sizeof(test));
+    QDict *rsp;
     struct iovec iov[] = {
         {
             .iov_base = &len,
@@ -165,7 +166,8 @@ static void rx_stop_cont_test(const QVirtioBus *bus, QVirtioDevice *dev,
     free_head = qvirtqueue_add(vq, req_addr, 64, true, false);
     qvirtqueue_kick(bus, dev, vq, free_head);
 
-    qmp("{ 'execute' : 'stop'}");
+    rsp = qmp("{ 'execute' : 'stop'}");
+    QDECREF(rsp);
 
     ret = iov_send(socket, iov, 2, 0, sizeof(len) + sizeof(test));
     g_assert_cmpint(ret, ==, sizeof(test) + sizeof(len));
@@ -173,8 +175,10 @@ static void rx_stop_cont_test(const QVirtioBus *bus, QVirtioDevice *dev,
     /* We could check the status, but this command is more importantly to
      * ensure the packet data gets queued in QEMU, before we do 'cont'.
      */
-    qmp("{ 'execute' : 'query-status'}");
-    qmp("{ 'execute' : 'cont'}");
+    rsp = qmp("{ 'execute' : 'query-status'}");
+    QDECREF(rsp);
+    rsp = qmp("{ 'execute' : 'cont'}");
+    QDECREF(rsp);
 
     qvirtio_wait_queue_isr(bus, dev, vq, QVIRTIO_NET_TIMEOUT_US);
     memread(req_addr + VNET_HDR_SIZE, buffer, sizeof(test));
@@ -230,8 +234,10 @@ static void pci_basic(gconstpointer data)
     /* End test */
     close(sv[0]);
     qvirtqueue_cleanup(&qvirtio_pci, &tx->vq, alloc);
+    qvirtqueue_cleanup(&qvirtio_pci, &rx->vq, alloc);
     pc_alloc_uninit(alloc);
     qvirtio_pci_device_disable(dev);
+    g_free(dev->pdev);
     g_free(dev);
     qpci_free_pc(bus);
     test_end();
-- 
2.9.0

[Qemu-devel] [PATCH v5 26/31] tests: fix vhost-user-test leak

[marcandre.lureau]

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

Spotted by valgrind.

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

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 46d0588..27b10c1 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -683,6 +683,7 @@ static void test_reconnect(void)
                                   qtest_get_arch());
     g_test_trap_subprocess(path, 0, 0);
     g_test_trap_assert_passed();
+    g_free(path);
 }
 #endif
 
-- 
2.9.0

[Qemu-devel] [PATCH v5 27/31] 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 | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 27b10c1..abcc3f2 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -131,6 +131,7 @@ typedef struct TestServer {
     CompatGCond data_cond;
     int log_fd;
     uint64_t rings;
+    bool test_fail;
 } TestServer;
 
 static const char *tmpfs;
@@ -221,6 +222,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;
@@ -685,6 +692,34 @@ static void test_reconnect(void)
     g_test_trap_assert_passed();
     g_free(path);
 }
+
+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();
+    g_free(path);
+}
+
 #endif
 
 int main(int argc, char **argv)
@@ -735,6 +770,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

[Qemu-devel] [PATCH v5 28/31] tests: add a simple /vhost-user/multiqueue test

[marcandre.lureau]

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

This test just checks that 2 virtio-net queues can be setup over
vhost-user and waits for them to be started.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 tests/Makefile.include  |   2 +-
 tests/vhost-user-test.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 107 insertions(+), 3 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index e7e50d6..fc2df39 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -617,7 +617,7 @@ tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-usb-obj-y)
 tests/usb-hcd-xhci-test$(EXESUF): tests/usb-hcd-xhci-test.o $(libqos-usb-obj-y)
 tests/pc-cpu-test$(EXESUF): tests/pc-cpu-test.o
 tests/postcopy-test$(EXESUF): tests/postcopy-test.o
-tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o $(qtest-obj-y) $(test-io-obj-y)
+tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o $(qtest-obj-y) $(test-io-obj-y) $(libqos-pc-obj-y) $(libqos-virtio-obj-y)
 tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
 tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o $(test-util-obj-y)
 tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o $(test-block-obj-y)
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index abcc3f2..0fe7cc8 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -17,6 +17,11 @@
 #include "sysemu/char.h"
 #include "sysemu/sysemu.h"
 
+#include "libqos/pci-pc.h"
+#include "libqos/virtio-pci.h"
+#include "libqos/malloc-pc.h"
+#include "hw/virtio/virtio-net.h"
+
 #include <linux/vhost.h>
 #include <sys/vfs.h>
 
@@ -46,6 +51,7 @@
 #define VHOST_MEMORY_MAX_NREGIONS    8
 
 #define VHOST_USER_F_PROTOCOL_FEATURES 30
+#define VHOST_USER_PROTOCOL_F_MQ 0
 #define VHOST_USER_PROTOCOL_F_LOG_SHMFD 1
 
 #define VHOST_LOG_PAGE 0x1000
@@ -68,6 +74,7 @@ typedef enum VhostUserRequest {
     VHOST_USER_SET_VRING_ERR = 14,
     VHOST_USER_GET_PROTOCOL_FEATURES = 15,
     VHOST_USER_SET_PROTOCOL_FEATURES = 16,
+    VHOST_USER_GET_QUEUE_NUM = 17,
     VHOST_USER_SET_VRING_ENABLE = 18,
     VHOST_USER_MAX
 } VhostUserRequest;
@@ -132,6 +139,7 @@ typedef struct TestServer {
     int log_fd;
     uint64_t rings;
     bool test_fail;
+    int queues;
 } TestServer;
 
 static const char *tmpfs;
@@ -253,6 +261,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         msg.size = sizeof(m.payload.u64);
         msg.payload.u64 = 0x1ULL << VHOST_F_LOG_ALL |
             0x1ULL << VHOST_USER_F_PROTOCOL_FEATURES;
+        if (s->queues > 1) {
+            msg.payload.u64 |= 0x1ULL << VIRTIO_NET_F_MQ;
+        }
         p = (uint8_t *) &msg;
         qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
         break;
@@ -267,6 +278,9 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         msg.flags |= VHOST_USER_REPLY_MASK;
         msg.size = sizeof(m.payload.u64);
         msg.payload.u64 = 1 << VHOST_USER_PROTOCOL_F_LOG_SHMFD;
+        if (s->queues > 1) {
+            msg.payload.u64 |= 1 << VHOST_USER_PROTOCOL_F_MQ;
+        }
         p = (uint8_t *) &msg;
         qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
         break;
@@ -279,7 +293,7 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         p = (uint8_t *) &msg;
         qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
 
-        assert(msg.payload.state.index < 2);
+        assert(msg.payload.state.index < s->queues * 2);
         s->rings &= ~(0x1ULL << msg.payload.state.index);
         break;
 
@@ -319,10 +333,18 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         break;
 
     case VHOST_USER_SET_VRING_BASE:
-        assert(msg.payload.state.index < 2);
+        assert(msg.payload.state.index < s->queues * 2);
         s->rings |= 0x1ULL << msg.payload.state.index;
         break;
 
+    case VHOST_USER_GET_QUEUE_NUM:
+        msg.flags |= VHOST_USER_REPLY_MASK;
+        msg.size = sizeof(m.payload.u64);
+        msg.payload.u64 = s->queues;
+        p = (uint8_t *) &msg;
+        qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
+        break;
+
     default:
         break;
     }
@@ -369,6 +391,7 @@ static TestServer *test_server_new(const gchar *name)
     g_cond_init(&server->data_cond);
 
     server->log_fd = -1;
+    server->queues = 1;
 
     return server;
 }
@@ -722,6 +745,86 @@ static void test_connect_fail(void)
 
 #endif
 
+static QVirtioPCIDevice *virtio_net_pci_init(QPCIBus *bus, int slot)
+{
+    QVirtioPCIDevice *dev;
+
+    dev = qvirtio_pci_device_find(bus, VIRTIO_ID_NET);
+    g_assert(dev != NULL);
+    g_assert_cmphex(dev->vdev.device_type, ==, VIRTIO_ID_NET);
+
+    qvirtio_pci_device_enable(dev);
+    qvirtio_reset(&qvirtio_pci, &dev->vdev);
+    qvirtio_set_acknowledge(&qvirtio_pci, &dev->vdev);
+    qvirtio_set_driver(&qvirtio_pci, &dev->vdev);
+
+    return dev;
+}
+
+static void driver_init(const QVirtioBus *bus, QVirtioDevice *dev)
+{
+    uint32_t features;
+
+    features = qvirtio_get_features(bus, dev);
+    features = features & ~(QVIRTIO_F_BAD_FEATURE |
+                            (1u << VIRTIO_RING_F_INDIRECT_DESC) |
+                            (1u << VIRTIO_RING_F_EVENT_IDX));
+    qvirtio_set_features(bus, dev, features);
+
+    qvirtio_set_driver_ok(bus, dev);
+}
+
+#define PCI_SLOT                0x04
+
+static void test_multiqueue(void)
+{
+    const int queues = 2;
+    TestServer *s = test_server_new("mq");
+    QVirtioPCIDevice *dev;
+    QPCIBus *bus;
+    QVirtQueuePCI *vq[queues * 2];
+    QGuestAllocator *alloc;
+    char *cmd;
+    int i;
+
+    s->queues = queues;
+    test_server_listen(s);
+
+    cmd = g_strdup_printf(QEMU_CMD_ACCEL QEMU_CMD_MEM QEMU_CMD_CHR
+                          QEMU_CMD_NETDEV ",queues=%d "
+                          "-device virtio-net-pci,netdev=net0,mq=on,vectors=%d",
+                          512, 512, root, s->chr_name,
+                          s->socket_path, "", s->chr_name,
+                          queues, queues * 2 + 2);
+    qtest_start(cmd);
+    g_free(cmd);
+
+    bus = qpci_init_pc();
+    dev = virtio_net_pci_init(bus, PCI_SLOT);
+
+    alloc = pc_alloc_init();
+    for (i = 0; i < queues * 2; i++) {
+        vq[i] = (QVirtQueuePCI *)qvirtqueue_setup(&qvirtio_pci, &dev->vdev,
+                                              alloc, i);
+    }
+
+    driver_init(&qvirtio_pci, &dev->vdev);
+    wait_for_rings_started(s, queues * 2);
+
+    /* End test */
+    for (i = 0; i < queues * 2; i++) {
+        qvirtqueue_cleanup(&qvirtio_pci, &vq[i]->vq, alloc);
+    }
+    pc_alloc_uninit(alloc);
+    qvirtio_pci_device_disable(dev);
+    g_free(dev->pdev);
+    g_free(dev);
+    qpci_free_pc(bus);
+    qtest_end();
+
+    test_server_free(s);
+}
+
 int main(int argc, char **argv)
 {
     QTestState *s = NULL;
@@ -766,6 +869,7 @@ int main(int argc, char **argv)
 
     qtest_add_data_func("/vhost-user/read-guest-mem", server, read_guest_mem);
     qtest_add_func("/vhost-user/migrate", test_migrate);
+    qtest_add_func("/vhost-user/multiqueue", test_multiqueue);
 #ifdef CONFIG_HAS_GLIB_SUBPROCESS_TESTS
     qtest_add_func("/vhost-user/reconnect/subprocess",
                    test_reconnect_subprocess);
-- 
2.9.0

[Qemu-devel] [PATCH v5 29/31] vhost-user: add error report in vhost_user_write()

[marcandre.lureau]

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

Similar to vhost_user_read() error report, it is useful to have early
error report.

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

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 819481d..1995fd2 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -176,7 +176,7 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
                             int *fds, int fd_num)
 {
     CharDriverState *chr = dev->opaque;
-    int size = VHOST_USER_HDR_SIZE + msg->size;
+    int ret, size = VHOST_USER_HDR_SIZE + msg->size;
 
     /*
      * For non-vring specific requests, like VHOST_USER_SET_MEM_TABLE,
@@ -188,11 +188,18 @@ static int vhost_user_write(struct vhost_dev *dev, VhostUserMsg *msg,
     }
 
     if (qemu_chr_fe_set_msgfds(chr, fds, fd_num) < 0) {
+        error_report("Failed to set msg fds.");
         return -1;
     }
 
-    return qemu_chr_fe_write_all(chr, (const uint8_t *) msg, size) == size ?
-            0 : -1;
+    ret = qemu_chr_fe_write_all(chr, (const uint8_t *) msg, size);
+    if (ret != size) {
+        error_report("Failed to write msg."
+                     " Wrote %d instead of %d.", ret, size);
+        return -1;
+    }
+
+    return 0;
 }
 
 static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
-- 
2.9.0

[Qemu-devel] [PATCH v5 30/31] vhost: add vhost_net_set_backend()

[marcandre.lureau]

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

Not all vhost-user backends support ops->vhost_net_set_backend(). It is
a nicer to provide an assert/error than to crash trying to
call. Furthermore, it improves a bit the code by hiding vhost_ops
details.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 hw/net/vhost_net.c        |  9 +++------
 hw/virtio/vhost.c         | 10 ++++++++++
 include/hw/virtio/vhost.h |  4 ++++
 3 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index dd41a8e..dc61dc1 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -242,8 +242,7 @@ static int vhost_net_start_one(struct vhost_net *net,
         qemu_set_fd_handler(net->backend, NULL, NULL, NULL);
         file.fd = net->backend;
         for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
-            const VhostOps *vhost_ops = net->dev.vhost_ops;
-            r = vhost_ops->vhost_net_set_backend(&net->dev, &file);
+            r = vhost_net_set_backend(&net->dev, &file);
             if (r < 0) {
                 r = -errno;
                 goto fail;
@@ -255,8 +254,7 @@ fail:
     file.fd = -1;
     if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
         while (file.index-- > 0) {
-            const VhostOps *vhost_ops = net->dev.vhost_ops;
-            int r = vhost_ops->vhost_net_set_backend(&net->dev, &file);
+            int r = vhost_net_set_backend(&net->dev, &file);
             assert(r >= 0);
         }
     }
@@ -277,8 +275,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
 
     if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
         for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
-            const VhostOps *vhost_ops = net->dev.vhost_ops;
-            int r = vhost_ops->vhost_net_set_backend(&net->dev, &file);
+            int r = vhost_net_set_backend(&net->dev, &file);
             assert(r >= 0);
         }
     }
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 970261b..c9466f1 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1369,3 +1369,13 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
     vhost_log_put(hdev, true);
     hdev->started = false;
 }
+
+int vhost_net_set_backend(struct vhost_dev *hdev,
+                          struct vhost_vring_file *file)
+{
+    if (hdev->vhost_ops->vhost_net_set_backend) {
+        return hdev->vhost_ops->vhost_net_set_backend(hdev, file);
+    }
+
+    return -1;
+}
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 2106ed8..e433089 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -86,4 +86,8 @@ uint64_t vhost_get_features(struct vhost_dev *hdev, const int *feature_bits,
 void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
                         uint64_t features);
 bool vhost_has_free_slot(void);
+
+int vhost_net_set_backend(struct vhost_dev *hdev,
+                          struct vhost_vring_file *file);
+
 #endif
-- 
2.9.0

[Qemu-devel] [PATCH v5 31/31] RFC: vhost: do not update last avail idx on get_vring_base() failure

[marcandre.lureau]

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

The state.num value will probably be 0 in this case, but I guess that
doesn't make sense to update.

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index c9466f1..6175d8b 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -945,8 +945,9 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
     r = dev->vhost_ops->vhost_get_vring_base(dev, &state);
     if (r < 0) {
         VHOST_OPS_DEBUG("vhost VQ %d ring restore failed: %d", idx, r);
+    } else {
+        virtio_queue_set_last_avail_idx(vdev, idx, state.num);
     }
-    virtio_queue_set_last_avail_idx(vdev, idx, state.num);
     virtio_queue_invalidate_signalled_used(vdev, idx);
 
     /* In the cross-endian case, we need to reset the vring endianness to
-- 
2.9.0

Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init()

[Ilya Maximets]

On 21.07.2016 11:57, Marc-André Lureau wrote:
> 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 not call vhost_virtqueue_cleanup() but vhost_dev_cleanup()
> instead.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/virtio/vhost.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 9400b47..c61302a 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1047,7 +1047,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;
>          }
>      }
>  
> @@ -1104,19 +1105,13 @@ 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_busyloop:
>      while (--i >= 0) {
>          vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0);
>      }
> -    i = hdev->nvqs;
> -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;
>  }
>  
> 

This patch introduces closing of zero fd on backend init failure or any
other error before virtqueue_init loop because of calling
'vhost_virtqueue_cleanup()' on not initialized virtqueues.

I'm suggesting following fixup:

----------------------------------------------------------------------
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 6175d8b..d7428c5 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1038,8 +1038,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
                    VhostBackendType backend_type, uint32_t busyloop_timeout)
 {
     uint64_t features;
-    int i, r;
+    int i, r, n_initialized_vqs;
 
+    n_initialized_vqs = 0;
     hdev->migration_blocker = NULL;
 
     r = vhost_set_backend_type(hdev, backend_type);

@@ -1069,10 +1071,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
         goto fail;
     }
 
-    for (i = 0; i < hdev->nvqs; ++i) {
+    for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
         r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
         if (r < 0) {
-            hdev->nvqs = i;
             goto fail;
         }
     }
@@ -1136,6 +1137,7 @@ fail_busyloop:
         vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0);
     }
 fail:
+    hdev->nvqs = n_initialized_vqs;
     vhost_dev_cleanup(hdev);
     return r;
 }
----------------------------------------------------------------------

Best regards, Ilya Maximets.

Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init()

[Marc-André Lureau]

Hi

----- Original Message -----
> On 21.07.2016 11:57, Marc-André Lureau wrote:
> > 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 not call vhost_virtqueue_cleanup() but vhost_dev_cleanup()
> > instead.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/virtio/vhost.c | 13 ++++---------
> >  1 file changed, 4 insertions(+), 9 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 9400b47..c61302a 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -1047,7 +1047,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;
> >          }
> >      }
> >  
> > @@ -1104,19 +1105,13 @@ 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_busyloop:
> >      while (--i >= 0) {
> >          vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0);
> >      }
> > -    i = hdev->nvqs;
> > -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;
> >  }
> >  
> > 
> 
> This patch introduces closing of zero fd on backend init failure or any
> other error before virtqueue_init loop because of calling
> 'vhost_virtqueue_cleanup()' on not initialized virtqueues.
> 
> I'm suggesting following fixup:
> 
> ----------------------------------------------------------------------
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 6175d8b..d7428c5 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1038,8 +1038,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> *opaque,
>                     VhostBackendType backend_type, uint32_t busyloop_timeout)
>  {
>      uint64_t features;
> -    int i, r;
> +    int i, r, n_initialized_vqs;
>  
> +    n_initialized_vqs = 0;
>      hdev->migration_blocker = NULL;
>  
>      r = vhost_set_backend_type(hdev, backend_type);
> 
> @@ -1069,10 +1071,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> *opaque,
>          goto fail;
>      }
>  
> -    for (i = 0; i < hdev->nvqs; ++i) {
> +    for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
>          r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
>          if (r < 0) {
> -            hdev->nvqs = i;

Isn't that assignment doing the same thing?

btw, thanks for the review

>              goto fail;
>          }
>      }
> @@ -1136,6 +1137,7 @@ fail_busyloop:
>          vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0);
>      }
>  fail:
> +    hdev->nvqs = n_initialized_vqs;
>      vhost_dev_cleanup(hdev);
>      return r;
>  }
> ----------------------------------------------------------------------
> 
> Best regards, Ilya Maximets.
> 

Re: [Qemu-devel] [v5, 17/31] vhost-user: keep vhost_net after a disconnection

[Ilya Maximets]

On 21.07.2016 11:57, Marc-André Lureau wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Many code paths assume get_vhost_net() returns non-null.
> 
> Keep VhostUserState.vhost_net after a successful vhost_net_init(),
> instead of freeing it in vhost_net_cleanup().
> 
> VhostUserState.vhost_net is thus freed before after being recreated or
> on final vhost_user_cleanup() and there is no need to save the acked
> features.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/net/vhost_net.c |  1 -
>  net/tap.c          |  1 +
>  net/vhost-user.c   | 36 ++++++++++++++++--------------------
>  3 files changed, 17 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index c11f69c..7b76591 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 40a8c74..6abb962 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -312,6 +312,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 2af212b..60fab9a 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -23,7 +23,6 @@ typedef struct VhostUserState {
>      CharDriverState *chr;
>      VHostNetState *vhost_net;
>      guint watch;
> -    uint64_t acked_features;
>  } VhostUserState;
>  
>  typedef struct VhostUserChardevProps {
> @@ -42,12 +41,7 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc)
>  {
>      VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
>      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_USER);
> -    return s->acked_features;
> -}
> -
> -static int vhost_user_running(VhostUserState *s)
> -{
> -    return (s->vhost_net) ? 1 : 0;
> +    return s->vhost_net ? vhost_net_get_acked_features(s->vhost_net) : 0;
>  }
>  
>  static void vhost_user_stop(int queues, NetClientState *ncs[])
> @@ -59,15 +53,9 @@ static void vhost_user_stop(int queues, NetClientState *ncs[])
>          assert(ncs[i]->info->type == NET_CLIENT_DRIVER_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);
>              vhost_net_cleanup(s->vhost_net);
> -            s->vhost_net = NULL;
>          }
>      }
>  }
> @@ -75,6 +63,7 @@ static void vhost_user_stop(int queues, NetClientState *ncs[])
>  static int vhost_user_start(int queues, NetClientState *ncs[])
>  {
>      VhostNetOptions options;
> +    struct vhost_net *net = NULL;
>      VhostUserState *s;
>      int max_queues;
>      int i;
> @@ -85,33 +74,39 @@ static int vhost_user_start(int queues, NetClientState *ncs[])
>          assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER);
>  
>          s = DO_UPCAST(VhostUserState, nc, ncs[i]);
> -        if (vhost_user_running(s)) {
> -            continue;
> -        }
>  
>          options.net_backend = ncs[i];
>          options.opaque      = s->chr;
>          options.busyloop_timeout = 0;
> -        s->vhost_net = vhost_net_init(&options);
> -        if (!s->vhost_net) {
> +        net = vhost_net_init(&options);
> +        if (!net) {
>              error_report("failed to init vhost_net for queue %d", i);
>              goto err;
>          }
>  
>          if (i == 0) {
> -            max_queues = vhost_net_get_max_queues(s->vhost_net);
> +            max_queues = vhost_net_get_max_queues(net);
>              if (queues > max_queues) {
>                  error_report("you are asking more queues than supported: %d",
>                               max_queues);
>                  goto err;
>              }
>          }
> +
> +        if (s->vhost_net) {
> +            vhost_net_cleanup(s->vhost_net);
> +            g_free(s->vhost_net);
> +        }
> +        s->vhost_net = net;
>      }
>  
>      return 0;
>  
>  err:
> -    vhost_user_stop(i + 1, ncs);
> +    if (net) {
> +        vhost_net_cleanup(net);
> +    }
> +    vhost_user_stop(i, ncs);
>      return -1;
>  }
>  
> @@ -150,6 +145,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;
>      }
>      if (s->chr) {
> 

In patch number 7 of this patch set was introduced 'memset()' inside
'vhost_dev_cleanup()' which clears 'acked_features' field of 'vhost_dev'
structure. This patch uses already zeroed value of this field for
features restore after reconnection.

You should not remove 'acked_features' from 'struct VhostUserState' or
avoid cleaning of this field in 'vhost_dev'.

I'm suggesting following fixup (mainly, just a partial revert):
----------------------------------------------------------------------
diff --git a/net/vhost-user.c b/net/vhost-user.c
index 60fab9a..3100a5e 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -23,6 +23,7 @@ typedef struct VhostUserState {
     CharDriverState *chr;
     VHostNetState *vhost_net;
     guint watch;
+    uint64_t acked_features;
 } VhostUserState;
 
 typedef struct VhostUserChardevProps {
@@ -41,7 +42,7 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc)
 {
     VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
     assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_USER);
-    return s->vhost_net ? vhost_net_get_acked_features(s->vhost_net) : 0;
+    return s->acked_features;
 }
 
 static void vhost_user_stop(int queues, NetClientState *ncs[])
@@ -55,6 +56,11 @@ static void vhost_user_stop(int queues, NetClientState *ncs[])
         s = DO_UPCAST(VhostUserState, nc, ncs[i]);
 
         if (s->vhost_net) {
+            /* save acked features */
+            uint64_t features = vhost_net_get_acked_features(s->vhost_net);
+            if (features) {
+                s->acked_features = features;
+            }
             vhost_net_cleanup(s->vhost_net);
         }
     }
----------------------------------------------------------------------

Best regards, Ilya Maximets.

Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init()

[Ilya Maximets]

On 25.07.2016 15:45, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>> On 21.07.2016 11:57, Marc-André Lureau wrote:
>>> 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 not call vhost_virtqueue_cleanup() but vhost_dev_cleanup()
>>> instead.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> ---
>>>  hw/virtio/vhost.c | 13 ++++---------
>>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>> index 9400b47..c61302a 100644
>>> --- a/hw/virtio/vhost.c
>>> +++ b/hw/virtio/vhost.c
>>> @@ -1047,7 +1047,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;
>>>          }
>>>      }
>>>  
>>> @@ -1104,19 +1105,13 @@ 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_busyloop:
>>>      while (--i >= 0) {
>>>          vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0);
>>>      }
>>> -    i = hdev->nvqs;
>>> -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;
>>>  }
>>>  
>>>
>>
>> This patch introduces closing of zero fd on backend init failure or any
>> other error before virtqueue_init loop because of calling
>> 'vhost_virtqueue_cleanup()' on not initialized virtqueues.
>>
>> I'm suggesting following fixup:
>>
>> ----------------------------------------------------------------------
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 6175d8b..d7428c5 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1038,8 +1038,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void
>> *opaque,
>>                     VhostBackendType backend_type, uint32_t busyloop_timeout)
>>  {
>>      uint64_t features;
>> -    int i, r;
>> +    int i, r, n_initialized_vqs;
>>  
>> +    n_initialized_vqs = 0;
>>      hdev->migration_blocker = NULL;
>>  
>>      r = vhost_set_backend_type(hdev, backend_type);
>>
>> @@ -1069,10 +1071,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void
>> *opaque,
>>          goto fail;
>>      }
>>  
>> -    for (i = 0; i < hdev->nvqs; ++i) {
>> +    for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
>>          r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
>>          if (r < 0) {
>> -            hdev->nvqs = i;
> 
> Isn't that assignment doing the same thing?

Yes.
But assignment to zero (hdev->nvqs = 0) required before all previous
'goto fail;' instructions. I think, it's not a clean solution.

> btw, thanks for the review
> 
>>              goto fail;
>>          }
>>      }
>> @@ -1136,6 +1137,7 @@ fail_busyloop:
>>          vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i, 0);
>>      }
>>  fail:
>> +    hdev->nvqs = n_initialized_vqs;
>>      vhost_dev_cleanup(hdev);
>>      return r;
>>  }
>> ----------------------------------------------------------------------
>>
>> Best regards, Ilya Maximets.
>>
> 
> 

Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init()

[Marc-André Lureau]

Hi

----- Original Message -----
> On 25.07.2016 15:45, Marc-André Lureau wrote:
> > Hi
> > 
> > ----- Original Message -----
> >> On 21.07.2016 11:57, Marc-André Lureau wrote:
> >>> 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 not call vhost_virtqueue_cleanup() but vhost_dev_cleanup()
> >>> instead.
> >>>
> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>> ---
> >>>  hw/virtio/vhost.c | 13 ++++---------
> >>>  1 file changed, 4 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >>> index 9400b47..c61302a 100644
> >>> --- a/hw/virtio/vhost.c
> >>> +++ b/hw/virtio/vhost.c
> >>> @@ -1047,7 +1047,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;
> >>>          }
> >>>      }
> >>>  
> >>> @@ -1104,19 +1105,13 @@ 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_busyloop:
> >>>      while (--i >= 0) {
> >>>          vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i,
> >>>          0);
> >>>      }
> >>> -    i = hdev->nvqs;
> >>> -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;
> >>>  }
> >>>  
> >>>
> >>
> >> This patch introduces closing of zero fd on backend init failure or any
> >> other error before virtqueue_init loop because of calling
> >> 'vhost_virtqueue_cleanup()' on not initialized virtqueues.
> >>
> >> I'm suggesting following fixup:
> >>
> >> ----------------------------------------------------------------------
> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> index 6175d8b..d7428c5 100644
> >> --- a/hw/virtio/vhost.c
> >> +++ b/hw/virtio/vhost.c
> >> @@ -1038,8 +1038,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> >> *opaque,
> >>                     VhostBackendType backend_type, uint32_t
> >>                     busyloop_timeout)
> >>  {
> >>      uint64_t features;
> >> -    int i, r;
> >> +    int i, r, n_initialized_vqs;
> >>  
> >> +    n_initialized_vqs = 0;
> >>      hdev->migration_blocker = NULL;
> >>  
> >>      r = vhost_set_backend_type(hdev, backend_type);
> >>
> >> @@ -1069,10 +1071,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void
> >> *opaque,
> >>          goto fail;
> >>      }
> >>  
> >> -    for (i = 0; i < hdev->nvqs; ++i) {
> >> +    for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
> >>          r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index +
> >>          i);
> >>          if (r < 0) {
> >> -            hdev->nvqs = i;
> > 
> > Isn't that assignment doing the same thing?
> 
> Yes.
> But assignment to zero (hdev->nvqs = 0) required before all previous
> 'goto fail;' instructions. I think, it's not a clean solution.
> 

Good point, I'll squash your change, should I add your sign-off-by?

thanks

> > btw, thanks for the review
> > 
> >>              goto fail;
> >>          }
> >>      }
> >> @@ -1136,6 +1137,7 @@ fail_busyloop:
> >>          vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i,
> >>          0);
> >>      }
> >>  fail:
> >> +    hdev->nvqs = n_initialized_vqs;
> >>      vhost_dev_cleanup(hdev);
> >>      return r;
> >>  }
> >> ----------------------------------------------------------------------
> >>
> >> Best regards, Ilya Maximets.
> >>
> > 
> > 
> 

Re: [Qemu-devel] [v5, 17/31] vhost-user: keep vhost_net after a disconnection

[Marc-André Lureau]

Hi

----- Original Message -----
> On 21.07.2016 11:57, Marc-André Lureau wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > Many code paths assume get_vhost_net() returns non-null.
> > 
> > Keep VhostUserState.vhost_net after a successful vhost_net_init(),
> > instead of freeing it in vhost_net_cleanup().
> > 
> > VhostUserState.vhost_net is thus freed before after being recreated or
> > on final vhost_user_cleanup() and there is no need to save the acked
> > features.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/net/vhost_net.c |  1 -
> >  net/tap.c          |  1 +
> >  net/vhost-user.c   | 36 ++++++++++++++++--------------------
> >  3 files changed, 17 insertions(+), 21 deletions(-)
> > 
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index c11f69c..7b76591 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 40a8c74..6abb962 100644
> > --- a/net/tap.c
> > +++ b/net/tap.c
> > @@ -312,6 +312,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 2af212b..60fab9a 100644
> > --- a/net/vhost-user.c
> > +++ b/net/vhost-user.c
> > @@ -23,7 +23,6 @@ typedef struct VhostUserState {
> >      CharDriverState *chr;
> >      VHostNetState *vhost_net;
> >      guint watch;
> > -    uint64_t acked_features;
> >  } VhostUserState;
> >  
> >  typedef struct VhostUserChardevProps {
> > @@ -42,12 +41,7 @@ uint64_t vhost_user_get_acked_features(NetClientState
> > *nc)
> >  {
> >      VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
> >      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_USER);
> > -    return s->acked_features;
> > -}
> > -
> > -static int vhost_user_running(VhostUserState *s)
> > -{
> > -    return (s->vhost_net) ? 1 : 0;
> > +    return s->vhost_net ? vhost_net_get_acked_features(s->vhost_net) : 0;
> >  }
> >  
> >  static void vhost_user_stop(int queues, NetClientState *ncs[])
> > @@ -59,15 +53,9 @@ static void vhost_user_stop(int queues, NetClientState
> > *ncs[])
> >          assert(ncs[i]->info->type == NET_CLIENT_DRIVER_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);
> >              vhost_net_cleanup(s->vhost_net);
> > -            s->vhost_net = NULL;
> >          }
> >      }
> >  }
> > @@ -75,6 +63,7 @@ static void vhost_user_stop(int queues, NetClientState
> > *ncs[])
> >  static int vhost_user_start(int queues, NetClientState *ncs[])
> >  {
> >      VhostNetOptions options;
> > +    struct vhost_net *net = NULL;
> >      VhostUserState *s;
> >      int max_queues;
> >      int i;
> > @@ -85,33 +74,39 @@ static int vhost_user_start(int queues, NetClientState
> > *ncs[])
> >          assert(ncs[i]->info->type == NET_CLIENT_DRIVER_VHOST_USER);
> >  
> >          s = DO_UPCAST(VhostUserState, nc, ncs[i]);
> > -        if (vhost_user_running(s)) {
> > -            continue;
> > -        }
> >  
> >          options.net_backend = ncs[i];
> >          options.opaque      = s->chr;
> >          options.busyloop_timeout = 0;
> > -        s->vhost_net = vhost_net_init(&options);
> > -        if (!s->vhost_net) {
> > +        net = vhost_net_init(&options);
> > +        if (!net) {
> >              error_report("failed to init vhost_net for queue %d", i);
> >              goto err;
> >          }
> >  
> >          if (i == 0) {
> > -            max_queues = vhost_net_get_max_queues(s->vhost_net);
> > +            max_queues = vhost_net_get_max_queues(net);
> >              if (queues > max_queues) {
> >                  error_report("you are asking more queues than supported:
> >                  %d",
> >                               max_queues);
> >                  goto err;
> >              }
> >          }
> > +
> > +        if (s->vhost_net) {
> > +            vhost_net_cleanup(s->vhost_net);
> > +            g_free(s->vhost_net);
> > +        }
> > +        s->vhost_net = net;
> >      }
> >  
> >      return 0;
> >  
> >  err:
> > -    vhost_user_stop(i + 1, ncs);
> > +    if (net) {
> > +        vhost_net_cleanup(net);
> > +    }
> > +    vhost_user_stop(i, ncs);
> >      return -1;
> >  }
> >  
> > @@ -150,6 +145,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;
> >      }
> >      if (s->chr) {
> > 
> 
> In patch number 7 of this patch set was introduced 'memset()' inside
> 'vhost_dev_cleanup()' which clears 'acked_features' field of 'vhost_dev'
> structure. This patch uses already zeroed value of this field for
> features restore after reconnection.
> 
> You should not remove 'acked_features' from 'struct VhostUserState' or
> avoid cleaning of this field in 'vhost_dev'.
> 
> I'm suggesting following fixup (mainly, just a partial revert):

Thanks! I'll squash the fix.

> ----------------------------------------------------------------------
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 60fab9a..3100a5e 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -23,6 +23,7 @@ typedef struct VhostUserState {
>      CharDriverState *chr;
>      VHostNetState *vhost_net;
>      guint watch;
> +    uint64_t acked_features;
>  } VhostUserState;
>  
>  typedef struct VhostUserChardevProps {
> @@ -41,7 +42,7 @@ uint64_t vhost_user_get_acked_features(NetClientState *nc)
>  {
>      VhostUserState *s = DO_UPCAST(VhostUserState, nc, nc);
>      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_USER);
> -    return s->vhost_net ? vhost_net_get_acked_features(s->vhost_net) : 0;
> +    return s->acked_features;
>  }
>  
>  static void vhost_user_stop(int queues, NetClientState *ncs[])
> @@ -55,6 +56,11 @@ static void vhost_user_stop(int queues, NetClientState
> *ncs[])
>          s = DO_UPCAST(VhostUserState, nc, ncs[i]);
>  
>          if (s->vhost_net) {
> +            /* save acked features */
> +            uint64_t features = vhost_net_get_acked_features(s->vhost_net);
> +            if (features) {
> +                s->acked_features = features;
> +            }
>              vhost_net_cleanup(s->vhost_net);
>          }
>      }
> ----------------------------------------------------------------------
> 
> Best regards, Ilya Maximets.
> 

Re: [Qemu-devel] [v5, 09/31] vhost: fix calling vhost_dev_cleanup() after vhost_dev_init()

[Ilya Maximets]

On 25.07.2016 16:05, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
>> On 25.07.2016 15:45, Marc-André Lureau wrote:
>>> Hi
>>>
>>> ----- Original Message -----
>>>> On 21.07.2016 11:57, Marc-André Lureau wrote:
>>>>> 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 not call vhost_virtqueue_cleanup() but vhost_dev_cleanup()
>>>>> instead.
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>> ---
>>>>>  hw/virtio/vhost.c | 13 ++++---------
>>>>>  1 file changed, 4 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>>>> index 9400b47..c61302a 100644
>>>>> --- a/hw/virtio/vhost.c
>>>>> +++ b/hw/virtio/vhost.c
>>>>> @@ -1047,7 +1047,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;
>>>>>          }
>>>>>      }
>>>>>  
>>>>> @@ -1104,19 +1105,13 @@ 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_busyloop:
>>>>>      while (--i >= 0) {
>>>>>          vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i,
>>>>>          0);
>>>>>      }
>>>>> -    i = hdev->nvqs;
>>>>> -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;
>>>>>  }
>>>>>  
>>>>>
>>>>
>>>> This patch introduces closing of zero fd on backend init failure or any
>>>> other error before virtqueue_init loop because of calling
>>>> 'vhost_virtqueue_cleanup()' on not initialized virtqueues.
>>>>
>>>> I'm suggesting following fixup:
>>>>
>>>> ----------------------------------------------------------------------
>>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>>> index 6175d8b..d7428c5 100644
>>>> --- a/hw/virtio/vhost.c
>>>> +++ b/hw/virtio/vhost.c
>>>> @@ -1038,8 +1038,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void
>>>> *opaque,
>>>>                     VhostBackendType backend_type, uint32_t
>>>>                     busyloop_timeout)
>>>>  {
>>>>      uint64_t features;
>>>> -    int i, r;
>>>> +    int i, r, n_initialized_vqs;
>>>>  
>>>> +    n_initialized_vqs = 0;
>>>>      hdev->migration_blocker = NULL;
>>>>  
>>>>      r = vhost_set_backend_type(hdev, backend_type);
>>>>
>>>> @@ -1069,10 +1071,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void
>>>> *opaque,
>>>>          goto fail;
>>>>      }
>>>>  
>>>> -    for (i = 0; i < hdev->nvqs; ++i) {
>>>> +    for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
>>>>          r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index +
>>>>          i);
>>>>          if (r < 0) {
>>>> -            hdev->nvqs = i;
>>>
>>> Isn't that assignment doing the same thing?
>>
>> Yes.
>> But assignment to zero (hdev->nvqs = 0) required before all previous
>> 'goto fail;' instructions. I think, it's not a clean solution.
>>
> 
> Good point, I'll squash your change,

Thanks for fixing it.

> should I add your sign-off-by?

I don't mind if you want to.

Best regards, Ilya Maximets.