* [PATCH v2 01/13] block/export: shorten serial string to fit
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
@ 2020-09-24 15:15 ` Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 02/13] util/vhost-user-server: s/fileds/fields/ typo fix Stefan Hajnoczi
` (12 subsequent siblings)
13 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-24 15:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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] 21+ messages in thread* [PATCH v2 02/13] util/vhost-user-server: s/fileds/fields/ typo fix
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 01/13] block/export: shorten serial string to fit Stefan Hajnoczi
@ 2020-09-24 15:15 ` Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 03/13] util/vhost-user-server: drop unnecessary QOM cast Stefan Hajnoczi
` (11 subsequent siblings)
13 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-24 15:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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] 21+ messages in thread* [PATCH v2 03/13] util/vhost-user-server: drop unnecessary QOM cast
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 01/13] block/export: shorten serial string to fit Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 02/13] util/vhost-user-server: s/fileds/fields/ typo fix Stefan Hajnoczi
@ 2020-09-24 15:15 ` Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 04/13] util/vhost-user-server: drop unnecessary watch deletion Stefan Hajnoczi
` (10 subsequent siblings)
13 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-24 15:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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] 21+ messages in thread* [PATCH v2 04/13] util/vhost-user-server: drop unnecessary watch deletion
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
` (2 preceding siblings ...)
2020-09-24 15:15 ` [PATCH v2 03/13] util/vhost-user-server: drop unnecessary QOM cast Stefan Hajnoczi
@ 2020-09-24 15:15 ` Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 05/13] block/export: consolidate request structs into VuBlockReq Stefan Hajnoczi
` (9 subsequent siblings)
13 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-24 15:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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] 21+ messages in thread* [PATCH v2 05/13] block/export: consolidate request structs into VuBlockReq
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
` (3 preceding siblings ...)
2020-09-24 15:15 ` [PATCH v2 04/13] util/vhost-user-server: drop unnecessary watch deletion Stefan Hajnoczi
@ 2020-09-24 15:15 ` Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 06/13] util/vhost-user-server: drop unused DevicePanicNotifier Stefan Hajnoczi
` (8 subsequent siblings)
13 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-24 15:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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] 21+ messages in thread* [PATCH v2 06/13] util/vhost-user-server: drop unused DevicePanicNotifier
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
` (4 preceding siblings ...)
2020-09-24 15:15 ` [PATCH v2 05/13] block/export: consolidate request structs into VuBlockReq Stefan Hajnoczi
@ 2020-09-24 15:15 ` Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 07/13] util/vhost-user-server: fix memory leak in vu_message_read() Stefan Hajnoczi
` (7 subsequent siblings)
13 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-24 15:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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] 21+ messages in thread* [PATCH v2 07/13] util/vhost-user-server: fix memory leak in vu_message_read()
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
` (5 preceding siblings ...)
2020-09-24 15:15 ` [PATCH v2 06/13] util/vhost-user-server: drop unused DevicePanicNotifier Stefan Hajnoczi
@ 2020-09-24 15:15 ` Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 08/13] util/vhost-user-server: check EOF when reading payload Stefan Hajnoczi
` (6 subsequent siblings)
13 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-24 15:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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] 21+ messages in thread* [PATCH v2 08/13] util/vhost-user-server: check EOF when reading payload
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
` (6 preceding siblings ...)
2020-09-24 15:15 ` [PATCH v2 07/13] util/vhost-user-server: fix memory leak in vu_message_read() Stefan Hajnoczi
@ 2020-09-24 15:15 ` Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 09/13] util/vhost-user-server: rework vu_client_trip() coroutine lifecycle Stefan Hajnoczi
` (5 subsequent siblings)
13 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-24 15:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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] 21+ messages in thread* [PATCH v2 09/13] util/vhost-user-server: rework vu_client_trip() coroutine lifecycle
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
` (7 preceding siblings ...)
2020-09-24 15:15 ` [PATCH v2 08/13] util/vhost-user-server: check EOF when reading payload Stefan Hajnoczi
@ 2020-09-24 15:15 ` Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 10/13] block/export: report flush errors Stefan Hajnoczi
` (4 subsequent siblings)
13 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-24 15:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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] 21+ messages in thread* [PATCH v2 10/13] block/export: report flush errors
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
` (8 preceding siblings ...)
2020-09-24 15:15 ` [PATCH v2 09/13] util/vhost-user-server: rework vu_client_trip() coroutine lifecycle Stefan Hajnoczi
@ 2020-09-24 15:15 ` Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API Stefan Hajnoczi
` (3 subsequent siblings)
13 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-24 15:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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] 21+ messages in thread* [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
` (9 preceding siblings ...)
2020-09-24 15:15 ` [PATCH v2 10/13] block/export: report flush errors Stefan Hajnoczi
@ 2020-09-24 15:15 ` Stefan Hajnoczi
2020-09-30 5:28 ` Markus Armbruster
2020-09-24 15:15 ` [PATCH v2 12/13] util/vhost-user-server: move header to include/ Stefan Hajnoczi
` (2 subsequent siblings)
13 siblings, 1 reply; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-24 15:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
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>
---
v2:
* Replace str unix-socket with SocketAddress addr to match NBD and
support file descriptor passing
* Make addr mandatory [Markus]
* Update vhost-user-blk-test.c to use --export syntax
---
qapi/block-export.json | 21 +-
block/export/vhost-user-blk-server.h | 23 +-
block/export/export.c | 8 +-
block/export/vhost-user-blk-server.c | 452 +++++++--------------------
tests/qtest/vhost-user-blk-test.c | 2 +-
util/vhost-user-server.c | 10 +-
block/export/meson.build | 1 +
block/meson.build | 1 -
8 files changed, 158 insertions(+), 360 deletions(-)
diff --git a/qapi/block-export.json b/qapi/block-export.json
index ace0d66e17..2e44625bb1 100644
--- a/qapi/block-export.json
+++ b/qapi/block-export.json
@@ -84,6 +84,21 @@
'data': { '*name': 'str', '*description': 'str',
'*bitmap': 'str' } }
+##
+# @BlockExportOptionsVhostUserBlk:
+#
+# A vhost-user-blk block export.
+#
+# @addr: The vhost-user socket on which to listen. Both 'unix' and 'fd'
+# SocketAddress types are supported. Passed fds must be UNIX domain
+# sockets.
+# @logical-block-size: Logical block size in bytes. Defaults to 512 bytes.
+#
+# Since: 5.2
+##
+{ 'struct': 'BlockExportOptionsVhostUserBlk',
+ 'data': { 'addr': 'SocketAddress', '*logical-block-size': 'size' } }
+
##
# @NbdServerAddOptions:
#
@@ -180,11 +195,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 +229,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..91fc7040b2 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,67 @@ 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;
Error *local_err = NULL;
- uint32_t value;
+ uint64_t logical_block_size;
- if (!vu_prop_modifiable(vus, errp)) {
- return;
- }
+ vexp->writable = opts->writable;
+ vexp->blkcfg.wce = 0;
- visit_type_uint32(v, name, &value, &local_err);
- if (local_err) {
- goto out;
+ 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(object_get_typename(obj), name, value, &local_err);
+ 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, vu_opts->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/tests/qtest/vhost-user-blk-test.c b/tests/qtest/vhost-user-blk-test.c
index d4ccac6b54..42e4cfde82 100644
--- a/tests/qtest/vhost-user-blk-test.c
+++ b/tests/qtest/vhost-user-blk-test.c
@@ -674,7 +674,7 @@ static char *start_vhost_user_blk(GString *cmd_line, int vus_instances)
img_path = drive_create();
g_string_append_printf(storage_daemon_command,
"--blockdev driver=file,node-name=disk%d,filename=%s "
- "--object vhost-user-blk-server,id=disk%d,unix-socket=%s,"
+ "--export type=vhost-user-blk,id=disk%d,addr.type=unix,addr.path=%s,"
"node-name=disk%i,writable=on ",
i, img_path, i, sock_path, i);
diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index d8b8c08b5f..2a27139eb8 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -408,7 +408,15 @@ bool vhost_user_server_start(VuServer *server,
Error **errp)
{
QEMUBH *bh;
- QIONetListener *listener = qio_net_listener_new();
+ QIONetListener *listener;
+
+ if (socket_addr->type != SOCKET_ADDRESS_TYPE_UNIX &&
+ socket_addr->type != SOCKET_ADDRESS_TYPE_FD) {
+ error_setg(errp, "Only socket address types 'unix' and 'fd' are supported");
+ return false;
+ }
+
+ listener = qio_net_listener_new();
if (qio_net_listener_open_sync(listener, socket_addr, 1,
errp) < 0) {
object_unref(OBJECT(listener));
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] 21+ messages in thread* Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
2020-09-24 15:15 ` [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API Stefan Hajnoczi
@ 2020-09-30 5:28 ` Markus Armbruster
2020-09-30 9:45 ` Stefan Hajnoczi
0 siblings, 1 reply; 21+ messages in thread
From: Markus Armbruster @ 2020-09-30 5:28 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block, qemu-devel,
Coiby Xu, Max Reitz, Paolo Bonzini
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.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> v2:
> * Replace str unix-socket with SocketAddress addr to match NBD and
> support file descriptor passing
> * Make addr mandatory [Markus]
> * Update vhost-user-blk-test.c to use --export syntax
> ---
> qapi/block-export.json | 21 +-
> block/export/vhost-user-blk-server.h | 23 +-
> block/export/export.c | 8 +-
> block/export/vhost-user-blk-server.c | 452 +++++++--------------------
> tests/qtest/vhost-user-blk-test.c | 2 +-
> util/vhost-user-server.c | 10 +-
> block/export/meson.build | 1 +
> block/meson.build | 1 -
> 8 files changed, 158 insertions(+), 360 deletions(-)
>
> diff --git a/qapi/block-export.json b/qapi/block-export.json
> index ace0d66e17..2e44625bb1 100644
> --- a/qapi/block-export.json
> +++ b/qapi/block-export.json
> @@ -84,6 +84,21 @@
> 'data': { '*name': 'str', '*description': 'str',
> '*bitmap': 'str' } }
>
> +##
> +# @BlockExportOptionsVhostUserBlk:
> +#
> +# A vhost-user-blk block export.
> +#
> +# @addr: The vhost-user socket on which to listen. Both 'unix' and 'fd'
> +# SocketAddress types are supported. Passed fds must be UNIX domain
> +# sockets.
"addr.type must be 'unix' or 'fd'" is not visible in introspection.
Awkward. Practical problem only if other addresses ever become
available here. Is that possible?
> +# @logical-block-size: Logical block size in bytes. Defaults to 512 bytes.
> +#
> +# Since: 5.2
> +##
> +{ 'struct': 'BlockExportOptionsVhostUserBlk',
> + 'data': { 'addr': 'SocketAddress', '*logical-block-size': 'size' } }
> +
> ##
> # @NbdServerAddOptions:
> #
> @@ -180,11 +195,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 +229,8 @@
> '*writethrough': 'bool' },
> 'discriminator': 'type',
> 'data': {
> - 'nbd': 'BlockExportOptionsNbd'
> + 'nbd': 'BlockExportOptionsNbd',
> + 'vhost-user-blk': 'BlockExportOptionsVhostUserBlk'
> } }
>
> ##
[...]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
2020-09-30 5:28 ` Markus Armbruster
@ 2020-09-30 9:45 ` Stefan Hajnoczi
2020-09-30 14:33 ` Markus Armbruster
0 siblings, 1 reply; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-30 9:45 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block, qemu-devel,
Coiby Xu, Max Reitz, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 3998 bytes --]
On Wed, Sep 30, 2020 at 07:28:58AM +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.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > v2:
> > * Replace str unix-socket with SocketAddress addr to match NBD and
> > support file descriptor passing
> > * Make addr mandatory [Markus]
> > * Update vhost-user-blk-test.c to use --export syntax
> > ---
> > qapi/block-export.json | 21 +-
> > block/export/vhost-user-blk-server.h | 23 +-
> > block/export/export.c | 8 +-
> > block/export/vhost-user-blk-server.c | 452 +++++++--------------------
> > tests/qtest/vhost-user-blk-test.c | 2 +-
> > util/vhost-user-server.c | 10 +-
> > block/export/meson.build | 1 +
> > block/meson.build | 1 -
> > 8 files changed, 158 insertions(+), 360 deletions(-)
> >
> > diff --git a/qapi/block-export.json b/qapi/block-export.json
> > index ace0d66e17..2e44625bb1 100644
> > --- a/qapi/block-export.json
> > +++ b/qapi/block-export.json
> > @@ -84,6 +84,21 @@
> > 'data': { '*name': 'str', '*description': 'str',
> > '*bitmap': 'str' } }
> >
> > +##
> > +# @BlockExportOptionsVhostUserBlk:
> > +#
> > +# A vhost-user-blk block export.
> > +#
> > +# @addr: The vhost-user socket on which to listen. Both 'unix' and 'fd'
> > +# SocketAddress types are supported. Passed fds must be UNIX domain
> > +# sockets.
>
> "addr.type must be 'unix' or 'fd'" is not visible in introspection.
> Awkward. Practical problem only if other addresses ever become
> available here. Is that possible?
addr.type=fd itself has the same problem, because it is a file
descriptor without type information. Therefore the QMP client cannot
introspect which types of file descriptors can be passed.
Two ideas:
1. Introduce per-address family fd types (SocketAddrFdTcpInet,
SocketAddrFdTcpInet6, SocketAddrFdUnix, etc) to express specific
address families in the QAPI schema.
Then use per-command unions to express the address families supported
by specific commands. For example,
BlockExportOptionsVhostUserBlkSocketAddr would only allow
SocketAddrUnix and SocketAddrFdUnix. That way new address families
can be supported in the future and introspection reports.
2. Use a side-channel (query-features, I think we discussed something
like this a while back) to report features that cannot be
introspected.
I think the added complexity for achieving full introspection is not
worth it. It becomes harder to define new QAPI commands, increases the
chance of errors, and is more tedious to program for clients/servers.
Accepting any SocketAddr seems reasonable to me since vhost-user
requires an address family that has file descriptor passing. Very few
address families support this feature and we don't expect to add new
ones often.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
2020-09-30 9:45 ` Stefan Hajnoczi
@ 2020-09-30 14:33 ` Markus Armbruster
2020-10-01 15:25 ` Stefan Hajnoczi
0 siblings, 1 reply; 21+ messages in thread
From: Markus Armbruster @ 2020-09-30 14:33 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block, qemu-devel,
Coiby Xu, Max Reitz, Paolo Bonzini
Stefan Hajnoczi <stefanha@redhat.com> writes:
> On Wed, Sep 30, 2020 at 07:28:58AM +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.
>> >
>> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > ---
>> > v2:
>> > * Replace str unix-socket with SocketAddress addr to match NBD and
>> > support file descriptor passing
>> > * Make addr mandatory [Markus]
>> > * Update vhost-user-blk-test.c to use --export syntax
>> > ---
>> > qapi/block-export.json | 21 +-
>> > block/export/vhost-user-blk-server.h | 23 +-
>> > block/export/export.c | 8 +-
>> > block/export/vhost-user-blk-server.c | 452 +++++++--------------------
>> > tests/qtest/vhost-user-blk-test.c | 2 +-
>> > util/vhost-user-server.c | 10 +-
>> > block/export/meson.build | 1 +
>> > block/meson.build | 1 -
>> > 8 files changed, 158 insertions(+), 360 deletions(-)
>> >
>> > diff --git a/qapi/block-export.json b/qapi/block-export.json
>> > index ace0d66e17..2e44625bb1 100644
>> > --- a/qapi/block-export.json
>> > +++ b/qapi/block-export.json
>> > @@ -84,6 +84,21 @@
>> > 'data': { '*name': 'str', '*description': 'str',
>> > '*bitmap': 'str' } }
>> >
>> > +##
>> > +# @BlockExportOptionsVhostUserBlk:
>> > +#
>> > +# A vhost-user-blk block export.
>> > +#
>> > +# @addr: The vhost-user socket on which to listen. Both 'unix' and 'fd'
>> > +# SocketAddress types are supported. Passed fds must be UNIX domain
>> > +# sockets.
>>
>> "addr.type must be 'unix' or 'fd'" is not visible in introspection.
>> Awkward. Practical problem only if other addresses ever become
>> available here. Is that possible?
>
> addr.type=fd itself has the same problem, because it is a file
> descriptor without type information. Therefore the QMP client cannot
> introspect which types of file descriptors can be passed.
Yes, but if introspection could tell us which which values of addr.type
are valid, then a client should figure out the address families, as
follows. Any valid value other than 'fd' corresponds to an address
family. The set of values valid for addr.type therefore corresponds to
a set of address families. The address families in that set are all
valid with 'fd', aren't they?
> Two ideas:
>
> 1. Introduce per-address family fd types (SocketAddrFdTcpInet,
> SocketAddrFdTcpInet6, SocketAddrFdUnix, etc) to express specific
> address families in the QAPI schema.
>
> Then use per-command unions to express the address families supported
> by specific commands. For example,
> BlockExportOptionsVhostUserBlkSocketAddr would only allow
> SocketAddrUnix and SocketAddrFdUnix. That way new address families
> can be supported in the future and introspection reports.
Awkward. These types would have to differ structurally, or else they
are indistinguishable in introspection.
> 2. Use a side-channel (query-features, I think we discussed something
> like this a while back) to report features that cannot be
> introspected.
We implemented this in the form of QAPI feature flags, visible in
introspection. You could do something like
'addr': { 'type': 'SocketAddress',
'features': [ 'unix' ] }
> I think the added complexity for achieving full introspection is not
> worth it. It becomes harder to define new QAPI commands, increases the
> chance of errors, and is more tedious to program for clients/servers.
Hence my question: is it possible that address families other than unix
become available here?
When that happens, we have an introspection problem of the sort we
common solve with a feature flag.
> Accepting any SocketAddr seems reasonable to me since vhost-user
> requires an address family that has file descriptor passing. Very few
> address families support this feature and we don't expect to add new
> ones often.
Your answer appears to be "yes in theory, quite unlikely in practice".
Correct?
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
2020-09-30 14:33 ` Markus Armbruster
@ 2020-10-01 15:25 ` Stefan Hajnoczi
2020-10-02 5:20 ` Markus Armbruster
0 siblings, 1 reply; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-10-01 15:25 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block, qemu-devel,
Coiby Xu, Max Reitz, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 5576 bytes --]
On Wed, Sep 30, 2020 at 04:33:18PM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
> > On Wed, Sep 30, 2020 at 07:28:58AM +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.
> >> >
> >> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> > ---
> >> > v2:
> >> > * Replace str unix-socket with SocketAddress addr to match NBD and
> >> > support file descriptor passing
> >> > * Make addr mandatory [Markus]
> >> > * Update vhost-user-blk-test.c to use --export syntax
> >> > ---
> >> > qapi/block-export.json | 21 +-
> >> > block/export/vhost-user-blk-server.h | 23 +-
> >> > block/export/export.c | 8 +-
> >> > block/export/vhost-user-blk-server.c | 452 +++++++--------------------
> >> > tests/qtest/vhost-user-blk-test.c | 2 +-
> >> > util/vhost-user-server.c | 10 +-
> >> > block/export/meson.build | 1 +
> >> > block/meson.build | 1 -
> >> > 8 files changed, 158 insertions(+), 360 deletions(-)
> >> >
> >> > diff --git a/qapi/block-export.json b/qapi/block-export.json
> >> > index ace0d66e17..2e44625bb1 100644
> >> > --- a/qapi/block-export.json
> >> > +++ b/qapi/block-export.json
> >> > @@ -84,6 +84,21 @@
> >> > 'data': { '*name': 'str', '*description': 'str',
> >> > '*bitmap': 'str' } }
> >> >
> >> > +##
> >> > +# @BlockExportOptionsVhostUserBlk:
> >> > +#
> >> > +# A vhost-user-blk block export.
> >> > +#
> >> > +# @addr: The vhost-user socket on which to listen. Both 'unix' and 'fd'
> >> > +# SocketAddress types are supported. Passed fds must be UNIX domain
> >> > +# sockets.
> >>
> >> "addr.type must be 'unix' or 'fd'" is not visible in introspection.
> >> Awkward. Practical problem only if other addresses ever become
> >> available here. Is that possible?
> >
> > addr.type=fd itself has the same problem, because it is a file
> > descriptor without type information. Therefore the QMP client cannot
> > introspect which types of file descriptors can be passed.
>
> Yes, but if introspection could tell us which which values of addr.type
> are valid, then a client should figure out the address families, as
> follows. Any valid value other than 'fd' corresponds to an address
> family. The set of values valid for addr.type therefore corresponds to
> a set of address families. The address families in that set are all
> valid with 'fd', aren't they?
Yes, in this case the address families in addr.type are the ones also
supported by addr.type=fd.
> > Two ideas:
> >
> > 1. Introduce per-address family fd types (SocketAddrFdTcpInet,
> > SocketAddrFdTcpInet6, SocketAddrFdUnix, etc) to express specific
> > address families in the QAPI schema.
> >
> > Then use per-command unions to express the address families supported
> > by specific commands. For example,
> > BlockExportOptionsVhostUserBlkSocketAddr would only allow
> > SocketAddrUnix and SocketAddrFdUnix. That way new address families
> > can be supported in the future and introspection reports.
>
> Awkward. These types would have to differ structurally, or else they
> are indistinguishable in introspection.
>
> > 2. Use a side-channel (query-features, I think we discussed something
> > like this a while back) to report features that cannot be
> > introspected.
>
> We implemented this in the form of QAPI feature flags, visible in
> introspection. You could do something like
>
> 'addr': { 'type': 'SocketAddress',
> 'features': [ 'unix' ] }
That is nice.
>
> > I think the added complexity for achieving full introspection is not
> > worth it. It becomes harder to define new QAPI commands, increases the
> > chance of errors, and is more tedious to program for clients/servers.
>
> Hence my question: is it possible that address families other than unix
> become available here?
>
> When that happens, we have an introspection problem of the sort we
> common solve with a feature flag.
>
> > Accepting any SocketAddr seems reasonable to me since vhost-user
> > requires an address family that has file descriptor passing. Very few
> > address families support this feature and we don't expect to add new
> > ones often.
>
> Your answer appears to be "yes in theory, quite unlikely in practice".
> Correct?
Yes.
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
2020-10-01 15:25 ` Stefan Hajnoczi
@ 2020-10-02 5:20 ` Markus Armbruster
2020-10-08 16:02 ` Stefan Hajnoczi
0 siblings, 1 reply; 21+ messages in thread
From: Markus Armbruster @ 2020-10-02 5:20 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block, qemu-devel,
Coiby Xu, Max Reitz, Paolo Bonzini
Stefan Hajnoczi <stefanha@redhat.com> writes:
> On Wed, Sep 30, 2020 at 04:33:18PM +0200, Markus Armbruster wrote:
>> Stefan Hajnoczi <stefanha@redhat.com> writes:
>>
>> > On Wed, Sep 30, 2020 at 07:28:58AM +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.
>> >> >
>> >> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> >> > ---
>> >> > v2:
>> >> > * Replace str unix-socket with SocketAddress addr to match NBD and
>> >> > support file descriptor passing
>> >> > * Make addr mandatory [Markus]
>> >> > * Update vhost-user-blk-test.c to use --export syntax
>> >> > ---
>> >> > qapi/block-export.json | 21 +-
>> >> > block/export/vhost-user-blk-server.h | 23 +-
>> >> > block/export/export.c | 8 +-
>> >> > block/export/vhost-user-blk-server.c | 452 +++++++--------------------
>> >> > tests/qtest/vhost-user-blk-test.c | 2 +-
>> >> > util/vhost-user-server.c | 10 +-
>> >> > block/export/meson.build | 1 +
>> >> > block/meson.build | 1 -
>> >> > 8 files changed, 158 insertions(+), 360 deletions(-)
>> >> >
>> >> > diff --git a/qapi/block-export.json b/qapi/block-export.json
>> >> > index ace0d66e17..2e44625bb1 100644
>> >> > --- a/qapi/block-export.json
>> >> > +++ b/qapi/block-export.json
>> >> > @@ -84,6 +84,21 @@
>> >> > 'data': { '*name': 'str', '*description': 'str',
>> >> > '*bitmap': 'str' } }
>> >> >
>> >> > +##
>> >> > +# @BlockExportOptionsVhostUserBlk:
>> >> > +#
>> >> > +# A vhost-user-blk block export.
>> >> > +#
>> >> > +# @addr: The vhost-user socket on which to listen. Both 'unix' and 'fd'
>> >> > +# SocketAddress types are supported. Passed fds must be UNIX domain
>> >> > +# sockets.
>> >>
>> >> "addr.type must be 'unix' or 'fd'" is not visible in introspection.
>> >> Awkward. Practical problem only if other addresses ever become
>> >> available here. Is that possible?
>> >
>> > addr.type=fd itself has the same problem, because it is a file
>> > descriptor without type information. Therefore the QMP client cannot
>> > introspect which types of file descriptors can be passed.
>>
>> Yes, but if introspection could tell us which which values of addr.type
>> are valid, then a client should figure out the address families, as
>> follows. Any valid value other than 'fd' corresponds to an address
>> family. The set of values valid for addr.type therefore corresponds to
>> a set of address families. The address families in that set are all
>> valid with 'fd', aren't they?
>
> Yes, in this case the address families in addr.type are the ones also
> supported by addr.type=fd.
>
>> > Two ideas:
>> >
>> > 1. Introduce per-address family fd types (SocketAddrFdTcpInet,
>> > SocketAddrFdTcpInet6, SocketAddrFdUnix, etc) to express specific
>> > address families in the QAPI schema.
>> >
>> > Then use per-command unions to express the address families supported
>> > by specific commands. For example,
>> > BlockExportOptionsVhostUserBlkSocketAddr would only allow
>> > SocketAddrUnix and SocketAddrFdUnix. That way new address families
>> > can be supported in the future and introspection reports.
>>
>> Awkward. These types would have to differ structurally, or else they
>> are indistinguishable in introspection.
>>
>> > 2. Use a side-channel (query-features, I think we discussed something
>> > like this a while back) to report features that cannot be
>> > introspected.
>>
>> We implemented this in the form of QAPI feature flags, visible in
>> introspection. You could do something like
>>
>> 'addr': { 'type': 'SocketAddress',
>> 'features': [ 'unix' ] }
>
> That is nice.
>
>>
>> > I think the added complexity for achieving full introspection is not
>> > worth it. It becomes harder to define new QAPI commands, increases the
>> > chance of errors, and is more tedious to program for clients/servers.
>>
>> Hence my question: is it possible that address families other than unix
>> become available here?
>>
>> When that happens, we have an introspection problem of the sort we
>> common solve with a feature flag.
>>
>> > Accepting any SocketAddr seems reasonable to me since vhost-user
>> > requires an address family that has file descriptor passing. Very few
>> > address families support this feature and we don't expect to add new
>> > ones often.
>>
>> Your answer appears to be "yes in theory, quite unlikely in practice".
>> Correct?
Keeping introspection "tight" would be nice, but since a real need for
"tight" here seems quite unlikely, it doesn't seem to be worth the
trouble.
Perhaps this argument could be worked into the commit message. Up to
you.
Acked-by: Markus Armbruster <armbru@redhat.com>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API
2020-10-02 5:20 ` Markus Armbruster
@ 2020-10-08 16:02 ` Stefan Hajnoczi
0 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-10-08 16:02 UTC (permalink / raw)
To: Markus Armbruster
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block, qemu-devel,
Coiby Xu, Max Reitz, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]
On Fri, Oct 02, 2020 at 07:20:48AM +0200, Markus Armbruster wrote:
> Stefan Hajnoczi <stefanha@redhat.com> writes:
> > On Wed, Sep 30, 2020 at 04:33:18PM +0200, Markus Armbruster wrote:
> >> Stefan Hajnoczi <stefanha@redhat.com> writes:
> >> > On Wed, Sep 30, 2020 at 07:28:58AM +0200, Markus Armbruster wrote:
> >> >> Stefan Hajnoczi <stefanha@redhat.com> writes:
> >> Hence my question: is it possible that address families other than unix
> >> become available here?
> >>
> >> When that happens, we have an introspection problem of the sort we
> >> common solve with a feature flag.
> >>
> >> > Accepting any SocketAddr seems reasonable to me since vhost-user
> >> > requires an address family that has file descriptor passing. Very few
> >> > address families support this feature and we don't expect to add new
> >> > ones often.
> >>
> >> Your answer appears to be "yes in theory, quite unlikely in practice".
> >> Correct?
>
> Keeping introspection "tight" would be nice, but since a real need for
> "tight" here seems quite unlikely, it doesn't seem to be worth the
> trouble.
>
> Perhaps this argument could be worked into the commit message. Up to
> you.
>
> Acked-by: Markus Armbruster <armbru@redhat.com>
Thanks, I will update the commit message when merging.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 12/13] util/vhost-user-server: move header to include/
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
` (10 preceding siblings ...)
2020-09-24 15:15 ` [PATCH v2 11/13] block/export: convert vhost-user-blk server to block export API Stefan Hajnoczi
@ 2020-09-24 15:15 ` Stefan Hajnoczi
2020-09-24 15:15 ` [PATCH v2 13/13] util/vhost-user-server: use static library in meson.build Stefan Hajnoczi
2020-10-09 10:18 ` [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
13 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-24 15:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
Headers used by other subsystems are located in include/. Also add the
vhost-user-server and vhost-user-blk-server headers to MAINTAINERS.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
MAINTAINERS | 4 +++-
{util => include/qemu}/vhost-user-server.h | 0
block/export/vhost-user-blk-server.c | 2 +-
util/vhost-user-server.c | 2 +-
4 files changed, 5 insertions(+), 3 deletions(-)
rename {util => include/qemu}/vhost-user-server.h (100%)
diff --git a/MAINTAINERS b/MAINTAINERS
index 55ad6abe73..4d1d7c1854 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3043,9 +3043,11 @@ Vhost-user block device backend server
M: Coiby Xu <Coiby.Xu@gmail.com>
S: Maintained
F: block/export/vhost-user-blk-server.c
-F: util/vhost-user-server.c
+F: block/export/vhost-user-blk-server.h
+F: include/qemu/vhost-user-server.h
F: tests/qtest/vhost-user-blk-test.c
F: tests/qtest/libqos/vhost-user-blk.c
+F: util/vhost-user-server.c
Replication
M: Wen Congyang <wencongyang2@huawei.com>
diff --git a/util/vhost-user-server.h b/include/qemu/vhost-user-server.h
similarity index 100%
rename from util/vhost-user-server.h
rename to include/qemu/vhost-user-server.h
diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index 91fc7040b2..81072a5a46 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -13,7 +13,7 @@
#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 "qemu/vhost-user-server.h"
#include "vhost-user-blk-server.h"
#include "qapi/error.h"
#include "qom/object_interfaces.h"
diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index 2a27139eb8..3fd26c9f30 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -9,8 +9,8 @@
*/
#include "qemu/osdep.h"
#include "qemu/main-loop.h"
+#include "qemu/vhost-user-server.h"
#include "block/aio-wait.h"
-#include "vhost-user-server.h"
/*
* Theory of operation:
--
2.26.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v2 13/13] util/vhost-user-server: use static library in meson.build
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
` (11 preceding siblings ...)
2020-09-24 15:15 ` [PATCH v2 12/13] util/vhost-user-server: move header to include/ Stefan Hajnoczi
@ 2020-09-24 15:15 ` Stefan Hajnoczi
2020-10-09 10:18 ` [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
13 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-09-24 15:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Stefan Hajnoczi,
Paolo Bonzini
Don't compile contrib/libvhost-user/libvhost-user.c again. Instead build
the static library once and then reuse it throughout QEMU.
Also switch from CONFIG_LINUX to CONFIG_VHOST_USER, which is what the
vhost-user tools (vhost-user-gpu, etc) do.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block/export/export.c | 8 ++++----
block/export/meson.build | 2 +-
contrib/libvhost-user/meson.build | 1 +
meson.build | 6 +++++-
tests/qtest/meson.build | 2 +-
util/meson.build | 4 +++-
6 files changed, 15 insertions(+), 8 deletions(-)
diff --git a/block/export/export.c b/block/export/export.c
index d0c7590911..5f6d05a092 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -17,17 +17,17 @@
#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"
#include "qemu/id.h"
+#ifdef CONFIG_VHOST_USER
+#include "vhost-user-blk-server.h"
+#endif
static const BlockExportDriver *blk_exp_drivers[] = {
&blk_exp_nbd,
-#if CONFIG_LINUX
+#ifdef CONFIG_VHOST_USER
&blk_exp_vhost_user_blk,
#endif
};
diff --git a/block/export/meson.build b/block/export/meson.build
index ef3a9576f7..469a7aa0f5 100644
--- a/block/export/meson.build
+++ b/block/export/meson.build
@@ -1,2 +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'))
+block_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-blk-server.c'))
diff --git a/contrib/libvhost-user/meson.build b/contrib/libvhost-user/meson.build
index e68dd1a581..a261e7665f 100644
--- a/contrib/libvhost-user/meson.build
+++ b/contrib/libvhost-user/meson.build
@@ -1,3 +1,4 @@
libvhost_user = static_library('vhost-user',
files('libvhost-user.c', 'libvhost-user-glib.c'),
build_by_default: false)
+vhost_user = declare_dependency(link_with: libvhost_user)
diff --git a/meson.build b/meson.build
index 4c6c7310fa..eb84b97ebb 100644
--- a/meson.build
+++ b/meson.build
@@ -788,6 +788,11 @@ trace_events_subdirs += [
'util',
]
+vhost_user = not_found
+if 'CONFIG_VHOST_USER' in config_host
+ subdir('contrib/libvhost-user')
+endif
+
subdir('qapi')
subdir('qobject')
subdir('stubs')
@@ -1169,7 +1174,6 @@ if have_tools
install: true)
if 'CONFIG_VHOST_USER' in config_host
- subdir('contrib/libvhost-user')
subdir('contrib/vhost-user-blk')
subdir('contrib/vhost-user-gpu')
subdir('contrib/vhost-user-input')
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index c72821b09a..aa8d0985e1 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -191,7 +191,7 @@ qos_test_ss.add(
)
qos_test_ss.add(when: 'CONFIG_VIRTFS', if_true: files('virtio-9p-test.c'))
qos_test_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-test.c'))
-qos_test_ss.add(when: 'CONFIG_LINUX', if_true: files('vhost-user-blk-test.c'))
+qos_test_ss.add(when: 'CONFIG_VHOST_USER', if_true: files('vhost-user-blk-test.c'))
extra_qtest_deps = {
'bios-tables-test': [io],
diff --git a/util/meson.build b/util/meson.build
index 2296e81b34..9b2a7a5de9 100644
--- a/util/meson.build
+++ b/util/meson.build
@@ -66,7 +66,9 @@ if have_block
util_ss.add(files('main-loop.c'))
util_ss.add(files('nvdimm-utils.c'))
util_ss.add(files('qemu-coroutine.c', 'qemu-coroutine-lock.c', 'qemu-coroutine-io.c'))
- util_ss.add(when: 'CONFIG_LINUX', if_true: files('vhost-user-server.c'))
+ util_ss.add(when: 'CONFIG_VHOST_USER', if_true: [
+ files('vhost-user-server.c'), vhost_user
+ ])
util_ss.add(files('block-helpers.c'))
util_ss.add(files('qemu-coroutine-sleep.c'))
util_ss.add(files('qemu-co-shared-resource.c'))
--
2.26.2
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API
2020-09-24 15:15 [PATCH v2 00/13] block/export: convert vhost-user-blk-server to block exports API Stefan Hajnoczi
` (12 preceding siblings ...)
2020-09-24 15:15 ` [PATCH v2 13/13] util/vhost-user-server: use static library in meson.build Stefan Hajnoczi
@ 2020-10-09 10:18 ` Stefan Hajnoczi
13 siblings, 0 replies; 21+ messages in thread
From: Stefan Hajnoczi @ 2020-10-09 10:18 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Laurent Vivier, Thomas Huth, qemu-block,
Markus Armbruster, Coiby Xu, Max Reitz, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 3803 bytes --]
On Thu, Sep 24, 2020 at 04:15:36PM +0100, Stefan Hajnoczi wrote:
> v2:
> * Replace unix-socket=str with addr=SocketAddress for consistency with NBD and
> fd passing support. The new syntax is:
> --export vhost-user-blk,addr.type=unix,addr.path=/tmp/vhost-user-blk.sock,...
> * Make addr=SocketAddress mandatory instead of optional [Markus]
> * Update test-vhost-user-blk.c to use new syntax
> * Add patch moving vhost-user-server.h to include/
> * Add patch to use libvhost-user.a instead of compiling it multiple times
>
> 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,addr.type=unix,addr.path=/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 (13):
> 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
> util/vhost-user-server: move header to include/
> util/vhost-user-server: use static library in meson.build
>
> MAINTAINERS | 4 +-
> qapi/block-export.json | 21 +-
> block/export/vhost-user-blk-server.h | 23 +-
> {util => include/qemu}/vhost-user-server.h | 32 +-
> block/export/export.c | 8 +-
> block/export/vhost-user-blk-server.c | 525 ++++++---------------
> tests/qtest/vhost-user-blk-test.c | 2 +-
> util/vhost-user-server.c | 334 ++++++-------
> block/export/meson.build | 1 +
> block/meson.build | 1 -
> contrib/libvhost-user/meson.build | 1 +
> meson.build | 6 +-
> tests/qtest/meson.build | 2 +-
> util/meson.build | 4 +-
> 14 files changed, 376 insertions(+), 588 deletions(-)
> rename {util => include/qemu}/vhost-user-server.h (72%)
>
> --
> 2.26.2
>
Thanks, applied to my block tree with a mention of the QAPI schema
situation regarding address families as suggested by Markus:
https://github.com/stefanha/qemu/commits/block
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread