All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/11] block/export: convert vhost-user-blk-server to block exports API
@ 2020-09-22 16:03 Stefan Hajnoczi
  2020-09-22 16:03 ` [PATCH 01/11] block/export: shorten serial string to fit Stefan Hajnoczi
                   ` (10 more replies)
  0 siblings, 11 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 16:03 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Coiby Xu, mreitz,
	Stefan Hajnoczi

This patch series converts Coiby Xu's vhost-user-blk-server from a QOM object
to the block exports API. The block exports API provides a standard QMP and
command-line interface for managing block exports (NBD, FUSE, vhost-user-blk,
etc). A fair amount of init/shutdown code is removed because the block exports
API already takes care of that functionality.

Most of the patches are vhost-user-blk-server cleanups.

The syntax for launching qemu-storage-daemon is:

  $ qemu-storage-daemon \
      --blockdev file,node-name=drive0,filename=test.img \
      --export vhost-user-blk,node-name=drive0,id=export0,writable=on,unix-socket=/tmp/vhost-user-blk.sock

QEMU can connect to the vhost-user-blk export like this:

  $ qemu-system-x86_64 \
      -M accel=kvm,memory-backend=mem \
      -m 1G \
      -object memory-backend-memfd,size=1G,id=mem \
      -cpu host \
      -chardev socket,path=/tmp/vhost-user-blk.sock,id=char0 \
      -device vhost-user-blk-pci,chardev=char0

Based-on: 20200918080912.321299-1-coiby.xu@gmail.com ("[PATCH v10 0/7] vhost-user block device backend implementation")
Based-on: 20200907182011.521007-1-kwolf@redhat.com ("[PATCH 00/29] block/export: Add infrastructure and QAPI for block exports")

Stefan Hajnoczi (11):
  block/export: shorten serial string to fit
  util/vhost-user-server: s/fileds/fields/ typo fix
  util/vhost-user-server: drop unnecessary QOM cast
  util/vhost-user-server: drop unnecessary watch deletion
  block/export: consolidate request structs into VuBlockReq
  util/vhost-user-server: drop unused DevicePanicNotifier
  util/vhost-user-server: fix memory leak in vu_message_read()
  util/vhost-user-server: check EOF when reading payload
  util/vhost-user-server: rework vu_client_trip() coroutine lifecycle
  block/export: report flush errors
  block/export: convert vhost-user-blk server to block export API

 qapi/block-export.json               |  19 +-
 block/export/vhost-user-blk-server.h |  23 +-
 util/vhost-user-server.h             |  32 +-
 block/export/export.c                |   8 +-
 block/export/vhost-user-blk-server.c | 534 ++++++++-------------------
 util/vhost-user-server.c             | 322 ++++++++--------
 block/export/meson.build             |   1 +
 block/meson.build                    |   1 -
 8 files changed, 360 insertions(+), 580 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 14+ messages in thread
* [PATCH 01/11] block/export: shorten serial string to fit
  2020-09-22 16:03 [PATCH 00/11] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
@ 2020-09-22 16:03 ` Stefan Hajnoczi
  2020-09-22 16:03 ` [PATCH 02/11] util/vhost-user-server: s/fileds/fields/ typo fix Stefan Hajnoczi
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 16:03 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Coiby Xu, mreitz,
	Stefan Hajnoczi

The serial field is only 20 bytes long. Shorten the string so it fits.

This fixes the following compiler warning:

  ../block/export/vhost-user-blk-server.c:178:50: error: ‘%s’ directive output truncated writing 21 bytes into a region of size 20 [-Werror=format-truncation=]
  178 |         snprintf(elem->in_sg[0].iov_base, size, "%s", "vhost_user_blk_server");
      |                                                  ^~   ~~~~~~~~~~~~~~~~~~~~~~~

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/export/vhost-user-blk-server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index ec78130f09..fb7764a730 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -175,7 +175,7 @@ static void coroutine_fn vu_block_virtio_process_req(void *opaque)
     case VIRTIO_BLK_T_GET_ID: {
         size_t size = MIN(iov_size(&elem->in_sg[0], in_num),
                           VIRTIO_BLK_ID_BYTES);
-        snprintf(elem->in_sg[0].iov_base, size, "%s", "vhost_user_blk_server");
+        snprintf(elem->in_sg[0].iov_base, size, "%s", "vhost_user_blk");
         req->in->status = VIRTIO_BLK_S_OK;
         req->size = elem->in_sg[0].iov_len;
         break;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 02/11] util/vhost-user-server: s/fileds/fields/ typo fix
  2020-09-22 16:03 [PATCH 00/11] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
  2020-09-22 16:03 ` [PATCH 01/11] block/export: shorten serial string to fit Stefan Hajnoczi
@ 2020-09-22 16:03 ` Stefan Hajnoczi
  2020-09-22 16:03 ` [PATCH 03/11] util/vhost-user-server: drop unnecessary QOM cast Stefan Hajnoczi
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 16:03 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Coiby Xu, mreitz,
	Stefan Hajnoczi

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/vhost-user-server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index 7b50a2b1fd..2cd0cf8277 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -407,7 +407,7 @@ bool vhost_user_server_start(VuServer *server,
         return false;
     }
 
-    /* zero out unspecified fileds */
+    /* zero out unspecified fields */
     *server = (VuServer) {
         .listener              = listener,
         .vu_iface              = vu_iface,
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 03/11] util/vhost-user-server: drop unnecessary QOM cast
  2020-09-22 16:03 [PATCH 00/11] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
  2020-09-22 16:03 ` [PATCH 01/11] block/export: shorten serial string to fit Stefan Hajnoczi
  2020-09-22 16:03 ` [PATCH 02/11] util/vhost-user-server: s/fileds/fields/ typo fix Stefan Hajnoczi
@ 2020-09-22 16:03 ` Stefan Hajnoczi
  2020-09-22 16:03 ` [PATCH 04/11] util/vhost-user-server: drop unnecessary watch deletion Stefan Hajnoczi
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 16:03 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Coiby Xu, mreitz,
	Stefan Hajnoczi

We already have access to the value with the correct type (ioc and sioc
are the same QIOChannel).

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/vhost-user-server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index 2cd0cf8277..ebe47885f5 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -337,7 +337,7 @@ static void vu_accept(QIONetListener *listener, QIOChannelSocket *sioc,
     server->ioc = QIO_CHANNEL(sioc);
     object_ref(OBJECT(server->ioc));
     qio_channel_attach_aio_context(server->ioc, server->ctx);
-    qio_channel_set_blocking(QIO_CHANNEL(server->sioc), false, NULL);
+    qio_channel_set_blocking(server->ioc, false, NULL);
     vu_client_start(server);
 }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 04/11] util/vhost-user-server: drop unnecessary watch deletion
  2020-09-22 16:03 [PATCH 00/11] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
                   ` (2 preceding siblings ...)
  2020-09-22 16:03 ` [PATCH 03/11] util/vhost-user-server: drop unnecessary QOM cast Stefan Hajnoczi
@ 2020-09-22 16:03 ` Stefan Hajnoczi
  2020-09-22 16:03 ` [PATCH 05/11] block/export: consolidate request structs into VuBlockReq Stefan Hajnoczi
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 16:03 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Coiby Xu, mreitz,
	Stefan Hajnoczi

Explicitly deleting watches is not necessary since libvhost-user calls
remove_watch() during vu_deinit(). Add an assertion to check this
though.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/vhost-user-server.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index ebe47885f5..ebb850731b 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -48,21 +48,6 @@ static void close_client(VuServer *server)
     /* When this is set vu_client_trip will stop new processing vhost-user message */
     server->sioc = NULL;
 
-    VuFdWatch *vu_fd_watch, *next;
-    QTAILQ_FOREACH_SAFE(vu_fd_watch, &server->vu_fd_watches, next, next) {
-        aio_set_fd_handler(server->ioc->ctx, vu_fd_watch->fd, true, NULL,
-                           NULL, NULL, NULL);
-    }
-
-    while (!QTAILQ_EMPTY(&server->vu_fd_watches)) {
-        QTAILQ_FOREACH_SAFE(vu_fd_watch, &server->vu_fd_watches, next, next) {
-            if (!vu_fd_watch->processing) {
-                QTAILQ_REMOVE(&server->vu_fd_watches, vu_fd_watch, next);
-                g_free(vu_fd_watch);
-            }
-        }
-    }
-
     while (server->processing_msg) {
         if (server->ioc->read_coroutine) {
             server->ioc->read_coroutine = NULL;
@@ -73,6 +58,10 @@ static void close_client(VuServer *server)
     }
 
     vu_deinit(&server->vu_dev);
+
+    /* vu_deinit() should have called remove_watch() */
+    assert(QTAILQ_EMPTY(&server->vu_fd_watches));
+
     object_unref(OBJECT(sioc));
     object_unref(OBJECT(server->ioc));
 }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 05/11] block/export: consolidate request structs into VuBlockReq
  2020-09-22 16:03 [PATCH 00/11] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
                   ` (3 preceding siblings ...)
  2020-09-22 16:03 ` [PATCH 04/11] util/vhost-user-server: drop unnecessary watch deletion Stefan Hajnoczi
@ 2020-09-22 16:03 ` Stefan Hajnoczi
  2020-09-22 16:03 ` [PATCH 06/11] util/vhost-user-server: drop unused DevicePanicNotifier Stefan Hajnoczi
                   ` (5 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 16:03 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Coiby Xu, mreitz,
	Stefan Hajnoczi

Only one struct is needed per request. Drop req_data and the separate
VuBlockReq instance. Instead let vu_queue_pop() allocate everything at
once.

This fixes the req_data memory leak in vu_block_virtio_process_req().

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/export/vhost-user-blk-server.c | 68 +++++++++-------------------
 1 file changed, 21 insertions(+), 47 deletions(-)

diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index fb7764a730..ef07a87eb1 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -25,7 +25,7 @@ struct virtio_blk_inhdr {
 };
 
 typedef struct VuBlockReq {
-    VuVirtqElement *elem;
+    VuVirtqElement elem;
     int64_t sector_num;
     size_t size;
     struct virtio_blk_inhdr *in;
@@ -39,14 +39,10 @@ static void vu_block_req_complete(VuBlockReq *req)
     VuDev *vu_dev = &req->server->vu_dev;
 
     /* IO size with 1 extra status byte */
-    vu_queue_push(vu_dev, req->vq, req->elem, req->size + 1);
+    vu_queue_push(vu_dev, req->vq, &req->elem, req->size + 1);
     vu_queue_notify(vu_dev, req->vq);
 
-    if (req->elem) {
-        free(req->elem);
-    }
-
-    g_free(req);
+    free(req);
 }
 
 static VuBlockDev *get_vu_block_device_by_server(VuServer *server)
@@ -89,20 +85,12 @@ static void coroutine_fn vu_block_flush(VuBlockReq *req)
     blk_co_flush(backend);
 }
 
-struct req_data {
-    VuServer *server;
-    VuVirtq *vq;
-    VuVirtqElement *elem;
-};
-
 static void coroutine_fn vu_block_virtio_process_req(void *opaque)
 {
-    struct req_data *data = opaque;
-    VuServer *server = data->server;
-    VuVirtq *vq = data->vq;
-    VuVirtqElement *elem = data->elem;
+    VuBlockReq *req = opaque;
+    VuServer *server = req->server;
+    VuVirtqElement *elem = &req->elem;
     uint32_t type;
-    VuBlockReq *req;
 
     VuBlockDev *vdev_blk = get_vu_block_device_by_server(server);
     BlockBackend *backend = vdev_blk->backend;
@@ -111,18 +99,13 @@ static void coroutine_fn vu_block_virtio_process_req(void *opaque)
     struct iovec *out_iov = elem->out_sg;
     unsigned in_num = elem->in_num;
     unsigned out_num = elem->out_num;
+
     /* refer to hw/block/virtio_blk.c */
     if (elem->out_num < 1 || elem->in_num < 1) {
         error_report("virtio-blk request missing headers");
-        free(elem);
-        return;
+        goto err;
     }
 
-    req = g_new0(VuBlockReq, 1);
-    req->server = server;
-    req->vq = vq;
-    req->elem = elem;
-
     if (unlikely(iov_to_buf(out_iov, out_num, 0, &req->out,
                             sizeof(req->out)) != sizeof(req->out))) {
         error_report("virtio-blk request outhdr too short");
@@ -202,36 +185,27 @@ static void coroutine_fn vu_block_virtio_process_req(void *opaque)
 
 err:
     free(elem);
-    g_free(req);
-    return;
 }
 
 static void vu_block_process_vq(VuDev *vu_dev, int idx)
 {
-    VuServer *server;
-    VuVirtq *vq;
-    struct req_data *req_data;
+    VuServer *server = container_of(vu_dev, VuServer, vu_dev);
+    VuVirtq *vq = vu_get_queue(vu_dev, idx);
 
-    server = container_of(vu_dev, VuServer, vu_dev);
-    assert(server);
-
-    vq = vu_get_queue(vu_dev, idx);
-    assert(vq);
-    VuVirtqElement *elem;
     while (1) {
-        elem = vu_queue_pop(vu_dev, vq, sizeof(VuVirtqElement) +
-                                    sizeof(VuBlockReq));
-        if (elem) {
-            req_data = g_new0(struct req_data, 1);
-            req_data->server = server;
-            req_data->vq = vq;
-            req_data->elem = elem;
-            Coroutine *co = qemu_coroutine_create(vu_block_virtio_process_req,
-                                                  req_data);
-            aio_co_enter(server->ioc->ctx, co);
-        } else {
+        VuBlockReq *req;
+
+        req = vu_queue_pop(vu_dev, vq, sizeof(VuBlockReq));
+        if (!req) {
             break;
         }
+
+        req->server = server;
+        req->vq = vq;
+
+        Coroutine *co =
+            qemu_coroutine_create(vu_block_virtio_process_req, req);
+        qemu_coroutine_enter(co);
     }
 }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 06/11] util/vhost-user-server: drop unused DevicePanicNotifier
  2020-09-22 16:03 [PATCH 00/11] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
                   ` (4 preceding siblings ...)
  2020-09-22 16:03 ` [PATCH 05/11] block/export: consolidate request structs into VuBlockReq Stefan Hajnoczi
@ 2020-09-22 16:03 ` Stefan Hajnoczi
  2020-09-22 16:03 ` [PATCH 07/11] util/vhost-user-server: fix memory leak in vu_message_read() Stefan Hajnoczi
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 16:03 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Coiby Xu, mreitz,
	Stefan Hajnoczi

The device panic notifier callback is not used. Drop it.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/vhost-user-server.h             | 3 ---
 block/export/vhost-user-blk-server.c | 3 +--
 util/vhost-user-server.c             | 6 ------
 3 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/util/vhost-user-server.h b/util/vhost-user-server.h
index 5232f96718..92177fc911 100644
--- a/util/vhost-user-server.h
+++ b/util/vhost-user-server.h
@@ -29,12 +29,10 @@ typedef struct VuFdWatch {
 } VuFdWatch;
 
 typedef struct VuServer VuServer;
-typedef void DevicePanicNotifierFn(VuServer *server);
 
 struct VuServer {
     QIONetListener *listener;
     AioContext *ctx;
-    DevicePanicNotifierFn *device_panic_notifier;
     int max_queues;
     const VuDevIface *vu_iface;
     VuDev vu_dev;
@@ -54,7 +52,6 @@ bool vhost_user_server_start(VuServer *server,
                              SocketAddress *unix_socket,
                              AioContext *ctx,
                              uint16_t max_queues,
-                             DevicePanicNotifierFn *device_panic_notifier,
                              const VuDevIface *vu_iface,
                              Error **errp);
 
diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index ef07a87eb1..f2bfddbf26 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -439,8 +439,7 @@ static void vhost_user_blk_server_start(VuBlockDev *vu_block_device,
     ctx = bdrv_get_aio_context(blk_bs(vu_block_device->backend));
 
     if (!vhost_user_server_start(&vu_block_device->vu_server, addr, ctx,
-                                 VHOST_USER_BLK_MAX_QUEUES,
-                                 NULL, &vu_block_iface,
+                                 VHOST_USER_BLK_MAX_QUEUES, &vu_block_iface,
                                  errp)) {
         goto error;
     }
diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index ebb850731b..892815827d 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -81,10 +81,6 @@ static void panic_cb(VuDev *vu_dev, const char *buf)
         close_client(server);
     }
 
-    if (server->device_panic_notifier) {
-        server->device_panic_notifier(server);
-    }
-
     /*
      * Set the callback function for network listener so another
      * vhost-user client can connect to this server
@@ -385,7 +381,6 @@ bool vhost_user_server_start(VuServer *server,
                              SocketAddress *socket_addr,
                              AioContext *ctx,
                              uint16_t max_queues,
-                             DevicePanicNotifierFn *device_panic_notifier,
                              const VuDevIface *vu_iface,
                              Error **errp)
 {
@@ -402,7 +397,6 @@ bool vhost_user_server_start(VuServer *server,
         .vu_iface              = vu_iface,
         .max_queues            = max_queues,
         .ctx                   = ctx,
-        .device_panic_notifier = device_panic_notifier,
     };
 
     qio_net_listener_set_name(server->listener, "vhost-user-backend-listener");
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 07/11] util/vhost-user-server: fix memory leak in vu_message_read()
  2020-09-22 16:03 [PATCH 00/11] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
                   ` (5 preceding siblings ...)
  2020-09-22 16:03 ` [PATCH 06/11] util/vhost-user-server: drop unused DevicePanicNotifier Stefan Hajnoczi
@ 2020-09-22 16:03 ` Stefan Hajnoczi
  2020-09-22 16:03 ` [PATCH 08/11] util/vhost-user-server: check EOF when reading payload Stefan Hajnoczi
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 16:03 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Coiby Xu, mreitz,
	Stefan Hajnoczi

fds[] is leaked when qio_channel_readv_full() fails.

Use vmsg->fds[] instead of keeping a local fds[] array. Then we can
reuse goto fail to clean up fds. vmsg->fd_num must be zeroed before the
loop to make this safe.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/vhost-user-server.c | 50 ++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 27 deletions(-)

diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index 892815827d..5a60e2ca2a 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -100,21 +100,11 @@ vu_message_read(VuDev *vu_dev, int conn_fd, VhostUserMsg *vmsg)
     };
     int rc, read_bytes = 0;
     Error *local_err = NULL;
-    /*
-     * Store fds/nfds returned from qio_channel_readv_full into
-     * temporary variables.
-     *
-     * VhostUserMsg is a packed structure, gcc will complain about passing
-     * pointer to a packed structure member if we pass &VhostUserMsg.fd_num
-     * and &VhostUserMsg.fds directly when calling qio_channel_readv_full,
-     * thus two temporary variables nfds and fds are used here.
-     */
-    size_t nfds = 0, nfds_t = 0;
     const size_t max_fds = G_N_ELEMENTS(vmsg->fds);
-    int *fds_t = NULL;
     VuServer *server = container_of(vu_dev, VuServer, vu_dev);
     QIOChannel *ioc = server->ioc;
 
+    vmsg->fd_num = 0;
     if (!ioc) {
         error_report_err(local_err);
         goto fail;
@@ -122,41 +112,47 @@ vu_message_read(VuDev *vu_dev, int conn_fd, VhostUserMsg *vmsg)
 
     assert(qemu_in_coroutine());
     do {
+        size_t nfds = 0;
+        int *fds = NULL;
+
         /*
          * qio_channel_readv_full may have short reads, keeping calling it
          * until getting VHOST_USER_HDR_SIZE or 0 bytes in total
          */
-        rc = qio_channel_readv_full(ioc, &iov, 1, &fds_t, &nfds_t, &local_err);
+        rc = qio_channel_readv_full(ioc, &iov, 1, &fds, &nfds, &local_err);
         if (rc < 0) {
             if (rc == QIO_CHANNEL_ERR_BLOCK) {
+                assert(local_err == NULL);
                 qio_channel_yield(ioc, G_IO_IN);
                 continue;
             } else {
                 error_report_err(local_err);
-                return false;
+                goto fail;
             }
         }
-        read_bytes += rc;
-        if (nfds_t > 0) {
-            if (nfds + nfds_t > max_fds) {
+
+        if (nfds > 0) {
+            if (vmsg->fd_num + nfds > max_fds) {
                 error_report("A maximum of %zu fds are allowed, "
                              "however got %lu fds now",
-                             max_fds, nfds + nfds_t);
+                             max_fds, vmsg->fd_num + nfds);
+                g_free(fds);
                 goto fail;
             }
-            memcpy(vmsg->fds + nfds, fds_t,
-                   nfds_t *sizeof(vmsg->fds[0]));
-            nfds += nfds_t;
-            g_free(fds_t);
+            memcpy(vmsg->fds + vmsg->fd_num, fds, nfds * sizeof(vmsg->fds[0]));
+            vmsg->fd_num += nfds;
+            g_free(fds);
         }
-        if (read_bytes == VHOST_USER_HDR_SIZE || rc == 0) {
-            break;
+
+        if (rc == 0) { /* socket closed */
+            goto fail;
         }
-        iov.iov_base = (char *)vmsg + read_bytes;
-        iov.iov_len = VHOST_USER_HDR_SIZE - read_bytes;
-    } while (true);
 
-    vmsg->fd_num = nfds;
+        iov.iov_base += rc;
+        iov.iov_len -= rc;
+        read_bytes += rc;
+    } while (read_bytes != VHOST_USER_HDR_SIZE);
+
     /* qio_channel_readv_full will make socket fds blocking, unblock them */
     vmsg_unblock_fds(vmsg);
     if (vmsg->size > sizeof(vmsg->payload)) {
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 08/11] util/vhost-user-server: check EOF when reading payload
  2020-09-22 16:03 [PATCH 00/11] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
                   ` (6 preceding siblings ...)
  2020-09-22 16:03 ` [PATCH 07/11] util/vhost-user-server: fix memory leak in vu_message_read() Stefan Hajnoczi
@ 2020-09-22 16:03 ` Stefan Hajnoczi
  2020-09-22 16:03 ` [PATCH 09/11] util/vhost-user-server: rework vu_client_trip() coroutine lifecycle Stefan Hajnoczi
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 16:03 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Coiby Xu, mreitz,
	Stefan Hajnoczi

Unexpected EOF is an error that must be reported.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/vhost-user-server.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index 5a60e2ca2a..ec555abcb2 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -169,8 +169,10 @@ vu_message_read(VuDev *vu_dev, int conn_fd, VhostUserMsg *vmsg)
     };
     if (vmsg->size) {
         rc = qio_channel_readv_all_eof(ioc, &iov_payload, 1, &local_err);
-        if (rc == -1) {
-            error_report_err(local_err);
+        if (rc != 1) {
+            if (local_err) {
+                error_report_err(local_err);
+            }
             goto fail;
         }
     }
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 09/11] util/vhost-user-server: rework vu_client_trip() coroutine lifecycle
  2020-09-22 16:03 [PATCH 00/11] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
                   ` (7 preceding siblings ...)
  2020-09-22 16:03 ` [PATCH 08/11] util/vhost-user-server: check EOF when reading payload Stefan Hajnoczi
@ 2020-09-22 16:03 ` Stefan Hajnoczi
  2020-09-22 16:04 ` [PATCH 10/11] block/export: report flush errors Stefan Hajnoczi
  2020-09-22 16:04 ` [PATCH 11/11] block/export: convert vhost-user-blk server to block export API Stefan Hajnoczi
  10 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 16:03 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Coiby Xu, mreitz,
	Stefan Hajnoczi

The vu_client_trip() coroutine is leaked during AioContext switching. It
is also unsafe to destroy the vu_dev in panic_cb() since its callers
still access it in some cases.

Rework the lifecycle to solve these safety issues.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/vhost-user-server.h             |  29 ++--
 block/export/vhost-user-blk-server.c |   9 +-
 util/vhost-user-server.c             | 245 +++++++++++++++------------
 3 files changed, 155 insertions(+), 128 deletions(-)

diff --git a/util/vhost-user-server.h b/util/vhost-user-server.h
index 92177fc911..0da4c2cc4c 100644
--- a/util/vhost-user-server.h
+++ b/util/vhost-user-server.h
@@ -19,34 +19,36 @@
 #include "qapi/error.h"
 #include "standard-headers/linux/virtio_blk.h"
 
+/* A kick fd that we monitor on behalf of libvhost-user */
 typedef struct VuFdWatch {
     VuDev *vu_dev;
     int fd; /*kick fd*/
     void *pvt;
     vu_watch_cb cb;
-    bool processing;
     QTAILQ_ENTRY(VuFdWatch) next;
 } VuFdWatch;
 
-typedef struct VuServer VuServer;
-
-struct VuServer {
+/**
+ * VuServer:
+ * A vhost-user server instance with user-defined VuDevIface callbacks.
+ * Vhost-user device backends can be implemented using VuServer. VuDevIface
+ * callbacks and virtqueue kicks run in the given AioContext.
+ */
+typedef struct {
     QIONetListener *listener;
+    QEMUBH *restart_listener_bh;
     AioContext *ctx;
     int max_queues;
     const VuDevIface *vu_iface;
+
+    /* Protected by ctx lock */
     VuDev vu_dev;
     QIOChannel *ioc; /* The I/O channel with the client */
     QIOChannelSocket *sioc; /* The underlying data channel with the client */
-    /* IOChannel for fd provided via VHOST_USER_SET_SLAVE_REQ_FD */
-    QIOChannel *ioc_slave;
-    QIOChannelSocket *sioc_slave;
-    Coroutine *co_trip; /* coroutine for processing VhostUserMsg */
     QTAILQ_HEAD(, VuFdWatch) vu_fd_watches;
-    /* restart coroutine co_trip if AIOContext is changed */
-    bool aio_context_changed;
-    bool processing_msg;
-};
+
+    Coroutine *co_trip; /* coroutine for processing VhostUserMsg */
+} VuServer;
 
 bool vhost_user_server_start(VuServer *server,
                              SocketAddress *unix_socket,
@@ -57,6 +59,7 @@ bool vhost_user_server_start(VuServer *server,
 
 void vhost_user_server_stop(VuServer *server);
 
-void vhost_user_server_set_aio_context(VuServer *server, AioContext *ctx);
+void vhost_user_server_attach_aio_context(VuServer *server, AioContext *ctx);
+void vhost_user_server_detach_aio_context(VuServer *server);
 
 #endif /* VHOST_USER_SERVER_H */
diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index f2bfddbf26..b609a3e4d6 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -313,18 +313,13 @@ static const VuDevIface vu_block_iface = {
 static void blk_aio_attached(AioContext *ctx, void *opaque)
 {
     VuBlockDev *vub_dev = opaque;
-    aio_context_acquire(ctx);
-    vhost_user_server_set_aio_context(&vub_dev->vu_server, ctx);
-    aio_context_release(ctx);
+    vhost_user_server_attach_aio_context(&vub_dev->vu_server, ctx);
 }
 
 static void blk_aio_detach(void *opaque)
 {
     VuBlockDev *vub_dev = opaque;
-    AioContext *ctx = vub_dev->vu_server.ctx;
-    aio_context_acquire(ctx);
-    vhost_user_server_set_aio_context(&vub_dev->vu_server, NULL);
-    aio_context_release(ctx);
+    vhost_user_server_detach_aio_context(&vub_dev->vu_server);
 }
 
 static void
diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index ec555abcb2..d8b8c08b5f 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -9,8 +9,50 @@
  */
 #include "qemu/osdep.h"
 #include "qemu/main-loop.h"
+#include "block/aio-wait.h"
 #include "vhost-user-server.h"
 
+/*
+ * Theory of operation:
+ *
+ * VuServer is started and stopped by vhost_user_server_start() and
+ * vhost_user_server_stop() from the main loop thread. Starting the server
+ * opens a vhost-user UNIX domain socket and listens for incoming connections.
+ * Only one connection is allowed at a time.
+ *
+ * The connection is handled by the vu_client_trip() coroutine in the
+ * VuServer->ctx AioContext. The coroutine consists of a vu_dispatch() loop
+ * where libvhost-user calls vu_message_read() to receive the next vhost-user
+ * protocol messages over the UNIX domain socket.
+ *
+ * When virtqueues are set up libvhost-user calls set_watch() to monitor kick
+ * fds. These fds are also handled in the VuServer->ctx AioContext.
+ *
+ * Both vu_client_trip() and kick fd monitoring can be stopped by shutting down
+ * the socket connection. Shutting down the socket connection causes
+ * vu_message_read() to fail since no more data can be received from the socket.
+ * After vu_dispatch() fails, vu_client_trip() calls vu_deinit() to stop
+ * libvhost-user before terminating the coroutine. vu_deinit() calls
+ * remove_watch() to stop monitoring kick fds and this stops virtqueue
+ * processing.
+ *
+ * When vu_client_trip() has finished cleaning up it schedules a BH in the main
+ * loop thread to accept the next client connection.
+ *
+ * When libvhost-user detects an error it calls panic_cb() and sets the
+ * dev->broken flag. Both vu_client_trip() and kick fd processing stop when
+ * the dev->broken flag is set.
+ *
+ * It is possible to switch AioContexts using
+ * vhost_user_server_detach_aio_context() and
+ * vhost_user_server_attach_aio_context(). They stop monitoring fds in the old
+ * AioContext and resume monitoring in the new AioContext. The vu_client_trip()
+ * coroutine remains in a yielded state during the switch. This is made
+ * possible by QIOChannel's support for spurious coroutine re-entry in
+ * qio_channel_yield(). The coroutine will restart I/O when re-entered from the
+ * new AioContext.
+ */
+
 static void vmsg_close_fds(VhostUserMsg *vmsg)
 {
     int i;
@@ -27,68 +69,9 @@ static void vmsg_unblock_fds(VhostUserMsg *vmsg)
     }
 }
 
-static void vu_accept(QIONetListener *listener, QIOChannelSocket *sioc,
-                      gpointer opaque);
-
-static void close_client(VuServer *server)
-{
-    /*
-     * Before closing the client
-     *
-     * 1. Let vu_client_trip stop processing new vhost-user msg
-     *
-     * 2. remove kick_handler
-     *
-     * 3. wait for the kick handler to be finished
-     *
-     * 4. wait for the current vhost-user msg to be finished processing
-     */
-
-    QIOChannelSocket *sioc = server->sioc;
-    /* When this is set vu_client_trip will stop new processing vhost-user message */
-    server->sioc = NULL;
-
-    while (server->processing_msg) {
-        if (server->ioc->read_coroutine) {
-            server->ioc->read_coroutine = NULL;
-            qio_channel_set_aio_fd_handler(server->ioc, server->ioc->ctx, NULL,
-                                           NULL, server->ioc);
-            server->processing_msg = false;
-        }
-    }
-
-    vu_deinit(&server->vu_dev);
-
-    /* vu_deinit() should have called remove_watch() */
-    assert(QTAILQ_EMPTY(&server->vu_fd_watches));
-
-    object_unref(OBJECT(sioc));
-    object_unref(OBJECT(server->ioc));
-}
-
 static void panic_cb(VuDev *vu_dev, const char *buf)
 {
-    VuServer *server = container_of(vu_dev, VuServer, vu_dev);
-
-    /* avoid while loop in close_client */
-    server->processing_msg = false;
-
-    if (buf) {
-        error_report("vu_panic: %s", buf);
-    }
-
-    if (server->sioc) {
-        close_client(server);
-    }
-
-    /*
-     * Set the callback function for network listener so another
-     * vhost-user client can connect to this server
-     */
-    qio_net_listener_set_client_func(server->listener,
-                                     vu_accept,
-                                     server,
-                                     NULL);
+    error_report("vu_panic: %s", buf);
 }
 
 static bool coroutine_fn
@@ -185,28 +168,31 @@ fail:
     return false;
 }
 
-
-static void vu_client_start(VuServer *server);
 static coroutine_fn void vu_client_trip(void *opaque)
 {
     VuServer *server = opaque;
+    VuDev *vu_dev = &server->vu_dev;
 
-    while (!server->aio_context_changed && server->sioc) {
-        server->processing_msg = true;
-        vu_dispatch(&server->vu_dev);
-        server->processing_msg = false;
+    while (!vu_dev->broken && vu_dispatch(vu_dev)) {
+        /* Keep running */
     }
 
-    if (server->aio_context_changed && server->sioc) {
-        server->aio_context_changed = false;
-        vu_client_start(server);
-    }
-}
+    vu_deinit(vu_dev);
+
+    /* vu_deinit() should have called remove_watch() */
+    assert(QTAILQ_EMPTY(&server->vu_fd_watches));
+
+    object_unref(OBJECT(server->sioc));
+    server->sioc = NULL;
 
-static void vu_client_start(VuServer *server)
-{
-    server->co_trip = qemu_coroutine_create(vu_client_trip, server);
-    aio_co_enter(server->ctx, server->co_trip);
+    object_unref(OBJECT(server->ioc));
+    server->ioc = NULL;
+
+    server->co_trip = NULL;
+    if (server->restart_listener_bh) {
+        qemu_bh_schedule(server->restart_listener_bh);
+    }
+    aio_wait_kick();
 }
 
 /*
@@ -219,12 +205,18 @@ static void vu_client_start(VuServer *server)
 static void kick_handler(void *opaque)
 {
     VuFdWatch *vu_fd_watch = opaque;
-    vu_fd_watch->processing = true;
-    vu_fd_watch->cb(vu_fd_watch->vu_dev, 0, vu_fd_watch->pvt);
-    vu_fd_watch->processing = false;
+    VuDev *vu_dev = vu_fd_watch->vu_dev;
+
+    vu_fd_watch->cb(vu_dev, 0, vu_fd_watch->pvt);
+
+    /* Stop vu_client_trip() if an error occurred in vu_fd_watch->cb() */
+    if (vu_dev->broken) {
+        VuServer *server = container_of(vu_dev, VuServer, vu_dev);
+
+        qio_channel_shutdown(server->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+    }
 }
 
-
 static VuFdWatch *find_vu_fd_watch(VuServer *server, int fd)
 {
 
@@ -319,62 +311,95 @@ static void vu_accept(QIONetListener *listener, QIOChannelSocket *sioc,
     qio_channel_set_name(QIO_CHANNEL(sioc), "vhost-user client");
     server->ioc = QIO_CHANNEL(sioc);
     object_ref(OBJECT(server->ioc));
-    qio_channel_attach_aio_context(server->ioc, server->ctx);
+
+    /* TODO vu_message_write() spins if non-blocking! */
     qio_channel_set_blocking(server->ioc, false, NULL);
-    vu_client_start(server);
+
+    server->co_trip = qemu_coroutine_create(vu_client_trip, server);
+
+    aio_context_acquire(server->ctx);
+    vhost_user_server_attach_aio_context(server, server->ctx);
+    aio_context_release(server->ctx);
 }
 
-
 void vhost_user_server_stop(VuServer *server)
 {
+    aio_context_acquire(server->ctx);
+
+    qemu_bh_delete(server->restart_listener_bh);
+    server->restart_listener_bh = NULL;
+
     if (server->sioc) {
-        close_client(server);
+        VuFdWatch *vu_fd_watch;
+
+        QTAILQ_FOREACH(vu_fd_watch, &server->vu_fd_watches, next) {
+            aio_set_fd_handler(server->ctx, vu_fd_watch->fd, true,
+                               NULL, NULL, NULL, vu_fd_watch);
+        }
+
+        qio_channel_shutdown(server->ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
+
+        AIO_WAIT_WHILE(server->ctx, server->co_trip);
     }
 
+    aio_context_release(server->ctx);
+
     if (server->listener) {
         qio_net_listener_disconnect(server->listener);
         object_unref(OBJECT(server->listener));
     }
+}
+
+/*
+ * Allow the next client to connect to the server. Called from a BH in the main
+ * loop.
+ */
+static void restart_listener_bh(void *opaque)
+{
+    VuServer *server = opaque;
 
+    qio_net_listener_set_client_func(server->listener, vu_accept, server,
+                                     NULL);
 }
 
-void vhost_user_server_set_aio_context(VuServer *server, AioContext *ctx)
+/* Called with ctx acquired */
+void vhost_user_server_attach_aio_context(VuServer *server, AioContext *ctx)
 {
-    VuFdWatch *vu_fd_watch, *next;
-    void *opaque = NULL;
-    IOHandler *io_read = NULL;
-    bool attach;
+    VuFdWatch *vu_fd_watch;
 
-    server->ctx = ctx ? ctx : qemu_get_aio_context();
+    server->ctx = ctx;
 
     if (!server->sioc) {
-        /* not yet serving any client*/
         return;
     }
 
-    if (ctx) {
-        qio_channel_attach_aio_context(server->ioc, ctx);
-        server->aio_context_changed = true;
-        io_read = kick_handler;
-        attach = true;
-    } else {
+    qio_channel_attach_aio_context(server->ioc, ctx);
+
+    QTAILQ_FOREACH(vu_fd_watch, &server->vu_fd_watches, next) {
+        aio_set_fd_handler(ctx, vu_fd_watch->fd, true, kick_handler, NULL,
+                           NULL, vu_fd_watch);
+    }
+
+    aio_co_schedule(ctx, server->co_trip);
+}
+
+/* Called with server->ctx acquired */
+void vhost_user_server_detach_aio_context(VuServer *server)
+{
+    if (server->sioc) {
+        VuFdWatch *vu_fd_watch;
+
+        QTAILQ_FOREACH(vu_fd_watch, &server->vu_fd_watches, next) {
+            aio_set_fd_handler(server->ctx, vu_fd_watch->fd, true,
+                               NULL, NULL, NULL, vu_fd_watch);
+        }
+
         qio_channel_detach_aio_context(server->ioc);
-        /* server->ioc->ctx keeps the old AioConext */
-        ctx = server->ioc->ctx;
-        attach = false;
     }
 
-    QTAILQ_FOREACH_SAFE(vu_fd_watch, &server->vu_fd_watches, next, next) {
-        if (vu_fd_watch->cb) {
-            opaque = attach ? vu_fd_watch : NULL;
-            aio_set_fd_handler(ctx, vu_fd_watch->fd, true,
-                               io_read, NULL, NULL,
-                               opaque);
-        }
-    }
+    server->ctx = NULL;
 }
 
-
 bool vhost_user_server_start(VuServer *server,
                              SocketAddress *socket_addr,
                              AioContext *ctx,
@@ -382,6 +407,7 @@ bool vhost_user_server_start(VuServer *server,
                              const VuDevIface *vu_iface,
                              Error **errp)
 {
+    QEMUBH *bh;
     QIONetListener *listener = qio_net_listener_new();
     if (qio_net_listener_open_sync(listener, socket_addr, 1,
                                    errp) < 0) {
@@ -389,9 +415,12 @@ bool vhost_user_server_start(VuServer *server,
         return false;
     }
 
+    bh = qemu_bh_new(restart_listener_bh, server);
+
     /* zero out unspecified fields */
     *server = (VuServer) {
         .listener              = listener,
+        .restart_listener_bh   = bh,
         .vu_iface              = vu_iface,
         .max_queues            = max_queues,
         .ctx                   = ctx,
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 10/11] block/export: report flush errors
  2020-09-22 16:03 [PATCH 00/11] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
                   ` (8 preceding siblings ...)
  2020-09-22 16:03 ` [PATCH 09/11] util/vhost-user-server: rework vu_client_trip() coroutine lifecycle Stefan Hajnoczi
@ 2020-09-22 16:04 ` Stefan Hajnoczi
  2020-09-22 16:04 ` [PATCH 11/11] block/export: convert vhost-user-blk server to block export API Stefan Hajnoczi
  10 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 16:04 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Coiby Xu, mreitz,
	Stefan Hajnoczi

Propagate the flush return value since errors are possible.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/export/vhost-user-blk-server.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index b609a3e4d6..44d3c45fa2 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -78,11 +78,11 @@ vu_block_discard_write_zeroes(VuBlockReq *req, struct iovec *iov,
     return -EINVAL;
 }
 
-static void coroutine_fn vu_block_flush(VuBlockReq *req)
+static int coroutine_fn vu_block_flush(VuBlockReq *req)
 {
     VuBlockDev *vdev_blk = get_vu_block_device_by_server(req->server);
     BlockBackend *backend = vdev_blk->backend;
-    blk_co_flush(backend);
+    return blk_co_flush(backend);
 }
 
 static void coroutine_fn vu_block_virtio_process_req(void *opaque)
@@ -152,8 +152,11 @@ static void coroutine_fn vu_block_virtio_process_req(void *opaque)
         break;
     }
     case VIRTIO_BLK_T_FLUSH:
-        vu_block_flush(req);
-        req->in->status = VIRTIO_BLK_S_OK;
+        if (vu_block_flush(req) == 0) {
+            req->in->status = VIRTIO_BLK_S_OK;
+        } else {
+            req->in->status = VIRTIO_BLK_S_IOERR;
+        }
         break;
     case VIRTIO_BLK_T_GET_ID: {
         size_t size = MIN(iov_size(&elem->in_sg[0], in_num),
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH 11/11] block/export: convert vhost-user-blk server to block export API
  2020-09-22 16:03 [PATCH 00/11] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
                   ` (9 preceding siblings ...)
  2020-09-22 16:04 ` [PATCH 10/11] block/export: report flush errors Stefan Hajnoczi
@ 2020-09-22 16:04 ` Stefan Hajnoczi
  2020-09-23 13:42   ` Markus Armbruster
  10 siblings, 1 reply; 14+ messages in thread
From: Stefan Hajnoczi @ 2020-09-22 16:04 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, qemu-block, Markus Armbruster, Coiby Xu, mreitz,
	Stefan Hajnoczi

Use the new QAPI block exports API instead of defining our own QOM
objects.

This is a large change because the lifecycle of VuBlockDev needs to
follow BlockExportDriver. QOM properties are replaced by QAPI options
objects.

VuBlockDev is renamed VuBlkExport and contains a BlockExport field.
Several fields can be dropped since BlockExport already has equivalents.

The file names and meson build integration will be adjusted in a future
patch. libvhost-user should probably be built as a static library that
is linked into QEMU instead of as a .c file that results in duplicate
compilation.

The new command-line syntax is:

  $ qemu-storage-daemon \
      --blockdev file,node-name=drive0,filename=test.img \
      --export vhost-user-blk,node-name=drive0,id=export0,unix-socket=/tmp/vhost-user-blk.sock

Note that unix-socket is optional because we may wish to accept chardevs
too in the future.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 qapi/block-export.json               |  19 +-
 block/export/vhost-user-blk-server.h |  23 +-
 block/export/export.c                |   8 +-
 block/export/vhost-user-blk-server.c | 461 ++++++++-------------------
 block/export/meson.build             |   1 +
 block/meson.build                    |   1 -
 6 files changed, 156 insertions(+), 357 deletions(-)

diff --git a/qapi/block-export.json b/qapi/block-export.json
index ace0d66e17..840dcbe833 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -84,6 +84,19 @@
   'data': { '*name': 'str', '*description': 'str',
             '*bitmap': 'str' } }
 
+##
+# @BlockExportOptionsVhostUserBlk:
+#
+# A vhost-user-blk block export.
+#
+# @unix-socket: Path where the vhost-user UNIX domain socket will be created.
+# @logical-block-size: Logical block size in bytes.
+#
+# Since: 5.2
+##
+{ 'struct': 'BlockExportOptionsVhostUserBlk',
+  'data': { '*unix-socket': 'str', '*logical-block-size': 'size' } }
+
 ##
 # @NbdServerAddOptions:
 #
@@ -180,11 +193,12 @@
 # An enumeration of block export types
 #
 # @nbd: NBD export
+# @vhost-user-blk: vhost-user-blk export (since 5.2)
 #
 # Since: 4.2
 ##
 { 'enum': 'BlockExportType',
-  'data': [ 'nbd' ] }
+  'data': [ 'nbd', 'vhost-user-blk' ] }
 
 ##
 # @BlockExportOptions:
@@ -213,7 +227,8 @@
             '*writethrough': 'bool' },
   'discriminator': 'type',
   'data': {
-      'nbd': 'BlockExportOptionsNbd'
+      'nbd': 'BlockExportOptionsNbd',
+      'vhost-user-blk': 'BlockExportOptionsVhostUserBlk'
    } }
 
 ##
diff --git a/block/export/vhost-user-blk-server.h b/block/export/vhost-user-blk-server.h
index f06f37c4c8..fcf46fc8a5 100644
--- a/block/export/vhost-user-blk-server.h
+++ b/block/export/vhost-user-blk-server.h
@@ -10,27 +10,10 @@
 
 #ifndef VHOST_USER_BLK_SERVER_H
 #define VHOST_USER_BLK_SERVER_H
-#include "util/vhost-user-server.h"
 
-typedef struct VuBlockDev VuBlockDev;
-#define TYPE_VHOST_USER_BLK_SERVER "vhost-user-blk-server"
-#define VHOST_USER_BLK_SERVER(obj) \
-   OBJECT_CHECK(VuBlockDev, obj, TYPE_VHOST_USER_BLK_SERVER)
+#include "block/export.h"
 
-/* vhost user block device */
-struct VuBlockDev {
-    Object parent_obj;
-    char *node_name;
-    SocketAddress *addr;
-    AioContext *ctx;
-    VuServer vu_server;
-    bool running;
-    uint32_t blk_size;
-    BlockBackend *backend;
-    QIOChannelSocket *sioc;
-    QTAILQ_ENTRY(VuBlockDev) next;
-    struct virtio_blk_config blkcfg;
-    bool writable;
-};
+/* For block/export/export.c */
+extern const BlockExportDriver blk_exp_vhost_user_blk;
 
 #endif /* VHOST_USER_BLK_SERVER_H */
diff --git a/block/export/export.c b/block/export/export.c
index 87516d1e05..d0c7590911 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -17,6 +17,9 @@
 #include "sysemu/block-backend.h"
 #include "block/export.h"
 #include "block/nbd.h"
+#if CONFIG_LINUX
+#include "block/export/vhost-user-blk-server.h"
+#endif
 #include "qapi/error.h"
 #include "qapi/qapi-commands-block-export.h"
 #include "qapi/qapi-events-block-export.h"
@@ -24,6 +27,9 @@
 
 static const BlockExportDriver *blk_exp_drivers[] = {
     &blk_exp_nbd,
+#if CONFIG_LINUX
+    &blk_exp_vhost_user_blk,
+#endif
 };
 
 /* Only accessed from the main thread */
@@ -123,7 +129,7 @@ BlockExport *blk_exp_add(BlockExportOptions *export, Error **errp)
     assert(drv->instance_size >= sizeof(BlockExport));
     exp = g_malloc0(drv->instance_size);
     *exp = (BlockExport) {
-        .drv        = &blk_exp_nbd,
+        .drv        = drv,
         .refcount   = 1,
         .user_owned = true,
         .id         = g_strdup(export->id),
diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index 44d3c45fa2..9908b3287e 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -11,6 +11,9 @@
  */
 #include "qemu/osdep.h"
 #include "block/block.h"
+#include "contrib/libvhost-user/libvhost-user.h"
+#include "standard-headers/linux/virtio_blk.h"
+#include "util/vhost-user-server.h"
 #include "vhost-user-blk-server.h"
 #include "qapi/error.h"
 #include "qom/object_interfaces.h"
@@ -24,7 +27,7 @@ struct virtio_blk_inhdr {
     unsigned char status;
 };
 
-typedef struct VuBlockReq {
+typedef struct VuBlkReq {
     VuVirtqElement elem;
     int64_t sector_num;
     size_t size;
@@ -32,9 +35,19 @@ typedef struct VuBlockReq {
     struct virtio_blk_outhdr out;
     VuServer *server;
     struct VuVirtq *vq;
-} VuBlockReq;
+} VuBlkReq;
 
-static void vu_block_req_complete(VuBlockReq *req)
+/* vhost user block device */
+typedef struct {
+    BlockExport export;
+    VuServer vu_server;
+    uint32_t blk_size;
+    QIOChannelSocket *sioc;
+    struct virtio_blk_config blkcfg;
+    bool writable;
+} VuBlkExport;
+
+static void vu_blk_req_complete(VuBlkReq *req)
 {
     VuDev *vu_dev = &req->server->vu_dev;
 
@@ -45,14 +58,9 @@ static void vu_block_req_complete(VuBlockReq *req)
     free(req);
 }
 
-static VuBlockDev *get_vu_block_device_by_server(VuServer *server)
-{
-    return container_of(server, VuBlockDev, vu_server);
-}
-
 static int coroutine_fn
-vu_block_discard_write_zeroes(VuBlockReq *req, struct iovec *iov,
-                              uint32_t iovcnt, uint32_t type)
+vu_blk_discard_write_zeroes(BlockBackend *blk, struct iovec *iov,
+                            uint32_t iovcnt, uint32_t type)
 {
     struct virtio_blk_discard_write_zeroes desc;
     ssize_t size = iov_to_buf(iov, iovcnt, 0, &desc, sizeof(desc));
@@ -61,16 +69,14 @@ vu_block_discard_write_zeroes(VuBlockReq *req, struct iovec *iov,
         return -EINVAL;
     }
 
-    VuBlockDev *vdev_blk = get_vu_block_device_by_server(req->server);
     uint64_t range[2] = { le64_to_cpu(desc.sector) << 9,
                           le32_to_cpu(desc.num_sectors) << 9 };
     if (type == VIRTIO_BLK_T_DISCARD) {
-        if (blk_co_pdiscard(vdev_blk->backend, range[0], range[1]) == 0) {
+        if (blk_co_pdiscard(blk, range[0], range[1]) == 0) {
             return 0;
         }
     } else if (type == VIRTIO_BLK_T_WRITE_ZEROES) {
-        if (blk_co_pwrite_zeroes(vdev_blk->backend,
-                                 range[0], range[1], 0) == 0) {
+        if (blk_co_pwrite_zeroes(blk, range[0], range[1], 0) == 0) {
             return 0;
         }
     }
@@ -78,22 +84,15 @@ vu_block_discard_write_zeroes(VuBlockReq *req, struct iovec *iov,
     return -EINVAL;
 }
 
-static int coroutine_fn vu_block_flush(VuBlockReq *req)
+static void coroutine_fn vu_blk_virtio_process_req(void *opaque)
 {
-    VuBlockDev *vdev_blk = get_vu_block_device_by_server(req->server);
-    BlockBackend *backend = vdev_blk->backend;
-    return blk_co_flush(backend);
-}
-
-static void coroutine_fn vu_block_virtio_process_req(void *opaque)
-{
-    VuBlockReq *req = opaque;
+    VuBlkReq *req = opaque;
     VuServer *server = req->server;
     VuVirtqElement *elem = &req->elem;
     uint32_t type;
 
-    VuBlockDev *vdev_blk = get_vu_block_device_by_server(server);
-    BlockBackend *backend = vdev_blk->backend;
+    VuBlkExport *vexp = container_of(server, VuBlkExport, vu_server);
+    BlockBackend *blk = vexp->export.blk;
 
     struct iovec *in_iov = elem->in_sg;
     struct iovec *out_iov = elem->out_sg;
@@ -133,16 +132,19 @@ static void coroutine_fn vu_block_virtio_process_req(void *opaque)
         bool is_write = type & VIRTIO_BLK_T_OUT;
         req->sector_num = le64_to_cpu(req->out.sector);
 
-        int64_t offset = req->sector_num * vdev_blk->blk_size;
+        if (is_write && !vexp->writable) {
+            req->in->status = VIRTIO_BLK_S_IOERR;
+            break;
+        }
+
+        int64_t offset = req->sector_num * vexp->blk_size;
         QEMUIOVector qiov;
         if (is_write) {
             qemu_iovec_init_external(&qiov, out_iov, out_num);
-            ret = blk_co_pwritev(backend, offset, qiov.size,
-                                 &qiov, 0);
+            ret = blk_co_pwritev(blk, offset, qiov.size, &qiov, 0);
         } else {
             qemu_iovec_init_external(&qiov, in_iov, in_num);
-            ret = blk_co_preadv(backend, offset, qiov.size,
-                                &qiov, 0);
+            ret = blk_co_preadv(blk, offset, qiov.size, &qiov, 0);
         }
         if (ret >= 0) {
             req->in->status = VIRTIO_BLK_S_OK;
@@ -152,7 +154,7 @@ static void coroutine_fn vu_block_virtio_process_req(void *opaque)
         break;
     }
     case VIRTIO_BLK_T_FLUSH:
-        if (vu_block_flush(req) == 0) {
+        if (blk_co_flush(blk) == 0) {
             req->in->status = VIRTIO_BLK_S_OK;
         } else {
             req->in->status = VIRTIO_BLK_S_IOERR;
@@ -169,8 +171,13 @@ static void coroutine_fn vu_block_virtio_process_req(void *opaque)
     case VIRTIO_BLK_T_DISCARD:
     case VIRTIO_BLK_T_WRITE_ZEROES: {
         int rc;
-        rc = vu_block_discard_write_zeroes(req, &elem->out_sg[1],
-                                           out_num, type);
+
+        if (!vexp->writable) {
+            req->in->status = VIRTIO_BLK_S_IOERR;
+            break;
+        }
+
+        rc = vu_blk_discard_write_zeroes(blk, &elem->out_sg[1], out_num, type);
         if (rc == 0) {
             req->in->status = VIRTIO_BLK_S_OK;
         } else {
@@ -183,22 +190,22 @@ static void coroutine_fn vu_block_virtio_process_req(void *opaque)
         break;
     }
 
-    vu_block_req_complete(req);
+    vu_blk_req_complete(req);
     return;
 
 err:
-    free(elem);
+    free(req);
 }
 
-static void vu_block_process_vq(VuDev *vu_dev, int idx)
+static void vu_blk_process_vq(VuDev *vu_dev, int idx)
 {
     VuServer *server = container_of(vu_dev, VuServer, vu_dev);
     VuVirtq *vq = vu_get_queue(vu_dev, idx);
 
     while (1) {
-        VuBlockReq *req;
+        VuBlkReq *req;
 
-        req = vu_queue_pop(vu_dev, vq, sizeof(VuBlockReq));
+        req = vu_queue_pop(vu_dev, vq, sizeof(VuBlkReq));
         if (!req) {
             break;
         }
@@ -207,26 +214,26 @@ static void vu_block_process_vq(VuDev *vu_dev, int idx)
         req->vq = vq;
 
         Coroutine *co =
-            qemu_coroutine_create(vu_block_virtio_process_req, req);
+            qemu_coroutine_create(vu_blk_virtio_process_req, req);
         qemu_coroutine_enter(co);
     }
 }
 
-static void vu_block_queue_set_started(VuDev *vu_dev, int idx, bool started)
+static void vu_blk_queue_set_started(VuDev *vu_dev, int idx, bool started)
 {
     VuVirtq *vq;
 
     assert(vu_dev);
 
     vq = vu_get_queue(vu_dev, idx);
-    vu_set_queue_handler(vu_dev, vq, started ? vu_block_process_vq : NULL);
+    vu_set_queue_handler(vu_dev, vq, started ? vu_blk_process_vq : NULL);
 }
 
-static uint64_t vu_block_get_features(VuDev *dev)
+static uint64_t vu_blk_get_features(VuDev *dev)
 {
     uint64_t features;
     VuServer *server = container_of(dev, VuServer, vu_dev);
-    VuBlockDev *vdev_blk = get_vu_block_device_by_server(server);
+    VuBlkExport *vexp = container_of(server, VuBlkExport, vu_server);
     features = 1ull << VIRTIO_BLK_F_SIZE_MAX |
                1ull << VIRTIO_BLK_F_SEG_MAX |
                1ull << VIRTIO_BLK_F_TOPOLOGY |
@@ -240,35 +247,35 @@ static uint64_t vu_block_get_features(VuDev *dev)
                1ull << VIRTIO_RING_F_EVENT_IDX |
                1ull << VHOST_USER_F_PROTOCOL_FEATURES;
 
-    if (!vdev_blk->writable) {
+    if (!vexp->writable) {
         features |= 1ull << VIRTIO_BLK_F_RO;
     }
 
     return features;
 }
 
-static uint64_t vu_block_get_protocol_features(VuDev *dev)
+static uint64_t vu_blk_get_protocol_features(VuDev *dev)
 {
     return 1ull << VHOST_USER_PROTOCOL_F_CONFIG |
            1ull << VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD;
 }
 
 static int
-vu_block_get_config(VuDev *vu_dev, uint8_t *config, uint32_t len)
+vu_blk_get_config(VuDev *vu_dev, uint8_t *config, uint32_t len)
 {
+    /* TODO blkcfg must be little-endian for VIRTIO 1.0 */
     VuServer *server = container_of(vu_dev, VuServer, vu_dev);
-    VuBlockDev *vdev_blk = get_vu_block_device_by_server(server);
-    memcpy(config, &vdev_blk->blkcfg, len);
-
+    VuBlkExport *vexp = container_of(server, VuBlkExport, vu_server);
+    memcpy(config, &vexp->blkcfg, len);
     return 0;
 }
 
 static int
-vu_block_set_config(VuDev *vu_dev, const uint8_t *data,
+vu_blk_set_config(VuDev *vu_dev, const uint8_t *data,
                     uint32_t offset, uint32_t size, uint32_t flags)
 {
     VuServer *server = container_of(vu_dev, VuServer, vu_dev);
-    VuBlockDev *vdev_blk = get_vu_block_device_by_server(server);
+    VuBlkExport *vexp = container_of(server, VuBlkExport, vu_server);
     uint8_t wce;
 
     /* don't support live migration */
@@ -282,8 +289,8 @@ vu_block_set_config(VuDev *vu_dev, const uint8_t *data,
     }
 
     wce = *data;
-    vdev_blk->blkcfg.wce = wce;
-    blk_set_enable_write_cache(vdev_blk->backend, wce);
+    vexp->blkcfg.wce = wce;
+    blk_set_enable_write_cache(vexp->export.blk, wce);
     return 0;
 }
 
@@ -295,7 +302,7 @@ vu_block_set_config(VuDev *vu_dev, const uint8_t *data,
  * of vu_process_message.
  *
  */
-static int vu_block_process_msg(VuDev *dev, VhostUserMsg *vmsg, int *do_reply)
+static int vu_blk_process_msg(VuDev *dev, VhostUserMsg *vmsg, int *do_reply)
 {
     if (vmsg->request == VHOST_USER_NONE) {
         dev->panic(dev, "disconnect");
@@ -304,29 +311,29 @@ static int vu_block_process_msg(VuDev *dev, VhostUserMsg *vmsg, int *do_reply)
     return false;
 }
 
-static const VuDevIface vu_block_iface = {
-    .get_features          = vu_block_get_features,
-    .queue_set_started     = vu_block_queue_set_started,
-    .get_protocol_features = vu_block_get_protocol_features,
-    .get_config            = vu_block_get_config,
-    .set_config            = vu_block_set_config,
-    .process_msg           = vu_block_process_msg,
+static const VuDevIface vu_blk_iface = {
+    .get_features          = vu_blk_get_features,
+    .queue_set_started     = vu_blk_queue_set_started,
+    .get_protocol_features = vu_blk_get_protocol_features,
+    .get_config            = vu_blk_get_config,
+    .set_config            = vu_blk_set_config,
+    .process_msg           = vu_blk_process_msg,
 };
 
 static void blk_aio_attached(AioContext *ctx, void *opaque)
 {
-    VuBlockDev *vub_dev = opaque;
-    vhost_user_server_attach_aio_context(&vub_dev->vu_server, ctx);
+    VuBlkExport *vexp = opaque;
+    vhost_user_server_attach_aio_context(&vexp->vu_server, ctx);
 }
 
 static void blk_aio_detach(void *opaque)
 {
-    VuBlockDev *vub_dev = opaque;
-    vhost_user_server_detach_aio_context(&vub_dev->vu_server);
+    VuBlkExport *vexp = opaque;
+    vhost_user_server_detach_aio_context(&vexp->vu_server);
 }
 
 static void
-vu_block_initialize_config(BlockDriverState *bs,
+vu_blk_initialize_config(BlockDriverState *bs,
                            struct virtio_blk_config *config, uint32_t blk_size)
 {
     config->capacity = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
@@ -343,290 +350,78 @@ vu_block_initialize_config(BlockDriverState *bs,
     config->max_write_zeroes_seg = 1;
 }
 
-static VuBlockDev *vu_block_init(VuBlockDev *vu_block_device, Error **errp)
+static void vu_blk_exp_request_shutdown(BlockExport *exp)
 {
+    VuBlkExport *vexp = container_of(exp, VuBlkExport, export);
 
-    BlockBackend *blk;
-    Error *local_error = NULL;
-    const char *node_name = vu_block_device->node_name;
-    bool writable = vu_block_device->writable;
-    uint64_t perm = BLK_PERM_CONSISTENT_READ;
-    int ret;
-
-    AioContext *ctx;
-
-    BlockDriverState *bs = bdrv_lookup_bs(node_name, node_name, &local_error);
-
-    if (!bs) {
-        error_propagate(errp, local_error);
-        return NULL;
-    }
-
-    if (bdrv_is_read_only(bs)) {
-        writable = false;
-    }
-
-    if (writable) {
-        perm |= BLK_PERM_WRITE;
-    }
-
-    ctx = bdrv_get_aio_context(bs);
-    aio_context_acquire(ctx);
-    bdrv_invalidate_cache(bs, NULL);
-    aio_context_release(ctx);
-
-    /*
-     * Don't allow resize while the vhost user server is running,
-     * otherwise we don't care what happens with the node.
-     */
-    blk = blk_new(bdrv_get_aio_context(bs), perm,
-                  BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
-                  BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD);
-    ret = blk_insert_bs(blk, bs, errp);
-
-    if (ret < 0) {
-        goto fail;
-    }
-
-    blk_set_enable_write_cache(blk, false);
-
-    blk_set_allow_aio_context_change(blk, true);
-
-    vu_block_device->blkcfg.wce = 0;
-    vu_block_device->backend = blk;
-    if (!vu_block_device->blk_size) {
-        vu_block_device->blk_size = BDRV_SECTOR_SIZE;
-    }
-    vu_block_device->blkcfg.blk_size = vu_block_device->blk_size;
-    blk_set_guest_block_size(blk, vu_block_device->blk_size);
-    vu_block_initialize_config(bs, &vu_block_device->blkcfg,
-                                   vu_block_device->blk_size);
-    return vu_block_device;
-
-fail:
-    blk_unref(blk);
-    return NULL;
-}
-
-static void vu_block_deinit(VuBlockDev *vu_block_device)
-{
-    if (vu_block_device->backend) {
-        blk_remove_aio_context_notifier(vu_block_device->backend, blk_aio_attached,
-                                        blk_aio_detach, vu_block_device);
-    }
-
-    blk_unref(vu_block_device->backend);
-}
-
-static void vhost_user_blk_server_stop(VuBlockDev *vu_block_device)
-{
-    vhost_user_server_stop(&vu_block_device->vu_server);
-    vu_block_deinit(vu_block_device);
-}
-
-static void vhost_user_blk_server_start(VuBlockDev *vu_block_device,
-                                        Error **errp)
-{
-    AioContext *ctx;
-    SocketAddress *addr = vu_block_device->addr;
-
-    if (!vu_block_init(vu_block_device, errp)) {
-        return;
-    }
-
-    ctx = bdrv_get_aio_context(blk_bs(vu_block_device->backend));
-
-    if (!vhost_user_server_start(&vu_block_device->vu_server, addr, ctx,
-                                 VHOST_USER_BLK_MAX_QUEUES, &vu_block_iface,
-                                 errp)) {
-        goto error;
-    }
-
-    blk_add_aio_context_notifier(vu_block_device->backend, blk_aio_attached,
-                                 blk_aio_detach, vu_block_device);
-    vu_block_device->running = true;
-    return;
-
- error:
-    vu_block_deinit(vu_block_device);
-}
-
-static bool vu_prop_modifiable(VuBlockDev *vus, Error **errp)
-{
-    if (vus->running) {
-            error_setg(errp, "The property can't be modified "
-                       "while the server is running");
-            return false;
-    }
-    return true;
-}
-
-static void vu_set_node_name(Object *obj, const char *value, Error **errp)
-{
-    VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
-
-    if (!vu_prop_modifiable(vus, errp)) {
-        return;
-    }
-
-    if (vus->node_name) {
-        g_free(vus->node_name);
-    }
-
-    vus->node_name = g_strdup(value);
-}
-
-static char *vu_get_node_name(Object *obj, Error **errp)
-{
-    VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
-    return g_strdup(vus->node_name);
-}
-
-static void free_socket_addr(SocketAddress *addr)
-{
-        g_free(addr->u.q_unix.path);
-        g_free(addr);
-}
-
-static void vu_set_unix_socket(Object *obj, const char *value,
-                               Error **errp)
-{
-    VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
-
-    if (!vu_prop_modifiable(vus, errp)) {
-        return;
-    }
-
-    if (vus->addr) {
-        free_socket_addr(vus->addr);
-    }
-
-    SocketAddress *addr = g_new0(SocketAddress, 1);
-    addr->type = SOCKET_ADDRESS_TYPE_UNIX;
-    addr->u.q_unix.path = g_strdup(value);
-    vus->addr = addr;
+    vhost_user_server_stop(&vexp->vu_server);
 }
 
-static char *vu_get_unix_socket(Object *obj, Error **errp)
+static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
+                             Error **errp)
 {
-    VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
-    return g_strdup(vus->addr->u.q_unix.path);
-}
-
-static bool vu_get_block_writable(Object *obj, Error **errp)
-{
-    VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
-    return vus->writable;
-}
-
-static void vu_set_block_writable(Object *obj, bool value, Error **errp)
-{
-    VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
-
-    if (!vu_prop_modifiable(vus, errp)) {
-            return;
-    }
-
-    vus->writable = value;
-}
-
-static void vu_get_blk_size(Object *obj, Visitor *v, const char *name,
-                            void *opaque, Error **errp)
-{
-    VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
-    uint32_t value = vus->blk_size;
-
-    visit_type_uint32(v, name, &value, errp);
-}
-
-static void vu_set_blk_size(Object *obj, Visitor *v, const char *name,
-                            void *opaque, Error **errp)
-{
-    VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
-
+    VuBlkExport *vexp = container_of(exp, VuBlkExport, export);
+    BlockExportOptionsVhostUserBlk *vu_opts = &opts->u.vhost_user_blk;
+    SocketAddress addr = {
+        .type = SOCKET_ADDRESS_TYPE_UNIX,
+        .u.q_unix.path = vu_opts->has_unix_socket ?
+                         vu_opts->unix_socket :
+                         NULL,
+    };
     Error *local_err = NULL;
-    uint32_t value;
+    uint64_t logical_block_size;
 
-    if (!vu_prop_modifiable(vus, errp)) {
-            return;
+    if (!vu_opts->has_unix_socket) {
+        error_setg(errp, "Missing unix-socket path to listen on");
+        return -EINVAL;
     }
 
-    visit_type_uint32(v, name, &value, &local_err);
-    if (local_err) {
-        goto out;
-    }
+    vexp->writable = opts->writable;
+    vexp->blkcfg.wce = 0;
 
-    check_block_size(object_get_typename(obj), name, value, &local_err);
+    if (vu_opts->has_logical_block_size) {
+        logical_block_size = vu_opts->logical_block_size;
+    } else {
+        logical_block_size = BDRV_SECTOR_SIZE;
+    }
+    check_block_size(exp->id, "logical-block-size", logical_block_size,
+                     &local_err);
     if (local_err) {
-        goto out;
+        error_propagate(errp, local_err);
+        return -EINVAL;
+    }
+    vexp->blk_size = logical_block_size;
+    blk_set_guest_block_size(exp->blk, logical_block_size);
+    vu_blk_initialize_config(blk_bs(exp->blk), &vexp->blkcfg,
+                               logical_block_size);
+
+    blk_set_allow_aio_context_change(exp->blk, true);
+    blk_add_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
+                                 vexp);
+
+    if (!vhost_user_server_start(&vexp->vu_server, &addr, exp->ctx,
+                                 VHOST_USER_BLK_MAX_QUEUES, &vu_blk_iface,
+                                 errp)) {
+        blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
+                                        blk_aio_detach, vexp);
+        return -EADDRNOTAVAIL;
     }
 
-    vus->blk_size = value;
-
-out:
-    error_propagate(errp, local_err);
-}
-
-static void vhost_user_blk_server_instance_finalize(Object *obj)
-{
-    VuBlockDev *vub = VHOST_USER_BLK_SERVER(obj);
-
-    vhost_user_blk_server_stop(vub);
-
-    /*
-     * Unlike object_property_add_str, object_class_property_add_str
-     * doesn't have a release method. Thus manual memory freeing is
-     * needed.
-     */
-    free_socket_addr(vub->addr);
-    g_free(vub->node_name);
-}
-
-static void vhost_user_blk_server_complete(UserCreatable *obj, Error **errp)
-{
-    VuBlockDev *vub = VHOST_USER_BLK_SERVER(obj);
-
-    vhost_user_blk_server_start(vub, errp);
+    return 0;
 }
 
-static void vhost_user_blk_server_class_init(ObjectClass *klass,
-                                             void *class_data)
+static void vu_blk_exp_delete(BlockExport *exp)
 {
-    UserCreatableClass *ucc = USER_CREATABLE_CLASS(klass);
-    ucc->complete = vhost_user_blk_server_complete;
-
-    object_class_property_add_bool(klass, "writable",
-                                   vu_get_block_writable,
-                                   vu_set_block_writable);
-
-    object_class_property_add_str(klass, "node-name",
-                                  vu_get_node_name,
-                                  vu_set_node_name);
-
-    object_class_property_add_str(klass, "unix-socket",
-                                  vu_get_unix_socket,
-                                  vu_set_unix_socket);
+    VuBlkExport *vexp = container_of(exp, VuBlkExport, export);
 
-    object_class_property_add(klass, "logical-block-size", "uint32",
-                              vu_get_blk_size, vu_set_blk_size,
-                              NULL, NULL);
+    blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
+                                    vexp);
 }
 
-static const TypeInfo vhost_user_blk_server_info = {
-    .name = TYPE_VHOST_USER_BLK_SERVER,
-    .parent = TYPE_OBJECT,
-    .instance_size = sizeof(VuBlockDev),
-    .instance_finalize = vhost_user_blk_server_instance_finalize,
-    .class_init = vhost_user_blk_server_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        {TYPE_USER_CREATABLE},
-        {}
-    },
+const BlockExportDriver blk_exp_vhost_user_blk = {
+    .type               = BLOCK_EXPORT_TYPE_VHOST_USER_BLK,
+    .instance_size      = sizeof(VuBlkExport),
+    .create             = vu_blk_exp_create,
+    .delete             = vu_blk_exp_delete,
+    .request_shutdown   = vu_blk_exp_request_shutdown,
 };
-
-static void vhost_user_blk_server_register_types(void)
-{
-    type_register_static(&vhost_user_blk_server_info);
-}
-
-type_init(vhost_user_blk_server_register_types)
diff --git a/block/export/meson.build b/block/export/meson.build
index 558ef35d38..ef3a9576f7 100644
--- a/block/export/meson.build
+++ b/block/export/meson.build
@@ -1 +1,2 @@
 block_ss.add(files('export.c'))
+block_ss.add(when: 'CONFIG_LINUX', if_true: files('vhost-user-blk-server.c', '../../contrib/libvhost-user/libvhost-user.c'))
diff --git a/block/meson.build b/block/meson.build
index cd52104c84..0b38dc36f7 100644
--- a/block/meson.build
+++ b/block/meson.build
@@ -60,7 +60,6 @@ block_ss.add(when: 'CONFIG_WIN32', if_true: files('file-win32.c', 'win32-aio.c')
 block_ss.add(when: 'CONFIG_POSIX', if_true: [files('file-posix.c'), coref, iokit])
 block_ss.add(when: 'CONFIG_LIBISCSI', if_true: files('iscsi-opts.c'))
 block_ss.add(when: 'CONFIG_LINUX', if_true: files('nvme.c'))
-block_ss.add(when: 'CONFIG_LINUX', if_true: files('export/vhost-user-blk-server.c', '../contrib/libvhost-user/libvhost-user.c'))
 block_ss.add(when: 'CONFIG_REPLICATION', if_true: files('replication.c'))
 block_ss.add(when: 'CONFIG_SHEEPDOG', if_true: files('sheepdog.c'))
 block_ss.add(when: ['CONFIG_LINUX_AIO', libaio], if_true: files('linux-aio.c'))
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH 11/11] block/export: convert vhost-user-blk server to block export API
  2020-09-22 16:04 ` [PATCH 11/11] block/export: convert vhost-user-blk server to block export API Stefan Hajnoczi
@ 2020-09-23 13:42   ` Markus Armbruster
  2020-09-23 18:29     ` Stefan Hajnoczi
  0 siblings, 1 reply; 14+ messages in thread
From: Markus Armbruster @ 2020-09-23 13:42 UTC (permalink / raw)
  To: Stefan Hajnoczi; +Cc: Kevin Wolf, Coiby Xu, qemu-devel, qemu-block, mreitz

Stefan Hajnoczi <stefanha@redhat.com> writes:

> Use the new QAPI block exports API instead of defining our own QOM
> objects.
>
> This is a large change because the lifecycle of VuBlockDev needs to
> follow BlockExportDriver. QOM properties are replaced by QAPI options
> objects.
>
> VuBlockDev is renamed VuBlkExport and contains a BlockExport field.
> Several fields can be dropped since BlockExport already has equivalents.
>
> The file names and meson build integration will be adjusted in a future
> patch. libvhost-user should probably be built as a static library that
> is linked into QEMU instead of as a .c file that results in duplicate
> compilation.
>
> The new command-line syntax is:
>
>   $ qemu-storage-daemon \
>       --blockdev file,node-name=drive0,filename=test.img \
>       --export vhost-user-blk,node-name=drive0,id=export0,unix-socket=/tmp/vhost-user-blk.sock
>
> Note that unix-socket is optional because we may wish to accept chardevs
> too in the future.

It's optional in the QAPI schema, but the code cosunming the --export
appears to require it.

There is no need to make it optional now just in case: Changing an
option parameter from mandatory to optional is backward-compatible.

> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  qapi/block-export.json               |  19 +-
>  block/export/vhost-user-blk-server.h |  23 +-
>  block/export/export.c                |   8 +-
>  block/export/vhost-user-blk-server.c | 461 ++++++++-------------------
>  block/export/meson.build             |   1 +
>  block/meson.build                    |   1 -
>  6 files changed, 156 insertions(+), 357 deletions(-)
>
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index ace0d66e17..840dcbe833 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -84,6 +84,19 @@
>    'data': { '*name': 'str', '*description': 'str',
>              '*bitmap': 'str' } }
>  
> +##
> +# @BlockExportOptionsVhostUserBlk:
> +#
> +# A vhost-user-blk block export.
> +#
> +# @unix-socket: Path where the vhost-user UNIX domain socket will be created.
> +# @logical-block-size: Logical block size in bytes.
> +#
> +# Since: 5.2
> +##
> +{ 'struct': 'BlockExportOptionsVhostUserBlk',
> +  'data': { '*unix-socket': 'str', '*logical-block-size': 'size' } }

This is where we make @unix-socket optional.

Default behavior is not documented.

> +
>  ##
>  # @NbdServerAddOptions:
>  #
> @@ -180,11 +193,12 @@
>  # An enumeration of block export types
>  #
>  # @nbd: NBD export
> +# @vhost-user-blk: vhost-user-blk export (since 5.2)
>  #
>  # Since: 4.2
>  ##
>  { 'enum': 'BlockExportType',
> -  'data': [ 'nbd' ] }
> +  'data': [ 'nbd', 'vhost-user-blk' ] }
>  
>  ##
>  # @BlockExportOptions:
> @@ -213,7 +227,8 @@
>              '*writethrough': 'bool' },
>    'discriminator': 'type',
>    'data': {
> -      'nbd': 'BlockExportOptionsNbd'
> +      'nbd': 'BlockExportOptionsNbd',
> +      'vhost-user-blk': 'BlockExportOptionsVhostUserBlk'
>     } }
>  
>  ##
[...]
> diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
> index 44d3c45fa2..9908b3287e 100644
> --- a/block/export/vhost-user-blk-server.c
> +++ b/block/export/vhost-user-blk-server.c
[...]
> -static char *vu_get_unix_socket(Object *obj, Error **errp)
> +static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
> +                             Error **errp)
>  {
> -    VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
> -    return g_strdup(vus->addr->u.q_unix.path);
> -}
> -
> -static bool vu_get_block_writable(Object *obj, Error **errp)
> -{
> -    VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
> -    return vus->writable;
> -}
> -
> -static void vu_set_block_writable(Object *obj, bool value, Error **errp)
> -{
> -    VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
> -
> -    if (!vu_prop_modifiable(vus, errp)) {
> -            return;
> -    }
> -
> -    vus->writable = value;
> -}
> -
> -static void vu_get_blk_size(Object *obj, Visitor *v, const char *name,
> -                            void *opaque, Error **errp)
> -{
> -    VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
> -    uint32_t value = vus->blk_size;
> -
> -    visit_type_uint32(v, name, &value, errp);
> -}
> -
> -static void vu_set_blk_size(Object *obj, Visitor *v, const char *name,
> -                            void *opaque, Error **errp)
> -{
> -    VuBlockDev *vus = VHOST_USER_BLK_SERVER(obj);
> -
> +    VuBlkExport *vexp = container_of(exp, VuBlkExport, export);
> +    BlockExportOptionsVhostUserBlk *vu_opts = &opts->u.vhost_user_blk;
> +    SocketAddress addr = {
> +        .type = SOCKET_ADDRESS_TYPE_UNIX,
> +        .u.q_unix.path = vu_opts->has_unix_socket ?
> +                         vu_opts->unix_socket :
> +                         NULL,
> +    };
>      Error *local_err = NULL;
> -    uint32_t value;
> +    uint64_t logical_block_size;
>  
> -    if (!vu_prop_modifiable(vus, errp)) {
> -            return;
> +    if (!vu_opts->has_unix_socket) {
> +        error_setg(errp, "Missing unix-socket path to listen on");
> +        return -EINVAL;
>      }

This is where we require @unix-socket.

>  
> -    visit_type_uint32(v, name, &value, &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> +    vexp->writable = opts->writable;
> +    vexp->blkcfg.wce = 0;
>  
> -    check_block_size(object_get_typename(obj), name, value, &local_err);
> +    if (vu_opts->has_logical_block_size) {
> +        logical_block_size = vu_opts->logical_block_size;
> +    } else {
> +        logical_block_size = BDRV_SECTOR_SIZE;
> +    }
> +    check_block_size(exp->id, "logical-block-size", logical_block_size,
> +                     &local_err);
>      if (local_err) {
> -        goto out;
> +        error_propagate(errp, local_err);
> +        return -EINVAL;
> +    }
> +    vexp->blk_size = logical_block_size;
> +    blk_set_guest_block_size(exp->blk, logical_block_size);
> +    vu_blk_initialize_config(blk_bs(exp->blk), &vexp->blkcfg,
> +                               logical_block_size);
> +
> +    blk_set_allow_aio_context_change(exp->blk, true);
> +    blk_add_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
> +                                 vexp);
> +
> +    if (!vhost_user_server_start(&vexp->vu_server, &addr, exp->ctx,
> +                                 VHOST_USER_BLK_MAX_QUEUES, &vu_blk_iface,
> +                                 errp)) {
> +        blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
> +                                        blk_aio_detach, vexp);
> +        return -EADDRNOTAVAIL;
>      }
>  
> -    vus->blk_size = value;
> -
> -out:
> -    error_propagate(errp, local_err);
> -}
> -
> -static void vhost_user_blk_server_instance_finalize(Object *obj)
> -{
> -    VuBlockDev *vub = VHOST_USER_BLK_SERVER(obj);
> -
> -    vhost_user_blk_server_stop(vub);
> -
> -    /*
> -     * Unlike object_property_add_str, object_class_property_add_str
> -     * doesn't have a release method. Thus manual memory freeing is
> -     * needed.
> -     */
> -    free_socket_addr(vub->addr);
> -    g_free(vub->node_name);
> -}
> -
> -static void vhost_user_blk_server_complete(UserCreatable *obj, Error **errp)
> -{
> -    VuBlockDev *vub = VHOST_USER_BLK_SERVER(obj);
> -
> -    vhost_user_blk_server_start(vub, errp);
> +    return 0;
>  }
[...]



^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH 11/11] block/export: convert vhost-user-blk server to block export API
  2020-09-23 13:42   ` Markus Armbruster
@ 2020-09-23 18:29     ` Stefan Hajnoczi
  0 siblings, 0 replies; 14+ messages in thread
From: Stefan Hajnoczi @ 2020-09-23 18:29 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Kevin Wolf, Coiby Xu, qemu-devel, qemu-block, mreitz

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

On Wed, Sep 23, 2020 at 03:42:30PM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> 
> > Use the new QAPI block exports API instead of defining our own QOM
> > objects.
> >
> > This is a large change because the lifecycle of VuBlockDev needs to
> > follow BlockExportDriver. QOM properties are replaced by QAPI options
> > objects.
> >
> > VuBlockDev is renamed VuBlkExport and contains a BlockExport field.
> > Several fields can be dropped since BlockExport already has equivalents.
> >
> > The file names and meson build integration will be adjusted in a future
> > patch. libvhost-user should probably be built as a static library that
> > is linked into QEMU instead of as a .c file that results in duplicate
> > compilation.
> >
> > The new command-line syntax is:
> >
> >   $ qemu-storage-daemon \
> >       --blockdev file,node-name=drive0,filename=test.img \
> >       --export vhost-user-blk,node-name=drive0,id=export0,unix-socket=/tmp/vhost-user-blk.sock
> >
> > Note that unix-socket is optional because we may wish to accept chardevs
> > too in the future.
> 
> It's optional in the QAPI schema, but the code cosunming the --export
> appears to require it.
> 
> There is no need to make it optional now just in case: Changing an
> option parameter from mandatory to optional is backward-compatible.

Good point, it should be mandatory.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-09-23 18:42 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-22 16:03 [PATCH 00/11] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
2020-09-22 16:03 ` [PATCH 01/11] block/export: shorten serial string to fit Stefan Hajnoczi
2020-09-22 16:03 ` [PATCH 02/11] util/vhost-user-server: s/fileds/fields/ typo fix Stefan Hajnoczi
2020-09-22 16:03 ` [PATCH 03/11] util/vhost-user-server: drop unnecessary QOM cast Stefan Hajnoczi
2020-09-22 16:03 ` [PATCH 04/11] util/vhost-user-server: drop unnecessary watch deletion Stefan Hajnoczi
2020-09-22 16:03 ` [PATCH 05/11] block/export: consolidate request structs into VuBlockReq Stefan Hajnoczi
2020-09-22 16:03 ` [PATCH 06/11] util/vhost-user-server: drop unused DevicePanicNotifier Stefan Hajnoczi
2020-09-22 16:03 ` [PATCH 07/11] util/vhost-user-server: fix memory leak in vu_message_read() Stefan Hajnoczi
2020-09-22 16:03 ` [PATCH 08/11] util/vhost-user-server: check EOF when reading payload Stefan Hajnoczi
2020-09-22 16:03 ` [PATCH 09/11] util/vhost-user-server: rework vu_client_trip() coroutine lifecycle Stefan Hajnoczi
2020-09-22 16:04 ` [PATCH 10/11] block/export: report flush errors Stefan Hajnoczi
2020-09-22 16:04 ` [PATCH 11/11] block/export: convert vhost-user-blk server to block export API Stefan Hajnoczi
2020-09-23 13:42   ` Markus Armbruster
2020-09-23 18:29     ` Stefan Hajnoczi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.