[Qemu-devel] [PATCH v3 00/28] vhost-user reconnect fixes

[Qemu-devel] [PATCH v3 00/28] vhost-user reconnect fixes

[marcandre.lureau]

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

Hi,

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

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

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

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

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

 hw/net/vhost_net.c        |  28 ++++-----
 hw/virtio/vhost-user.c    |  67 ++++++++++++++-------
 hw/virtio/vhost.c         | 123 +++++++++++++++++++++++---------------
 include/hw/virtio/vhost.h |   4 ++
 include/sysemu/char.h     |   8 +++
 net/tap.c                 |   1 +
 net/vhost-user.c          |  44 ++++++++------
 qemu-char.c               |  82 ++++++++++++++++++--------
 tests/Makefile.include    |   2 +-
 tests/vhost-user-test.c   | 147 +++++++++++++++++++++++++++++++++++++++++++++-
 tests/virtio-net-test.c   |  12 +++-
 11 files changed, 385 insertions(+), 133 deletions(-)

-- 
2.9.0

[Qemu-devel] [PATCH v3 01/28] misc: indentation

[marcandre.lureau]

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

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

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

[Qemu-devel] [PATCH v3 02/28] vhost-user: minor simplification

[marcandre.lureau]

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

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

diff --git a/net/vhost-user.c b/net/vhost-user.c
index 92f4cfd..18bd170 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -315,7 +315,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");
@@ -325,7 +324,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 v3 03/28] vhost: don't assume opaque is a fd, use backend cleanup

[marcandre.lureau]

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

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

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

[Qemu-devel] [PATCH v3 04/28] vhost: make vhost_log_put() idempotent

[marcandre.lureau]

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

Clear dev->log* when calling vhost_log_put()

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index dbb4deb..2753bb4 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);
@@ -1290,7 +1290,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 v3 05/28] vhost: call vhost_log_put() on cleanup

[marcandre.lureau]

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

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

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

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

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

[marcandre.lureau]

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

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

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

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

[Qemu-devel] [PATCH v3 07/28] vhost: make vhost_dev_cleanup() idempotent

[marcandre.lureau]

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

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

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

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

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

[marcandre.lureau]

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

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

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

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

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

[marcandre.lureau]

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

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

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 9fc6e4d..75bc51e 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1025,7 +1025,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     for (i = 0; i < hdev->nvqs; ++i) {
         r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
         if (r < 0) {
-            goto fail_vq;
+            hdev->nvqs = i;
+            goto fail;
         }
     }
     hdev->features = features;
@@ -1071,14 +1072,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     memory_listener_register(&hdev->memory_listener, &address_space_memory);
     QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
     return 0;
-fail_vq:
-    while (--i >= 0) {
-        vhost_virtqueue_cleanup(hdev->vqs + i);
-    }
+
 fail:
-    r = -errno;
-    hdev->vhost_ops->vhost_backend_cleanup(hdev);
-    QLIST_REMOVE(hdev, entry);
+    vhost_dev_cleanup(hdev);
     return r;
 }
 
-- 
2.9.0

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

[marcandre.lureau]

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

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

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

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

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

[Michael S. Tsirkin]

On Wed, Jul 06, 2016 at 08:47:03PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Calling a vhost operation may fail, especially with disconnectable
> backends. Treat some that look harmless as recoverable errors (print
> error, or ignore on error code path).
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

So this is handy for now until disconnect handling stabilizes,
but eventually these bnign errors should be hidden from the user.
What I'd like to see is something like

#define VHOST_DISCONNECT_DEBUG error_report

and then use that.

After things stabilize we'll be able to drop these easily.



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

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

[Michael S. Tsirkin]

On Wed, Jul 06, 2016 at 08:47:03PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Calling a vhost operation may fail, especially with disconnectable
> backends. Treat some that look harmless as recoverable errors (print
> error, or ignore on error code path).
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

These might be recoverable for vhost-user not for vhost_net.

IMO the backend should return 0 if error is benign,
report errors to vhost only if they are fatal.

For example, consider set mem table. Write failing is one thing,
and it's benign, but e.g. table too big is another thing and isn't.

Also, we might want to distinguish between EBADF (fd closed)
and other types of errors. All this knowledge belomgs in vhost user.

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

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

[Marc-André Lureau]

----- Original Message -----
> On Wed, Jul 06, 2016 at 08:47:03PM +0200, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > Calling a vhost operation may fail, especially with disconnectable
> > backends. Treat some that look harmless as recoverable errors (print
> > error, or ignore on error code path).
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> These might be recoverable for vhost-user not for vhost_net.

I don't think we can hide all the error handling in vhost-user very long, soon enough we will need to reset the guest device state. If vhost-net doesn't support error, it should rather assert() there, but having the error handling done at higher level, at the vhost interface level at least, not at the backend level. 

> IMO the backend should return 0 if error is benign,
> report errors to vhost only if they are fatal.

Imho whether it's fatal and how to recover as not much to do with the backend (which each kind of just a proxy), it should be handled at higher level, possibly up to the guest.

> For example, consider set mem table. Write failing is one thing,
> and it's benign, but e.g. table too big is another thing and isn't.
> 
> Also, we might want to distinguish between EBADF (fd closed)
> and other types of errors. All this knowledge belomgs in vhost user.
> 
> > ---
> >  hw/virtio/vhost.c | 32 +++++++++++++++++++++-----------
> >  1 file changed, 21 insertions(+), 11 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 75bc51e..e03a031 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -400,7 +400,10 @@ static inline void vhost_dev_log_resize(struct
> > vhost_dev *dev, uint64_t size)
> >      /* inform backend of log switching, this must be done before
> >         releasing the current log, to ensure no logging is lost */
> >      r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> > -    assert(r >= 0);
> > +    if (r < 0) {
> > +        error_report("Failed to change backend log");
> > +    }
> > +
> >      vhost_log_put(dev, true);
> >      dev->log = log;
> >      dev->log_size = size;
> > @@ -567,7 +570,9 @@ static void vhost_commit(MemoryListener *listener)
> >  
> >      if (!dev->log_enabled) {
> >          r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> > -        assert(r >= 0);
> > +        if (r < 0) {
> > +            error_report("Failed to set mem table");
> > +        }
> >          dev->memory_changed = false;
> >          return;
> >      }
> > @@ -580,7 +585,9 @@ static void vhost_commit(MemoryListener *listener)
> >          vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
> >      }
> >      r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> > -    assert(r >= 0);
> > +    if (r < 0) {
> > +        error_report("Failed to set mem table");
> > +    }
> >      /* To log less, can only decrease log size after table update. */
> >      if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
> >          vhost_dev_log_resize(dev, log_size);
> > @@ -649,6 +656,7 @@ static int vhost_virtqueue_set_addr(struct vhost_dev
> > *dev,
> >      };
> >      int r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr);
> >      if (r < 0) {
> > +        error_report("Failed to set vring addr");
> >          return -errno;
> >      }
> >      return 0;
> > @@ -662,12 +670,15 @@ static int vhost_dev_set_features(struct vhost_dev
> > *dev, bool enable_log)
> >          features |= 0x1ULL << VHOST_F_LOG_ALL;
> >      }
> >      r = dev->vhost_ops->vhost_set_features(dev, features);
> > +    if (r < 0) {
> > +        error_report("Failed to set features");
> > +    }
> >      return r < 0 ? -errno : 0;
> >  }
> >  
> >  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> >  {
> > -    int r, t, i, idx;
> > +    int r, i, idx;
> >      r = vhost_dev_set_features(dev, enable_log);
> >      if (r < 0) {
> >          goto err_features;
> > @@ -684,12 +695,10 @@ static int vhost_dev_set_log(struct vhost_dev *dev,
> > bool enable_log)
> >  err_vq:
> >      for (; i >= 0; --i) {
> >          idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> > -        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> > -                                     dev->log_enabled);
> > -        assert(t >= 0);
> > +        vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> > +                                 dev->log_enabled);
> >      }
> > -    t = vhost_dev_set_features(dev, dev->log_enabled);
> > -    assert(t >= 0);
> > +    vhost_dev_set_features(dev, dev->log_enabled);
> >  err_features:
> >      return r;
> >  }
> > @@ -937,7 +946,6 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
> >          }
> >      }
> >  
> > -    assert (r >= 0);
> >      cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev,
> >      idx),
> >                                0, virtio_queue_get_ring_size(vdev, idx));
> >      cpu_physical_memory_unmap(vq->used, virtio_queue_get_used_size(vdev,
> >      idx),
> > @@ -1191,7 +1199,9 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev,
> > VirtIODevice *vdev, int n,
> >  
> >      file.index = hdev->vhost_ops->vhost_get_vq_index(hdev, n);
> >      r = hdev->vhost_ops->vhost_set_vring_call(hdev, &file);
> > -    assert(r >= 0);
> > +    if (r < 0) {
> > +        error_report("Failed to set vring call");
> > +    }
> >  }
> >  
> >  uint64_t vhost_get_features(struct vhost_dev *hdev, const int
> >  *feature_bits,
> > --
> > 2.9.0
> 

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

[Michael S. Tsirkin]

On Wed, Jul 20, 2016 at 09:41:26AM -0400, Marc-André Lureau wrote:
> 
> 
> ----- Original Message -----
> > On Wed, Jul 06, 2016 at 08:47:03PM +0200, marcandre.lureau@redhat.com wrote:
> > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > 
> > > Calling a vhost operation may fail, especially with disconnectable
> > > backends. Treat some that look harmless as recoverable errors (print
> > > error, or ignore on error code path).
> > > 
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > These might be recoverable for vhost-user not for vhost_net.
> 
> I don't think we can hide all the error handling in vhost-user very
> long, soon enough we will need to reset the guest device state.

Interesting. This will need some thought.


> If
> vhost-net doesn't support error, it should rather assert() there, but
> having the error handling done at higher level, at the vhost interface
> level at least, not at the backend level. 

Interesting. That might be reasonable too but would increase the scope
of this already large patchset even further.


> > IMO the backend should return 0 if error is benign,
> > report errors to vhost only if they are fatal.
> 
> Imho whether it's fatal and how to recover as not much to do with the backend (which each kind of just a proxy), it should be handled at higher level, possibly up to the guest.

Consider example below. EBADF means fd not writeable - remote exited
so that's benign. EFAULT means code bug. vhost has no idea there's
an fd though.


> > For example, consider set mem table. Write failing is one thing,
> > and it's benign, but e.g. table too big is another thing and isn't.
> > 
> > Also, we might want to distinguish between EBADF (fd closed)
> > and other types of errors. All this knowledge belomgs in vhost user.
> > 
> > > ---
> > >  hw/virtio/vhost.c | 32 +++++++++++++++++++++-----------
> > >  1 file changed, 21 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 75bc51e..e03a031 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -400,7 +400,10 @@ static inline void vhost_dev_log_resize(struct
> > > vhost_dev *dev, uint64_t size)
> > >      /* inform backend of log switching, this must be done before
> > >         releasing the current log, to ensure no logging is lost */
> > >      r = dev->vhost_ops->vhost_set_log_base(dev, log_base, log);
> > > -    assert(r >= 0);
> > > +    if (r < 0) {
> > > +        error_report("Failed to change backend log");
> > > +    }
> > > +
> > >      vhost_log_put(dev, true);
> > >      dev->log = log;
> > >      dev->log_size = size;
> > > @@ -567,7 +570,9 @@ static void vhost_commit(MemoryListener *listener)
> > >  
> > >      if (!dev->log_enabled) {
> > >          r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> > > -        assert(r >= 0);
> > > +        if (r < 0) {
> > > +            error_report("Failed to set mem table");
> > > +        }
> > >          dev->memory_changed = false;
> > >          return;
> > >      }
> > > @@ -580,7 +585,9 @@ static void vhost_commit(MemoryListener *listener)
> > >          vhost_dev_log_resize(dev, log_size + VHOST_LOG_BUFFER);
> > >      }
> > >      r = dev->vhost_ops->vhost_set_mem_table(dev, dev->mem);
> > > -    assert(r >= 0);
> > > +    if (r < 0) {
> > > +        error_report("Failed to set mem table");
> > > +    }
> > >      /* To log less, can only decrease log size after table update. */
> > >      if (dev->log_size > log_size + VHOST_LOG_BUFFER) {
> > >          vhost_dev_log_resize(dev, log_size);
> > > @@ -649,6 +656,7 @@ static int vhost_virtqueue_set_addr(struct vhost_dev
> > > *dev,
> > >      };
> > >      int r = dev->vhost_ops->vhost_set_vring_addr(dev, &addr);
> > >      if (r < 0) {
> > > +        error_report("Failed to set vring addr");
> > >          return -errno;
> > >      }
> > >      return 0;
> > > @@ -662,12 +670,15 @@ static int vhost_dev_set_features(struct vhost_dev
> > > *dev, bool enable_log)
> > >          features |= 0x1ULL << VHOST_F_LOG_ALL;
> > >      }
> > >      r = dev->vhost_ops->vhost_set_features(dev, features);
> > > +    if (r < 0) {
> > > +        error_report("Failed to set features");
> > > +    }
> > >      return r < 0 ? -errno : 0;
> > >  }
> > >  
> > >  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> > >  {
> > > -    int r, t, i, idx;
> > > +    int r, i, idx;
> > >      r = vhost_dev_set_features(dev, enable_log);
> > >      if (r < 0) {
> > >          goto err_features;
> > > @@ -684,12 +695,10 @@ static int vhost_dev_set_log(struct vhost_dev *dev,
> > > bool enable_log)
> > >  err_vq:
> > >      for (; i >= 0; --i) {
> > >          idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> > > -        t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> > > -                                     dev->log_enabled);
> > > -        assert(t >= 0);
> > > +        vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> > > +                                 dev->log_enabled);
> > >      }
> > > -    t = vhost_dev_set_features(dev, dev->log_enabled);
> > > -    assert(t >= 0);
> > > +    vhost_dev_set_features(dev, dev->log_enabled);
> > >  err_features:
> > >      return r;
> > >  }
> > > @@ -937,7 +946,6 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
> > >          }
> > >      }
> > >  
> > > -    assert (r >= 0);
> > >      cpu_physical_memory_unmap(vq->ring, virtio_queue_get_ring_size(vdev,
> > >      idx),
> > >                                0, virtio_queue_get_ring_size(vdev, idx));
> > >      cpu_physical_memory_unmap(vq->used, virtio_queue_get_used_size(vdev,
> > >      idx),
> > > @@ -1191,7 +1199,9 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev,
> > > VirtIODevice *vdev, int n,
> > >  
> > >      file.index = hdev->vhost_ops->vhost_get_vq_index(hdev, n);
> > >      r = hdev->vhost_ops->vhost_set_vring_call(hdev, &file);
> > > -    assert(r >= 0);
> > > +    if (r < 0) {
> > > +        error_report("Failed to set vring call");
> > > +    }
> > >  }
> > >  
> > >  uint64_t vhost_get_features(struct vhost_dev *hdev, const int
> > >  *feature_bits,
> > > --
> > > 2.9.0
> > 

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

[Marc-André Lureau]

Hi

----- Original Message -----
> On Wed, Jul 20, 2016 at 09:41:26AM -0400, Marc-André Lureau wrote:
> > 
> > 
> > ----- Original Message -----
> > > On Wed, Jul 06, 2016 at 08:47:03PM +0200, marcandre.lureau@redhat.com
> > > wrote:
> > > > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > > 
> > > > Calling a vhost operation may fail, especially with disconnectable
> > > > backends. Treat some that look harmless as recoverable errors (print
> > > > error, or ignore on error code path).
> > > > 
> > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > 
> > > These might be recoverable for vhost-user not for vhost_net.
> > 
> > I don't think we can hide all the error handling in vhost-user very
> > long, soon enough we will need to reset the guest device state.
> 
> Interesting. This will need some thought.
> 
> 
> > If
> > vhost-net doesn't support error, it should rather assert() there, but
> > having the error handling done at higher level, at the vhost interface
> > level at least, not at the backend level.
> 
> Interesting. That might be reasonable too but would increase the scope
> of this already large patchset even further.
> 
> 
> > > IMO the backend should return 0 if error is benign,
> > > report errors to vhost only if they are fatal.
> > 
> > Imho whether it's fatal and how to recover as not much to do with the
> > backend (which each kind of just a proxy), it should be handled at higher
> > level, possibly up to the guest.
> 
> Consider example below. EBADF means fd not writeable - remote exited
> so that's benign. EFAULT means code bug. vhost has no idea there's
> an fd though.
> 

It will probably be EPIPE, right? unless the fd was closed. Hhmpf, tcp_chr_write() actually returns the number of bytes to write if the peer is disconnected... and io_channel_send_full() on failure EINVAL.. 

> 
> > > For example, consider set mem table. Write failing is one thing,
> > > and it's benign, but e.g. table too big is another thing and isn't.

It depends, if the backend disconnects during that call, it isn't "fatal". A later reconnection will restart and reset vhost-user tables.

> > > Also, we might want to distinguish between EBADF (fd closed)
> > > and other types of errors. All this knowledge belomgs in vhost user.
> > > 

It's hard to hide disconnected state away, beside the need to report errors higher up, the handling of the disconnected state is not just in the vhost-user backend, but also in net/vhost-user.

I also notice that qemu_chr_fe_write*() will not trigger disconnect events, while read qemu_chr_fe_read() will: vhost_dev struct will be 0'ed during the call, by net_vhost_user_event() handler.

In most cases (there is a minor exception in set_vring_endian_legacy), vhost actually doesn't care about errno. However, it reports up errors using errno values. It seems it is only reported by vhost_dev_init() and vhost_dev_start(), and the value is used for strerror/error_report() (I notice also that failing vhost_dev_start() in vhost_scsi_start() is fatal)


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

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

[marcandre.lureau]

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

Let's use qemu proper error reporting API.

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e03a031..752a78f 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -427,11 +427,11 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
         l = vq->ring_size;
         p = cpu_physical_memory_map(vq->ring_phys, &l, 1);
         if (!p || l != vq->ring_size) {
-            fprintf(stderr, "Unable to map ring buffer for ring %d\n", i);
+            error_report("Unable to map ring buffer for ring %d", i);
             r = -ENOMEM;
         }
         if (p != vq->ring) {
-            fprintf(stderr, "Ring buffer relocated for ring %d\n", i);
+            error_report("Ring buffer relocated for ring %d", i);
             r = -EBUSY;
         }
         cpu_physical_memory_unmap(p, l, 0, 0);
@@ -928,8 +928,7 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
 
     r = dev->vhost_ops->vhost_get_vring_base(dev, &state);
     if (r < 0) {
-        fprintf(stderr, "vhost VQ %d ring restore failed: %d\n", idx, r);
-        fflush(stderr);
+        error_report("vhost VQ %d ring restore failed: %d", idx, r);
     }
     virtio_queue_set_last_avail_idx(vdev, idx, state.num);
     virtio_queue_invalidate_signalled_used(vdev, idx);
@@ -1014,8 +1013,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
     }
 
     if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
-        fprintf(stderr, "vhost backend memory slots limit is less"
-                " than current number of present memory slots\n");
+        error_report("vhost backend memory slots limit is less"
+                " than current number of present memory slots");
         r = -1;
         goto fail;
     }
@@ -1121,8 +1120,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;
     }
@@ -1131,7 +1131,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;
         }
     }
@@ -1142,8 +1142,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);
     }
@@ -1165,8 +1164,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 v3 12/28] vhost-user: check qemu_chr_fe_set_msgfds() return value

[marcandre.lureau]

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

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

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

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

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

[marcandre.lureau]

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

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

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

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

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

[Michael S. Tsirkin]

On Wed, Jul 06, 2016 at 08:47:06PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> More error checking to break code flow and report appropriate errors.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

So this will cause asserts here:

    /* 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);

which is the opposite of what we want.

For vhost_net ioctl failure is a fatal error bit for
vhost user this means backend exited and will not change
memory so it's benign.

the right thing to do is to return 0 status to caller on most errors.

If that is necessary for debugging, add a printout for now.


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

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

[Marc-André Lureau]

Hi

----- Original Message -----
> On Wed, Jul 06, 2016 at 08:47:06PM +0200, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > More error checking to break code flow and report appropriate errors.
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> So this will cause asserts here:
> 
>     /* 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);

That's why there is a previous patch "vhost: do not assert() on vhost_ops failure". Although there is still some abort() on migration. This is a corner case that isn't yet solved. I think it would make sense for the backend to state that it is disconnected, and for vhost to ingore migration ops failure in this case.

> which is the opposite of what we want.
> 
> For vhost_net ioctl failure is a fatal error bit for
> vhost user this means backend exited and will not change
> memory so it's benign.
> 
> the right thing to do is to return 0 status to caller on most errors.
> 
> If that is necessary for debugging, add a printout for now.

Yes, I am adding a VHOST_OPS_DEBUG as you proposed

> 
> > ---
> >  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 v3 14/28] qemu-char: check socket is actually connected

[marcandre.lureau]

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

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

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

diff --git a/qemu-char.c b/qemu-char.c
index 0698b98..c4fa779 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 v3 15/28] vhost-user: keep vhost_net after a disconnection

[marcandre.lureau]

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

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

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

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

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

[marcandre.lureau]

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

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

This reverts commit ecd34898596c60f79886061618dd7e01001113ad.

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

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

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

[marcandre.lureau]

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

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

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

Re: [Qemu-devel] [PATCH v3 17/28] get_vhost_net() should be != null after vhost_user_init

[Michael S. Tsirkin]

On Wed, Jul 06, 2016 at 08:47:10PM +0200, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  hw/net/vhost_net.c | 1 +
>  net/vhost-user.c   | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index f1dd367..22ea653 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -417,6 +417,7 @@ VHostNetState *get_vhost_net(NetClientState *nc)
>          break;
>      case NET_CLIENT_OPTIONS_KIND_VHOST_USER:
>          vhost_net = vhost_user_get_vhost_net(nc);
> +        assert(vhost_net != NULL);
>          break;
>      default:
>          break;
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 9ad7bcc..c9a5ed7 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -255,6 +255,8 @@ static int net_vhost_user_init(NetClientState *peer, const char *device,
>  
>      qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event, nc[0].name);
>  
> +    assert(s->vhost_net != NULL);
> +
>      return 0;
>  }
> 

I prefer just assert(s->vhost_net) please without != NULL.

 
> -- 
> 2.9.0

Re: [Qemu-devel] [PATCH v3 17/28] get_vhost_net() should be != null after vhost_user_init

[Marc-André Lureau]

Hi

----- Original Message -----
> On Wed, Jul 06, 2016 at 08:47:10PM +0200, marcandre.lureau@redhat.com wrote:
> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > 
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  hw/net/vhost_net.c | 1 +
> >  net/vhost-user.c   | 2 ++
> >  2 files changed, 3 insertions(+)
> > 
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index f1dd367..22ea653 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -417,6 +417,7 @@ VHostNetState *get_vhost_net(NetClientState *nc)
> >          break;
> >      case NET_CLIENT_OPTIONS_KIND_VHOST_USER:
> >          vhost_net = vhost_user_get_vhost_net(nc);
> > +        assert(vhost_net != NULL);
> >          break;
> >      default:
> >          break;
> > diff --git a/net/vhost-user.c b/net/vhost-user.c
> > index 9ad7bcc..c9a5ed7 100644
> > --- a/net/vhost-user.c
> > +++ b/net/vhost-user.c
> > @@ -255,6 +255,8 @@ static int net_vhost_user_init(NetClientState *peer,
> > const char *device,
> >  
> >      qemu_chr_add_handlers(chr, NULL, NULL, net_vhost_user_event,
> >      nc[0].name);
> >  
> > +    assert(s->vhost_net != NULL);
> > +
> >      return 0;
> >  }
> > 
> 
> I prefer just assert(s->vhost_net) please without != NULL.

ok (hopefully I didn't forget any)

> 
>  
> > --
> > 2.9.0
> 

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

[marcandre.lureau]

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

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

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

[Qemu-devel] [PATCH v3 19/28] 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 752a78f..4a183d3 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1188,6 +1188,9 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev, VirtIODevice *vdev, int n,
     int r, index = n - hdev->vq_index;
     struct vhost_vring_file file;
 
+    /* should only be called after backend is connected */
+    assert(hdev->vhost_ops != NULL);
+
     if (mask) {
         assert(vdev->use_guest_notifier_mask);
         file.fd = event_notifier_get_fd(&hdev->vqs[index].masked_notifier);
@@ -1234,6 +1237,9 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
 {
     int i, r;
 
+    /* should only be called after backend is connected */
+    assert(hdev->vhost_ops != NULL);
+
     hdev->started = true;
 
     r = vhost_dev_set_features(hdev, hdev->log_enabled);
@@ -1294,6 +1300,9 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
 {
     int i;
 
+    /* should only be called after backend is connected */
+    assert(hdev->vhost_ops != NULL);
+
     for (i = 0; i < hdev->nvqs; ++i) {
         vhost_virtqueue_stop(hdev,
                              vdev,
-- 
2.9.0

[Qemu-devel] [PATCH v3 20/28] 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 57df10a..15aa8ee 100644
--- a/include/sysemu/char.h
+++ b/include/sysemu/char.h
@@ -70,6 +70,7 @@ struct CharDriverState {
     int (*get_msgfds)(struct CharDriverState *s, int* fds, int num);
     int (*set_msgfds)(struct CharDriverState *s, int *fds, int num);
     int (*chr_add_client)(struct CharDriverState *chr, int fd);
+    int (*chr_wait_connected)(struct CharDriverState *chr, Error **errp);
     IOEventHandler *chr_event;
     IOCanReadHandler *chr_can_read;
     IOReadHandler *chr_read;
@@ -151,6 +152,13 @@ CharDriverState *qemu_chr_new(const char *label, const char *filename,
  */
 void qemu_chr_disconnect(CharDriverState *chr);
 
+/**
+ * @qemu_chr_wait_connected:
+ *
+ * Wait for characted backend to be connected.
+ */
+int qemu_chr_wait_connected(CharDriverState *chr, Error **errp);
+
 /**
  * @qemu_chr_new_noreplay:
  *
diff --git a/qemu-char.c b/qemu-char.c
index c4fa779..1daadc1 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 v3 21/28] 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 1daadc1..da7ea26 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 v3 22/28] 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 c9a5ed7..a49376e 100644
--- a/net/vhost-user.c
+++ b/net/vhost-user.c
@@ -24,6 +24,7 @@ typedef struct VhostUserState {
     VHostNetState *vhost_net;
     guint watch;
     uint64_t acked_features;
+    bool started;
 } VhostUserState;
 
 typedef struct VhostUserChardevProps {
@@ -216,6 +217,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);
@@ -234,7 +236,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;
 
@@ -243,6 +245,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);
@@ -253,7 +258,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 != NULL);
 
-- 
2.9.0

[Qemu-devel] [PATCH v3 23/28] 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 v3 24/28] 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 421d432..a0b91ab 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 v3 25/28] 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 a0b91ab..6595243 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 v3 26/28] 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 2010b11..3cb57b8 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -611,7 +611,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 6595243..aedbea6 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -16,6 +16,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>
 #include <qemu/sockets.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 v3 27/28] 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 v3 28/28] 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 bit nicer to provide an assert/error than to crash trying to call
it. 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 cc2f68a..c67f81e 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_OPTIONS_KIND_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_OPTIONS_KIND_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 4a183d3..2cb3c95 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1313,3 +1313,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 b60d758..4bc8ccd 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -85,4 +85,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