* [PATCH 1/7] vhost: Add Error parameter to vhost_dev_init()
2021-06-09 15:46 [PATCH 0/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
@ 2021-06-09 15:46 ` Kevin Wolf
2021-06-10 9:05 ` Stefano Garzarella
2021-06-11 19:14 ` Raphael Norwitz
2021-06-09 15:46 ` [PATCH 2/7] vhost: Distinguish errors in vhost_backend_init() Kevin Wolf
` (6 subsequent siblings)
7 siblings, 2 replies; 23+ messages in thread
From: Kevin Wolf @ 2021-06-09 15:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, raphael.norwitz, qemu-devel, mst
This allows callers to return better error messages instead of making
one up while the real error ends up on stderr. Most callers can
immediately make use of this because they already have an Error
parameter themselves. The others just keep printing the error with
error_report_err().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/hw/virtio/vhost.h | 2 +-
backends/cryptodev-vhost.c | 5 ++++-
backends/vhost-user.c | 4 ++--
hw/block/vhost-user-blk.c | 4 ++--
hw/net/vhost_net.c | 6 +++++-
hw/scsi/vhost-scsi.c | 4 +---
hw/scsi/vhost-user-scsi.c | 4 +---
hw/virtio/vhost-user-fs.c | 3 +--
hw/virtio/vhost-user-vsock.c | 3 +--
hw/virtio/vhost-vsock.c | 3 +--
hw/virtio/vhost.c | 16 ++++++++++------
11 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 21a9a52088..2d7aaad67b 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -104,7 +104,7 @@ struct vhost_net {
int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
VhostBackendType backend_type,
- uint32_t busyloop_timeout);
+ uint32_t busyloop_timeout, Error **errp);
void vhost_dev_cleanup(struct vhost_dev *hdev);
int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
index 8231e7f1bc..bc13e466b4 100644
--- a/backends/cryptodev-vhost.c
+++ b/backends/cryptodev-vhost.c
@@ -52,6 +52,7 @@ cryptodev_vhost_init(
{
int r;
CryptoDevBackendVhost *crypto;
+ Error *local_err = NULL;
crypto = g_new(CryptoDevBackendVhost, 1);
crypto->dev.max_queues = 1;
@@ -66,8 +67,10 @@ cryptodev_vhost_init(
/* vhost-user needs vq_index to initiate a specific queue pair */
crypto->dev.vq_index = crypto->cc->queue_index * crypto->dev.nvqs;
- r = vhost_dev_init(&crypto->dev, options->opaque, options->backend_type, 0);
+ r = vhost_dev_init(&crypto->dev, options->opaque, options->backend_type, 0,
+ &local_err);
if (r < 0) {
+ error_report_err(local_err);
goto fail;
}
diff --git a/backends/vhost-user.c b/backends/vhost-user.c
index b366610e16..10b39992d2 100644
--- a/backends/vhost-user.c
+++ b/backends/vhost-user.c
@@ -48,9 +48,9 @@ vhost_user_backend_dev_init(VhostUserBackend *b, VirtIODevice *vdev,
b->dev.nvqs = nvqs;
b->dev.vqs = g_new0(struct vhost_virtqueue, nvqs);
- ret = vhost_dev_init(&b->dev, &b->vhost_user, VHOST_BACKEND_TYPE_USER, 0);
+ ret = vhost_dev_init(&b->dev, &b->vhost_user, VHOST_BACKEND_TYPE_USER, 0,
+ errp);
if (ret < 0) {
- error_setg_errno(errp, -ret, "vhost initialization failed");
return -1;
}
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index c6210fad0c..0cb56baefb 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -332,9 +332,9 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
vhost_dev_set_config_notifier(&s->dev, &blk_ops);
- ret = vhost_dev_init(&s->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0);
+ ret = vhost_dev_init(&s->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0,
+ errp);
if (ret < 0) {
- error_setg_errno(errp, -ret, "vhost initialization failed");
return ret;
}
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 44c1ed92dc..447b119f85 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -22,6 +22,7 @@
#include "standard-headers/linux/vhost_types.h"
#include "hw/virtio/virtio-net.h"
#include "net/vhost_net.h"
+#include "qapi/error.h"
#include "qemu/error-report.h"
#include "qemu/main-loop.h"
@@ -157,6 +158,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
bool backend_kernel = options->backend_type == VHOST_BACKEND_TYPE_KERNEL;
struct vhost_net *net = g_new0(struct vhost_net, 1);
uint64_t features = 0;
+ Error *local_err = NULL;
if (!options->net_backend) {
fprintf(stderr, "vhost-net requires net backend to be setup\n");
@@ -187,8 +189,10 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
}
r = vhost_dev_init(&net->dev, options->opaque,
- options->backend_type, options->busyloop_timeout);
+ options->backend_type, options->busyloop_timeout,
+ &local_err);
if (r < 0) {
+ error_report_err(local_err);
goto fail;
}
if (backend_kernel) {
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 4d70fa036b..8c611bfd2d 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -219,10 +219,8 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
vsc->dev.backend_features = 0;
ret = vhost_dev_init(&vsc->dev, (void *)(uintptr_t)vhostfd,
- VHOST_BACKEND_TYPE_KERNEL, 0);
+ VHOST_BACKEND_TYPE_KERNEL, 0, errp);
if (ret < 0) {
- error_setg(errp, "vhost-scsi: vhost initialization failed: %s",
- strerror(-ret));
goto free_vqs;
}
diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
index 4666019442..1b2f7eed98 100644
--- a/hw/scsi/vhost-user-scsi.c
+++ b/hw/scsi/vhost-user-scsi.c
@@ -122,10 +122,8 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
vqs = vsc->dev.vqs;
ret = vhost_dev_init(&vsc->dev, &s->vhost_user,
- VHOST_BACKEND_TYPE_USER, 0);
+ VHOST_BACKEND_TYPE_USER, 0, errp);
if (ret < 0) {
- error_setg(errp, "vhost-user-scsi: vhost initialization failed: %s",
- strerror(-ret));
goto free_vhost;
}
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index 6f7f91533d..c595957983 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -235,9 +235,8 @@ static void vuf_device_realize(DeviceState *dev, Error **errp)
fs->vhost_dev.nvqs = 1 + fs->conf.num_request_queues;
fs->vhost_dev.vqs = g_new0(struct vhost_virtqueue, fs->vhost_dev.nvqs);
ret = vhost_dev_init(&fs->vhost_dev, &fs->vhost_user,
- VHOST_BACKEND_TYPE_USER, 0);
+ VHOST_BACKEND_TYPE_USER, 0, errp);
if (ret < 0) {
- error_setg_errno(errp, -ret, "vhost_dev_init failed");
goto err_virtio;
}
diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
index a6f08c26b9..b6a4a25ea1 100644
--- a/hw/virtio/vhost-user-vsock.c
+++ b/hw/virtio/vhost-user-vsock.c
@@ -108,9 +108,8 @@ static void vuv_device_realize(DeviceState *dev, Error **errp)
vhost_dev_set_config_notifier(&vvc->vhost_dev, &vsock_ops);
ret = vhost_dev_init(&vvc->vhost_dev, &vsock->vhost_user,
- VHOST_BACKEND_TYPE_USER, 0);
+ VHOST_BACKEND_TYPE_USER, 0, errp);
if (ret < 0) {
- error_setg_errno(errp, -ret, "vhost_dev_init failed");
goto err_virtio;
}
diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index 8ddfb9abfe..777cafe70d 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -170,9 +170,8 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
vhost_vsock_common_realize(vdev, "vhost-vsock");
ret = vhost_dev_init(&vvc->vhost_dev, (void *)(uintptr_t)vhostfd,
- VHOST_BACKEND_TYPE_KERNEL, 0);
+ VHOST_BACKEND_TYPE_KERNEL, 0, errp);
if (ret < 0) {
- error_setg_errno(errp, -ret, "vhost-vsock: vhost_dev_init failed");
goto err_virtio;
}
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 7b7bde7657..991c67ddcd 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1286,7 +1286,8 @@ static void vhost_virtqueue_cleanup(struct vhost_virtqueue *vq)
}
int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
- VhostBackendType backend_type, uint32_t busyloop_timeout)
+ VhostBackendType backend_type, uint32_t busyloop_timeout,
+ Error **errp)
{
uint64_t features;
int i, r, n_initialized_vqs = 0;
@@ -1300,24 +1301,26 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
r = hdev->vhost_ops->vhost_backend_init(hdev, opaque);
if (r < 0) {
+ error_setg(errp, "vhost_backend_init failed");
goto fail;
}
r = hdev->vhost_ops->vhost_set_owner(hdev);
if (r < 0) {
- VHOST_OPS_DEBUG("vhost_set_owner failed");
+ error_setg(errp, "vhost_set_owner failed");
goto fail;
}
r = hdev->vhost_ops->vhost_get_features(hdev, &features);
if (r < 0) {
- VHOST_OPS_DEBUG("vhost_get_features failed");
+ error_setg(errp, "vhost_get_features failed");
goto fail;
}
for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
if (r < 0) {
+ error_setg_errno(errp, -r, "Failed to initialize virtqueue %d", i);
goto fail;
}
}
@@ -1327,6 +1330,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
r = vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i,
busyloop_timeout);
if (r < 0) {
+ error_setg(errp, "Failed to set busyloop timeout");
goto fail_busyloop;
}
}
@@ -1367,7 +1371,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
if (hdev->migration_blocker != NULL) {
r = migrate_add_blocker(hdev->migration_blocker, &local_err);
if (local_err) {
- error_report_err(local_err);
+ error_propagate(errp, local_err);
error_free(hdev->migration_blocker);
goto fail_busyloop;
}
@@ -1384,8 +1388,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
- error_report("vhost backend memory slots limit is less"
- " than current number of present memory slots");
+ error_setg(errp, "vhost backend memory slots limit is less"
+ " than current number of present memory slots");
r = -1;
goto fail_busyloop;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 1/7] vhost: Add Error parameter to vhost_dev_init()
2021-06-09 15:46 ` [PATCH 1/7] vhost: Add Error parameter to vhost_dev_init() Kevin Wolf
@ 2021-06-10 9:05 ` Stefano Garzarella
2021-06-11 19:14 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Stefano Garzarella @ 2021-06-10 9:05 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:52PM +0200, Kevin Wolf wrote:
>This allows callers to return better error messages instead of making
>one up while the real error ends up on stderr. Most callers can
>immediately make use of this because they already have an Error
>parameter themselves. The others just keep printing the error with
>error_report_err().
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> include/hw/virtio/vhost.h | 2 +-
> backends/cryptodev-vhost.c | 5 ++++-
> backends/vhost-user.c | 4 ++--
> hw/block/vhost-user-blk.c | 4 ++--
> hw/net/vhost_net.c | 6 +++++-
> hw/scsi/vhost-scsi.c | 4 +---
> hw/scsi/vhost-user-scsi.c | 4 +---
> hw/virtio/vhost-user-fs.c | 3 +--
> hw/virtio/vhost-user-vsock.c | 3 +--
> hw/virtio/vhost-vsock.c | 3 +--
> hw/virtio/vhost.c | 16 ++++++++++------
> 11 files changed, 29 insertions(+), 25 deletions(-)
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/7] vhost: Add Error parameter to vhost_dev_init()
2021-06-09 15:46 ` [PATCH 1/7] vhost: Add Error parameter to vhost_dev_init() Kevin Wolf
2021-06-10 9:05 ` Stefano Garzarella
@ 2021-06-11 19:14 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Raphael Norwitz @ 2021-06-11 19:14 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Raphael Norwitz, qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:52PM +0200, Kevin Wolf wrote:
> This allows callers to return better error messages instead of making
> one up while the real error ends up on stderr. Most callers can
> immediately make use of this because they already have an Error
> parameter themselves. The others just keep printing the error with
> error_report_err().
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
> include/hw/virtio/vhost.h | 2 +-
> backends/cryptodev-vhost.c | 5 ++++-
> backends/vhost-user.c | 4 ++--
> hw/block/vhost-user-blk.c | 4 ++--
> hw/net/vhost_net.c | 6 +++++-
> hw/scsi/vhost-scsi.c | 4 +---
> hw/scsi/vhost-user-scsi.c | 4 +---
> hw/virtio/vhost-user-fs.c | 3 +--
> hw/virtio/vhost-user-vsock.c | 3 +--
> hw/virtio/vhost-vsock.c | 3 +--
> hw/virtio/vhost.c | 16 ++++++++++------
> 11 files changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 21a9a52088..2d7aaad67b 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -104,7 +104,7 @@ struct vhost_net {
>
> int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> VhostBackendType backend_type,
> - uint32_t busyloop_timeout);
> + uint32_t busyloop_timeout, Error **errp);
> void vhost_dev_cleanup(struct vhost_dev *hdev);
> int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev);
> void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev);
> diff --git a/backends/cryptodev-vhost.c b/backends/cryptodev-vhost.c
> index 8231e7f1bc..bc13e466b4 100644
> --- a/backends/cryptodev-vhost.c
> +++ b/backends/cryptodev-vhost.c
> @@ -52,6 +52,7 @@ cryptodev_vhost_init(
> {
> int r;
> CryptoDevBackendVhost *crypto;
> + Error *local_err = NULL;
>
> crypto = g_new(CryptoDevBackendVhost, 1);
> crypto->dev.max_queues = 1;
> @@ -66,8 +67,10 @@ cryptodev_vhost_init(
> /* vhost-user needs vq_index to initiate a specific queue pair */
> crypto->dev.vq_index = crypto->cc->queue_index * crypto->dev.nvqs;
>
> - r = vhost_dev_init(&crypto->dev, options->opaque, options->backend_type, 0);
> + r = vhost_dev_init(&crypto->dev, options->opaque, options->backend_type, 0,
> + &local_err);
> if (r < 0) {
> + error_report_err(local_err);
> goto fail;
> }
>
> diff --git a/backends/vhost-user.c b/backends/vhost-user.c
> index b366610e16..10b39992d2 100644
> --- a/backends/vhost-user.c
> +++ b/backends/vhost-user.c
> @@ -48,9 +48,9 @@ vhost_user_backend_dev_init(VhostUserBackend *b, VirtIODevice *vdev,
> b->dev.nvqs = nvqs;
> b->dev.vqs = g_new0(struct vhost_virtqueue, nvqs);
>
> - ret = vhost_dev_init(&b->dev, &b->vhost_user, VHOST_BACKEND_TYPE_USER, 0);
> + ret = vhost_dev_init(&b->dev, &b->vhost_user, VHOST_BACKEND_TYPE_USER, 0,
> + errp);
> if (ret < 0) {
> - error_setg_errno(errp, -ret, "vhost initialization failed");
> return -1;
> }
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index c6210fad0c..0cb56baefb 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -332,9 +332,9 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
>
> vhost_dev_set_config_notifier(&s->dev, &blk_ops);
>
> - ret = vhost_dev_init(&s->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0);
> + ret = vhost_dev_init(&s->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0,
> + errp);
> if (ret < 0) {
> - error_setg_errno(errp, -ret, "vhost initialization failed");
> return ret;
> }
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 44c1ed92dc..447b119f85 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -22,6 +22,7 @@
> #include "standard-headers/linux/vhost_types.h"
> #include "hw/virtio/virtio-net.h"
> #include "net/vhost_net.h"
> +#include "qapi/error.h"
> #include "qemu/error-report.h"
> #include "qemu/main-loop.h"
>
> @@ -157,6 +158,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
> bool backend_kernel = options->backend_type == VHOST_BACKEND_TYPE_KERNEL;
> struct vhost_net *net = g_new0(struct vhost_net, 1);
> uint64_t features = 0;
> + Error *local_err = NULL;
>
> if (!options->net_backend) {
> fprintf(stderr, "vhost-net requires net backend to be setup\n");
> @@ -187,8 +189,10 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options)
> }
>
> r = vhost_dev_init(&net->dev, options->opaque,
> - options->backend_type, options->busyloop_timeout);
> + options->backend_type, options->busyloop_timeout,
> + &local_err);
> if (r < 0) {
> + error_report_err(local_err);
> goto fail;
> }
> if (backend_kernel) {
> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
> index 4d70fa036b..8c611bfd2d 100644
> --- a/hw/scsi/vhost-scsi.c
> +++ b/hw/scsi/vhost-scsi.c
> @@ -219,10 +219,8 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
> vsc->dev.backend_features = 0;
>
> ret = vhost_dev_init(&vsc->dev, (void *)(uintptr_t)vhostfd,
> - VHOST_BACKEND_TYPE_KERNEL, 0);
> + VHOST_BACKEND_TYPE_KERNEL, 0, errp);
> if (ret < 0) {
> - error_setg(errp, "vhost-scsi: vhost initialization failed: %s",
> - strerror(-ret));
> goto free_vqs;
> }
>
> diff --git a/hw/scsi/vhost-user-scsi.c b/hw/scsi/vhost-user-scsi.c
> index 4666019442..1b2f7eed98 100644
> --- a/hw/scsi/vhost-user-scsi.c
> +++ b/hw/scsi/vhost-user-scsi.c
> @@ -122,10 +122,8 @@ static void vhost_user_scsi_realize(DeviceState *dev, Error **errp)
> vqs = vsc->dev.vqs;
>
> ret = vhost_dev_init(&vsc->dev, &s->vhost_user,
> - VHOST_BACKEND_TYPE_USER, 0);
> + VHOST_BACKEND_TYPE_USER, 0, errp);
> if (ret < 0) {
> - error_setg(errp, "vhost-user-scsi: vhost initialization failed: %s",
> - strerror(-ret));
> goto free_vhost;
> }
>
> diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
> index 6f7f91533d..c595957983 100644
> --- a/hw/virtio/vhost-user-fs.c
> +++ b/hw/virtio/vhost-user-fs.c
> @@ -235,9 +235,8 @@ static void vuf_device_realize(DeviceState *dev, Error **errp)
> fs->vhost_dev.nvqs = 1 + fs->conf.num_request_queues;
> fs->vhost_dev.vqs = g_new0(struct vhost_virtqueue, fs->vhost_dev.nvqs);
> ret = vhost_dev_init(&fs->vhost_dev, &fs->vhost_user,
> - VHOST_BACKEND_TYPE_USER, 0);
> + VHOST_BACKEND_TYPE_USER, 0, errp);
> if (ret < 0) {
> - error_setg_errno(errp, -ret, "vhost_dev_init failed");
> goto err_virtio;
> }
>
> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> index a6f08c26b9..b6a4a25ea1 100644
> --- a/hw/virtio/vhost-user-vsock.c
> +++ b/hw/virtio/vhost-user-vsock.c
> @@ -108,9 +108,8 @@ static void vuv_device_realize(DeviceState *dev, Error **errp)
> vhost_dev_set_config_notifier(&vvc->vhost_dev, &vsock_ops);
>
> ret = vhost_dev_init(&vvc->vhost_dev, &vsock->vhost_user,
> - VHOST_BACKEND_TYPE_USER, 0);
> + VHOST_BACKEND_TYPE_USER, 0, errp);
> if (ret < 0) {
> - error_setg_errno(errp, -ret, "vhost_dev_init failed");
> goto err_virtio;
> }
>
> diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
> index 8ddfb9abfe..777cafe70d 100644
> --- a/hw/virtio/vhost-vsock.c
> +++ b/hw/virtio/vhost-vsock.c
> @@ -170,9 +170,8 @@ static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
> vhost_vsock_common_realize(vdev, "vhost-vsock");
>
> ret = vhost_dev_init(&vvc->vhost_dev, (void *)(uintptr_t)vhostfd,
> - VHOST_BACKEND_TYPE_KERNEL, 0);
> + VHOST_BACKEND_TYPE_KERNEL, 0, errp);
> if (ret < 0) {
> - error_setg_errno(errp, -ret, "vhost-vsock: vhost_dev_init failed");
> goto err_virtio;
> }
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 7b7bde7657..991c67ddcd 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1286,7 +1286,8 @@ static void vhost_virtqueue_cleanup(struct vhost_virtqueue *vq)
> }
>
> int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> - VhostBackendType backend_type, uint32_t busyloop_timeout)
> + VhostBackendType backend_type, uint32_t busyloop_timeout,
> + Error **errp)
> {
> uint64_t features;
> int i, r, n_initialized_vqs = 0;
> @@ -1300,24 +1301,26 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>
> r = hdev->vhost_ops->vhost_backend_init(hdev, opaque);
> if (r < 0) {
> + error_setg(errp, "vhost_backend_init failed");
> goto fail;
> }
>
> r = hdev->vhost_ops->vhost_set_owner(hdev);
> if (r < 0) {
> - VHOST_OPS_DEBUG("vhost_set_owner failed");
> + error_setg(errp, "vhost_set_owner failed");
> goto fail;
> }
>
> r = hdev->vhost_ops->vhost_get_features(hdev, &features);
> if (r < 0) {
> - VHOST_OPS_DEBUG("vhost_get_features failed");
> + error_setg(errp, "vhost_get_features failed");
> goto fail;
> }
>
> for (i = 0; i < hdev->nvqs; ++i, ++n_initialized_vqs) {
> r = vhost_virtqueue_init(hdev, hdev->vqs + i, hdev->vq_index + i);
> if (r < 0) {
> + error_setg_errno(errp, -r, "Failed to initialize virtqueue %d", i);
> goto fail;
> }
> }
> @@ -1327,6 +1330,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> r = vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i,
> busyloop_timeout);
> if (r < 0) {
> + error_setg(errp, "Failed to set busyloop timeout");
> goto fail_busyloop;
> }
> }
> @@ -1367,7 +1371,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> if (hdev->migration_blocker != NULL) {
> r = migrate_add_blocker(hdev->migration_blocker, &local_err);
> if (local_err) {
> - error_report_err(local_err);
> + error_propagate(errp, local_err);
> error_free(hdev->migration_blocker);
> goto fail_busyloop;
> }
> @@ -1384,8 +1388,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> QLIST_INSERT_HEAD(&vhost_devices, hdev, entry);
>
> if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
> - error_report("vhost backend memory slots limit is less"
> - " than current number of present memory slots");
> + error_setg(errp, "vhost backend memory slots limit is less"
> + " than current number of present memory slots");
> r = -1;
> goto fail_busyloop;
> }
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 2/7] vhost: Distinguish errors in vhost_backend_init()
2021-06-09 15:46 [PATCH 0/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
2021-06-09 15:46 ` [PATCH 1/7] vhost: Add Error parameter to vhost_dev_init() Kevin Wolf
@ 2021-06-09 15:46 ` Kevin Wolf
2021-06-10 9:07 ` Stefano Garzarella
2021-06-11 19:43 ` Raphael Norwitz
2021-06-09 15:46 ` [PATCH 3/7] vhost: Return 0/-errno in vhost_dev_init() Kevin Wolf
` (5 subsequent siblings)
7 siblings, 2 replies; 23+ messages in thread
From: Kevin Wolf @ 2021-06-09 15:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, raphael.norwitz, qemu-devel, mst
Instead of just returning 0/-1 and letting the caller make up a
meaningless error message, add an Error parameter to allow reporting the
real error and switch to 0/-errno so that different kind of errors can
be distinguished in the caller.
Specifically, in vhost-user, EPROTO is used for all errors that relate
to the connection itself, whereas other error codes are used for errors
relating to the content of the connection. This will allow us later to
automatically reconnect when the connection goes away, without ending up
in an endless loop if it's a permanent error in the configuration.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/hw/virtio/vhost-backend.h | 3 ++-
hw/virtio/vhost-backend.c | 2 +-
hw/virtio/vhost-user.c | 41 ++++++++++++++++---------------
hw/virtio/vhost-vdpa.c | 2 +-
hw/virtio/vhost.c | 13 +++++-----
5 files changed, 32 insertions(+), 29 deletions(-)
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index 8a6f8e2a7a..728ebb0ed9 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -37,7 +37,8 @@ struct vhost_scsi_target;
struct vhost_iotlb_msg;
struct vhost_virtqueue;
-typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
+typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque,
+ Error **errp);
typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev);
diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
index 31b33bde37..f4f71cf58a 100644
--- a/hw/virtio/vhost-backend.c
+++ b/hw/virtio/vhost-backend.c
@@ -30,7 +30,7 @@ static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request,
return ioctl(fd, request, arg);
}
-static int vhost_kernel_init(struct vhost_dev *dev, void *opaque)
+static int vhost_kernel_init(struct vhost_dev *dev, void *opaque, Error **errp)
{
assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL);
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index ee57abe045..024cb201bb 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1856,7 +1856,8 @@ static int vhost_user_postcopy_notifier(NotifierWithReturn *notifier,
return 0;
}
-static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
+static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
+ Error **errp)
{
uint64_t features, protocol_features, ram_slots;
struct vhost_user *u;
@@ -1871,7 +1872,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
err = vhost_user_get_features(dev, &features);
if (err < 0) {
- return err;
+ return -EPROTO;
}
if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
@@ -1880,7 +1881,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
err = vhost_user_get_u64(dev, VHOST_USER_GET_PROTOCOL_FEATURES,
&protocol_features);
if (err < 0) {
- return err;
+ return -EPROTO;
}
dev->protocol_features =
@@ -1891,14 +1892,14 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
dev->protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
} else if (!(protocol_features &
(1ULL << VHOST_USER_PROTOCOL_F_CONFIG))) {
- error_report("Device expects VHOST_USER_PROTOCOL_F_CONFIG "
- "but backend does not support it.");
- return -1;
+ error_setg(errp, "Device expects VHOST_USER_PROTOCOL_F_CONFIG "
+ "but backend does not support it.");
+ return -EINVAL;
}
err = vhost_user_set_protocol_features(dev, dev->protocol_features);
if (err < 0) {
- return err;
+ return -EPROTO;
}
/* query the max queues we support if backend supports Multiple Queue */
@@ -1906,12 +1907,12 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
err = vhost_user_get_u64(dev, VHOST_USER_GET_QUEUE_NUM,
&dev->max_queues);
if (err < 0) {
- return err;
+ return -EPROTO;
}
}
if (dev->num_queues && dev->max_queues < dev->num_queues) {
- error_report("The maximum number of queues supported by the "
- "backend is %" PRIu64, dev->max_queues);
+ error_setg(errp, "The maximum number of queues supported by the "
+ "backend is %" PRIu64, dev->max_queues);
return -EINVAL;
}
@@ -1920,9 +1921,9 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
VHOST_USER_PROTOCOL_F_SLAVE_REQ) &&
virtio_has_feature(dev->protocol_features,
VHOST_USER_PROTOCOL_F_REPLY_ACK))) {
- error_report("IOMMU support requires reply-ack and "
- "slave-req protocol features.");
- return -1;
+ error_setg(errp, "IOMMU support requires reply-ack and "
+ "slave-req protocol features.");
+ return -EINVAL;
}
/* get max memory regions if backend supports configurable RAM slots */
@@ -1932,15 +1933,15 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
} else {
err = vhost_user_get_max_memslots(dev, &ram_slots);
if (err < 0) {
- return err;
+ return -EPROTO;
}
if (ram_slots < u->user->memory_slots) {
- error_report("The backend specified a max ram slots limit "
- "of %" PRIu64", when the prior validated limit was %d. "
- "This limit should never decrease.", ram_slots,
- u->user->memory_slots);
- return -1;
+ error_setg(errp, "The backend specified a max ram slots limit "
+ "of %" PRIu64", when the prior validated limit was "
+ "%d. This limit should never decrease.", ram_slots,
+ u->user->memory_slots);
+ return -EINVAL;
}
u->user->memory_slots = MIN(ram_slots, VHOST_USER_MAX_RAM_SLOTS);
@@ -1958,7 +1959,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
if (dev->vq_index == 0) {
err = vhost_setup_slave_channel(dev);
if (err < 0) {
- return err;
+ return -EPROTO;
}
}
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index ee51863d28..c2aadb57cb 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -273,7 +273,7 @@ static void vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &s);
}
-static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque)
+static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp)
{
struct vhost_vdpa *v;
uint64_t features;
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 991c67ddcd..fd13135706 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1289,9 +1289,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
VhostBackendType backend_type, uint32_t busyloop_timeout,
Error **errp)
{
+ ERRP_GUARD();
uint64_t features;
int i, r, n_initialized_vqs = 0;
- Error *local_err = NULL;
hdev->vdev = NULL;
hdev->migration_blocker = NULL;
@@ -1299,9 +1299,11 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
r = vhost_set_backend_type(hdev, backend_type);
assert(r >= 0);
- r = hdev->vhost_ops->vhost_backend_init(hdev, opaque);
+ r = hdev->vhost_ops->vhost_backend_init(hdev, opaque, errp);
if (r < 0) {
- error_setg(errp, "vhost_backend_init failed");
+ if (!*errp) {
+ error_setg_errno(errp, -r, "vhost_backend_init failed");
+ }
goto fail;
}
@@ -1369,9 +1371,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
}
if (hdev->migration_blocker != NULL) {
- r = migrate_add_blocker(hdev->migration_blocker, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ r = migrate_add_blocker(hdev->migration_blocker, errp);
+ if (*errp) {
error_free(hdev->migration_blocker);
goto fail_busyloop;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 2/7] vhost: Distinguish errors in vhost_backend_init()
2021-06-09 15:46 ` [PATCH 2/7] vhost: Distinguish errors in vhost_backend_init() Kevin Wolf
@ 2021-06-10 9:07 ` Stefano Garzarella
2021-06-11 19:43 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Stefano Garzarella @ 2021-06-10 9:07 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:53PM +0200, Kevin Wolf wrote:
>Instead of just returning 0/-1 and letting the caller make up a
>meaningless error message, add an Error parameter to allow reporting the
>real error and switch to 0/-errno so that different kind of errors can
>be distinguished in the caller.
>
>Specifically, in vhost-user, EPROTO is used for all errors that relate
>to the connection itself, whereas other error codes are used for errors
>relating to the content of the connection. This will allow us later to
>automatically reconnect when the connection goes away, without ending up
>in an endless loop if it's a permanent error in the configuration.
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> include/hw/virtio/vhost-backend.h | 3 ++-
> hw/virtio/vhost-backend.c | 2 +-
> hw/virtio/vhost-user.c | 41 ++++++++++++++++---------------
> hw/virtio/vhost-vdpa.c | 2 +-
> hw/virtio/vhost.c | 13 +++++-----
> 5 files changed, 32 insertions(+), 29 deletions(-)
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 2/7] vhost: Distinguish errors in vhost_backend_init()
2021-06-09 15:46 ` [PATCH 2/7] vhost: Distinguish errors in vhost_backend_init() Kevin Wolf
2021-06-10 9:07 ` Stefano Garzarella
@ 2021-06-11 19:43 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Raphael Norwitz @ 2021-06-11 19:43 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Raphael Norwitz, qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:53PM +0200, Kevin Wolf wrote:
> Instead of just returning 0/-1 and letting the caller make up a
> meaningless error message, add an Error parameter to allow reporting the
> real error and switch to 0/-errno so that different kind of errors can
> be distinguished in the caller.
>
> Specifically, in vhost-user, EPROTO is used for all errors that relate
> to the connection itself, whereas other error codes are used for errors
> relating to the content of the connection. This will allow us later to
> automatically reconnect when the connection goes away, without ending up
> in an endless loop if it's a permanent error in the configuration.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
> include/hw/virtio/vhost-backend.h | 3 ++-
> hw/virtio/vhost-backend.c | 2 +-
> hw/virtio/vhost-user.c | 41 ++++++++++++++++---------------
> hw/virtio/vhost-vdpa.c | 2 +-
> hw/virtio/vhost.c | 13 +++++-----
> 5 files changed, 32 insertions(+), 29 deletions(-)
>
> diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> index 8a6f8e2a7a..728ebb0ed9 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -37,7 +37,8 @@ struct vhost_scsi_target;
> struct vhost_iotlb_msg;
> struct vhost_virtqueue;
>
> -typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque);
> +typedef int (*vhost_backend_init)(struct vhost_dev *dev, void *opaque,
> + Error **errp);
> typedef int (*vhost_backend_cleanup)(struct vhost_dev *dev);
> typedef int (*vhost_backend_memslots_limit)(struct vhost_dev *dev);
>
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index 31b33bde37..f4f71cf58a 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -30,7 +30,7 @@ static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request,
> return ioctl(fd, request, arg);
> }
>
> -static int vhost_kernel_init(struct vhost_dev *dev, void *opaque)
> +static int vhost_kernel_init(struct vhost_dev *dev, void *opaque, Error **errp)
> {
> assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL);
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index ee57abe045..024cb201bb 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1856,7 +1856,8 @@ static int vhost_user_postcopy_notifier(NotifierWithReturn *notifier,
> return 0;
> }
>
> -static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
> +static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
> + Error **errp)
> {
> uint64_t features, protocol_features, ram_slots;
> struct vhost_user *u;
> @@ -1871,7 +1872,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
>
> err = vhost_user_get_features(dev, &features);
> if (err < 0) {
> - return err;
> + return -EPROTO;
> }
>
> if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> @@ -1880,7 +1881,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
> err = vhost_user_get_u64(dev, VHOST_USER_GET_PROTOCOL_FEATURES,
> &protocol_features);
> if (err < 0) {
> - return err;
> + return -EPROTO;
> }
>
> dev->protocol_features =
> @@ -1891,14 +1892,14 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
> dev->protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
> } else if (!(protocol_features &
> (1ULL << VHOST_USER_PROTOCOL_F_CONFIG))) {
> - error_report("Device expects VHOST_USER_PROTOCOL_F_CONFIG "
> - "but backend does not support it.");
> - return -1;
> + error_setg(errp, "Device expects VHOST_USER_PROTOCOL_F_CONFIG "
> + "but backend does not support it.");
> + return -EINVAL;
> }
>
> err = vhost_user_set_protocol_features(dev, dev->protocol_features);
> if (err < 0) {
> - return err;
> + return -EPROTO;
> }
>
> /* query the max queues we support if backend supports Multiple Queue */
> @@ -1906,12 +1907,12 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
> err = vhost_user_get_u64(dev, VHOST_USER_GET_QUEUE_NUM,
> &dev->max_queues);
> if (err < 0) {
> - return err;
> + return -EPROTO;
> }
> }
> if (dev->num_queues && dev->max_queues < dev->num_queues) {
> - error_report("The maximum number of queues supported by the "
> - "backend is %" PRIu64, dev->max_queues);
> + error_setg(errp, "The maximum number of queues supported by the "
> + "backend is %" PRIu64, dev->max_queues);
> return -EINVAL;
> }
>
> @@ -1920,9 +1921,9 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
> VHOST_USER_PROTOCOL_F_SLAVE_REQ) &&
> virtio_has_feature(dev->protocol_features,
> VHOST_USER_PROTOCOL_F_REPLY_ACK))) {
> - error_report("IOMMU support requires reply-ack and "
> - "slave-req protocol features.");
> - return -1;
> + error_setg(errp, "IOMMU support requires reply-ack and "
> + "slave-req protocol features.");
> + return -EINVAL;
> }
>
> /* get max memory regions if backend supports configurable RAM slots */
> @@ -1932,15 +1933,15 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
> } else {
> err = vhost_user_get_max_memslots(dev, &ram_slots);
> if (err < 0) {
> - return err;
> + return -EPROTO;
> }
>
> if (ram_slots < u->user->memory_slots) {
> - error_report("The backend specified a max ram slots limit "
> - "of %" PRIu64", when the prior validated limit was %d. "
> - "This limit should never decrease.", ram_slots,
> - u->user->memory_slots);
> - return -1;
> + error_setg(errp, "The backend specified a max ram slots limit "
> + "of %" PRIu64", when the prior validated limit was "
> + "%d. This limit should never decrease.", ram_slots,
> + u->user->memory_slots);
> + return -EINVAL;
> }
>
> u->user->memory_slots = MIN(ram_slots, VHOST_USER_MAX_RAM_SLOTS);
> @@ -1958,7 +1959,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque)
> if (dev->vq_index == 0) {
> err = vhost_setup_slave_channel(dev);
> if (err < 0) {
> - return err;
> + return -EPROTO;
> }
> }
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index ee51863d28..c2aadb57cb 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -273,7 +273,7 @@ static void vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
> vhost_vdpa_call(dev, VHOST_VDPA_SET_STATUS, &s);
> }
>
> -static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque)
> +static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp)
> {
> struct vhost_vdpa *v;
> uint64_t features;
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 991c67ddcd..fd13135706 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1289,9 +1289,9 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> VhostBackendType backend_type, uint32_t busyloop_timeout,
> Error **errp)
> {
> + ERRP_GUARD();
> uint64_t features;
> int i, r, n_initialized_vqs = 0;
> - Error *local_err = NULL;
>
> hdev->vdev = NULL;
> hdev->migration_blocker = NULL;
> @@ -1299,9 +1299,11 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> r = vhost_set_backend_type(hdev, backend_type);
> assert(r >= 0);
>
> - r = hdev->vhost_ops->vhost_backend_init(hdev, opaque);
> + r = hdev->vhost_ops->vhost_backend_init(hdev, opaque, errp);
> if (r < 0) {
> - error_setg(errp, "vhost_backend_init failed");
> + if (!*errp) {
> + error_setg_errno(errp, -r, "vhost_backend_init failed");
> + }
> goto fail;
> }
>
> @@ -1369,9 +1371,8 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> }
>
> if (hdev->migration_blocker != NULL) {
> - r = migrate_add_blocker(hdev->migration_blocker, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> + r = migrate_add_blocker(hdev->migration_blocker, errp);
> + if (*errp) {
> error_free(hdev->migration_blocker);
> goto fail_busyloop;
> }
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 3/7] vhost: Return 0/-errno in vhost_dev_init()
2021-06-09 15:46 [PATCH 0/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
2021-06-09 15:46 ` [PATCH 1/7] vhost: Add Error parameter to vhost_dev_init() Kevin Wolf
2021-06-09 15:46 ` [PATCH 2/7] vhost: Distinguish errors in vhost_backend_init() Kevin Wolf
@ 2021-06-09 15:46 ` Kevin Wolf
2021-06-10 9:09 ` Stefano Garzarella
2021-06-11 19:49 ` Raphael Norwitz
2021-06-09 15:46 ` [PATCH 4/7] vhost-user-blk: Add Error parameter to vhost_user_blk_start() Kevin Wolf
` (4 subsequent siblings)
7 siblings, 2 replies; 23+ messages in thread
From: Kevin Wolf @ 2021-06-09 15:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, raphael.norwitz, qemu-devel, mst
Instead of just returning 0/-1 and letting the caller make up a
meaningless error message, switch to 0/-errno so that different kinds of
errors can be distinguished in the caller.
This involves changing a few more callbacks in VhostOps to return
0/-errno: .vhost_set_owner(), .vhost_get_features() and
.vhost_virtqueue_set_busyloop_timeout(). The implementations of these
functions are trivial as they generally just send a message to the
backend.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
hw/virtio/vhost-backend.c | 4 +++-
hw/virtio/vhost-user.c | 10 +++++++---
hw/virtio/vhost-vdpa.c | 4 +++-
hw/virtio/vhost.c | 8 ++++----
4 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
index f4f71cf58a..594d770b75 100644
--- a/hw/virtio/vhost-backend.c
+++ b/hw/virtio/vhost-backend.c
@@ -24,10 +24,12 @@ static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request,
void *arg)
{
int fd = (uintptr_t) dev->opaque;
+ int ret;
assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL);
- return ioctl(fd, request, arg);
+ ret = ioctl(fd, request, arg);
+ return ret < 0 ? -errno : ret;
}
static int vhost_kernel_init(struct vhost_dev *dev, void *opaque, Error **errp)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 024cb201bb..889559d86a 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1353,7 +1353,11 @@ static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64)
static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
{
- return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
+ if (vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features) < 0) {
+ return -EPROTO;
+ }
+
+ return 0;
}
static int vhost_user_set_owner(struct vhost_dev *dev)
@@ -1364,7 +1368,7 @@ static int vhost_user_set_owner(struct vhost_dev *dev)
};
if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
- return -1;
+ return -EPROTO;
}
return 0;
@@ -1872,7 +1876,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
err = vhost_user_get_features(dev, &features);
if (err < 0) {
- return -EPROTO;
+ return err;
}
if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index c2aadb57cb..71897c1a01 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -253,10 +253,12 @@ static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
{
struct vhost_vdpa *v = dev->opaque;
int fd = v->device_fd;
+ int ret;
assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
- return ioctl(fd, request, arg);
+ ret = ioctl(fd, request, arg);
+ return ret < 0 ? -errno : ret;
}
static void vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index fd13135706..c7f9d8bb06 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1309,13 +1309,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
r = hdev->vhost_ops->vhost_set_owner(hdev);
if (r < 0) {
- error_setg(errp, "vhost_set_owner failed");
+ error_setg_errno(errp, -r, "vhost_set_owner failed");
goto fail;
}
r = hdev->vhost_ops->vhost_get_features(hdev, &features);
if (r < 0) {
- error_setg(errp, "vhost_get_features failed");
+ error_setg_errno(errp, -r, "vhost_get_features failed");
goto fail;
}
@@ -1332,7 +1332,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
r = vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i,
busyloop_timeout);
if (r < 0) {
- error_setg(errp, "Failed to set busyloop timeout");
+ error_setg_errno(errp, -r, "Failed to set busyloop timeout");
goto fail_busyloop;
}
}
@@ -1391,7 +1391,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
error_setg(errp, "vhost backend memory slots limit is less"
" than current number of present memory slots");
- r = -1;
+ r = -EINVAL;
goto fail_busyloop;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 3/7] vhost: Return 0/-errno in vhost_dev_init()
2021-06-09 15:46 ` [PATCH 3/7] vhost: Return 0/-errno in vhost_dev_init() Kevin Wolf
@ 2021-06-10 9:09 ` Stefano Garzarella
2021-06-11 19:49 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Stefano Garzarella @ 2021-06-10 9:09 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:54PM +0200, Kevin Wolf wrote:
>Instead of just returning 0/-1 and letting the caller make up a
>meaningless error message, switch to 0/-errno so that different kinds of
>errors can be distinguished in the caller.
>
>This involves changing a few more callbacks in VhostOps to return
>0/-errno: .vhost_set_owner(), .vhost_get_features() and
>.vhost_virtqueue_set_busyloop_timeout(). The implementations of these
>functions are trivial as they generally just send a message to the
>backend.
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> hw/virtio/vhost-backend.c | 4 +++-
> hw/virtio/vhost-user.c | 10 +++++++---
> hw/virtio/vhost-vdpa.c | 4 +++-
> hw/virtio/vhost.c | 8 ++++----
> 4 files changed, 17 insertions(+), 9 deletions(-)
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 3/7] vhost: Return 0/-errno in vhost_dev_init()
2021-06-09 15:46 ` [PATCH 3/7] vhost: Return 0/-errno in vhost_dev_init() Kevin Wolf
2021-06-10 9:09 ` Stefano Garzarella
@ 2021-06-11 19:49 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Raphael Norwitz @ 2021-06-11 19:49 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Raphael Norwitz, qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:54PM +0200, Kevin Wolf wrote:
> Instead of just returning 0/-1 and letting the caller make up a
> meaningless error message, switch to 0/-errno so that different kinds of
> errors can be distinguished in the caller.
>
> This involves changing a few more callbacks in VhostOps to return
> 0/-errno: .vhost_set_owner(), .vhost_get_features() and
> .vhost_virtqueue_set_busyloop_timeout(). The implementations of these
> functions are trivial as they generally just send a message to the
> backend.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
> hw/virtio/vhost-backend.c | 4 +++-
> hw/virtio/vhost-user.c | 10 +++++++---
> hw/virtio/vhost-vdpa.c | 4 +++-
> hw/virtio/vhost.c | 8 ++++----
> 4 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index f4f71cf58a..594d770b75 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -24,10 +24,12 @@ static int vhost_kernel_call(struct vhost_dev *dev, unsigned long int request,
> void *arg)
> {
> int fd = (uintptr_t) dev->opaque;
> + int ret;
>
> assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_KERNEL);
>
> - return ioctl(fd, request, arg);
> + ret = ioctl(fd, request, arg);
> + return ret < 0 ? -errno : ret;
> }
>
> static int vhost_kernel_init(struct vhost_dev *dev, void *opaque, Error **errp)
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 024cb201bb..889559d86a 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1353,7 +1353,11 @@ static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64)
>
> static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
> {
> - return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
> + if (vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features) < 0) {
> + return -EPROTO;
> + }
> +
> + return 0;
> }
>
> static int vhost_user_set_owner(struct vhost_dev *dev)
> @@ -1364,7 +1368,7 @@ static int vhost_user_set_owner(struct vhost_dev *dev)
> };
>
> if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> - return -1;
> + return -EPROTO;
> }
>
> return 0;
> @@ -1872,7 +1876,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
>
> err = vhost_user_get_features(dev, &features);
> if (err < 0) {
> - return -EPROTO;
> + return err;
> }
>
> if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index c2aadb57cb..71897c1a01 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -253,10 +253,12 @@ static int vhost_vdpa_call(struct vhost_dev *dev, unsigned long int request,
> {
> struct vhost_vdpa *v = dev->opaque;
> int fd = v->device_fd;
> + int ret;
>
> assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
>
> - return ioctl(fd, request, arg);
> + ret = ioctl(fd, request, arg);
> + return ret < 0 ? -errno : ret;
> }
>
> static void vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index fd13135706..c7f9d8bb06 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1309,13 +1309,13 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
>
> r = hdev->vhost_ops->vhost_set_owner(hdev);
> if (r < 0) {
> - error_setg(errp, "vhost_set_owner failed");
> + error_setg_errno(errp, -r, "vhost_set_owner failed");
> goto fail;
> }
>
> r = hdev->vhost_ops->vhost_get_features(hdev, &features);
> if (r < 0) {
> - error_setg(errp, "vhost_get_features failed");
> + error_setg_errno(errp, -r, "vhost_get_features failed");
> goto fail;
> }
>
> @@ -1332,7 +1332,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> r = vhost_virtqueue_set_busyloop_timeout(hdev, hdev->vq_index + i,
> busyloop_timeout);
> if (r < 0) {
> - error_setg(errp, "Failed to set busyloop timeout");
> + error_setg_errno(errp, -r, "Failed to set busyloop timeout");
> goto fail_busyloop;
> }
> }
> @@ -1391,7 +1391,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> if (used_memslots > hdev->vhost_ops->vhost_backend_memslots_limit(hdev)) {
> error_setg(errp, "vhost backend memory slots limit is less"
> " than current number of present memory slots");
> - r = -1;
> + r = -EINVAL;
> goto fail_busyloop;
> }
>
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 4/7] vhost-user-blk: Add Error parameter to vhost_user_blk_start()
2021-06-09 15:46 [PATCH 0/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
` (2 preceding siblings ...)
2021-06-09 15:46 ` [PATCH 3/7] vhost: Return 0/-errno in vhost_dev_init() Kevin Wolf
@ 2021-06-09 15:46 ` Kevin Wolf
2021-06-10 9:11 ` Stefano Garzarella
2021-06-11 19:55 ` Raphael Norwitz
2021-06-09 15:46 ` [PATCH 5/7] vhost: Distinguish errors in vhost_dev_get_config() Kevin Wolf
` (3 subsequent siblings)
7 siblings, 2 replies; 23+ messages in thread
From: Kevin Wolf @ 2021-06-09 15:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, raphael.norwitz, qemu-devel, mst
Instead of letting the caller make up a meaningless error message, add
an Error parameter to allow reporting the real error.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
hw/block/vhost-user-blk.c | 31 +++++++++++++++----------------
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 0cb56baefb..e9382e152a 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -113,7 +113,7 @@ const VhostDevConfigOps blk_ops = {
.vhost_dev_config_notifier = vhost_user_blk_handle_config_change,
};
-static int vhost_user_blk_start(VirtIODevice *vdev)
+static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
{
VHostUserBlk *s = VHOST_USER_BLK(vdev);
BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
@@ -121,19 +121,19 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
int i, ret;
if (!k->set_guest_notifiers) {
- error_report("binding does not support guest notifiers");
+ error_setg(errp, "binding does not support guest notifiers");
return -ENOSYS;
}
ret = vhost_dev_enable_notifiers(&s->dev, vdev);
if (ret < 0) {
- error_report("Error enabling host notifiers: %d", -ret);
+ error_setg_errno(errp, -ret, "Error enabling host notifiers");
return ret;
}
ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, true);
if (ret < 0) {
- error_report("Error binding guest notifier: %d", -ret);
+ error_setg_errno(errp, -ret, "Error binding guest notifier");
goto err_host_notifiers;
}
@@ -141,27 +141,27 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
ret = vhost_dev_prepare_inflight(&s->dev, vdev);
if (ret < 0) {
- error_report("Error set inflight format: %d", -ret);
+ error_setg_errno(errp, -ret, "Error setting inflight format");
goto err_guest_notifiers;
}
if (!s->inflight->addr) {
ret = vhost_dev_get_inflight(&s->dev, s->queue_size, s->inflight);
if (ret < 0) {
- error_report("Error get inflight: %d", -ret);
+ error_setg_errno(errp, -ret, "Error getting inflight");
goto err_guest_notifiers;
}
}
ret = vhost_dev_set_inflight(&s->dev, s->inflight);
if (ret < 0) {
- error_report("Error set inflight: %d", -ret);
+ error_setg_errno(errp, -ret, "Error setting inflight");
goto err_guest_notifiers;
}
ret = vhost_dev_start(&s->dev, vdev);
if (ret < 0) {
- error_report("Error starting vhost: %d", -ret);
+ error_setg_errno(errp, -ret, "Error starting vhost");
goto err_guest_notifiers;
}
s->started_vu = true;
@@ -214,6 +214,7 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
{
VHostUserBlk *s = VHOST_USER_BLK(vdev);
bool should_start = virtio_device_started(vdev, status);
+ Error *local_err = NULL;
int ret;
if (!vdev->vm_running) {
@@ -229,10 +230,9 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
}
if (should_start) {
- ret = vhost_user_blk_start(vdev);
+ ret = vhost_user_blk_start(vdev, &local_err);
if (ret < 0) {
- error_report("vhost-user-blk: vhost start failed: %s",
- strerror(-ret));
+ error_reportf_err(local_err, "vhost-user-blk: vhost start failed: ");
qemu_chr_fe_disconnect(&s->chardev);
}
} else {
@@ -270,6 +270,7 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
{
VHostUserBlk *s = VHOST_USER_BLK(vdev);
+ Error *local_err = NULL;
int i, ret;
if (!vdev->start_on_kick) {
@@ -287,10 +288,9 @@ static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
/* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
* vhost here instead of waiting for .set_status().
*/
- ret = vhost_user_blk_start(vdev);
+ ret = vhost_user_blk_start(vdev, &local_err);
if (ret < 0) {
- error_report("vhost-user-blk: vhost start failed: %s",
- strerror(-ret));
+ error_reportf_err(local_err, "vhost-user-blk: vhost start failed: ");
qemu_chr_fe_disconnect(&s->chardev);
return;
}
@@ -340,9 +340,8 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
/* restore vhost state */
if (virtio_device_started(vdev, vdev->status)) {
- ret = vhost_user_blk_start(vdev);
+ ret = vhost_user_blk_start(vdev, errp);
if (ret < 0) {
- error_setg_errno(errp, -ret, "vhost start failed");
return ret;
}
}
--
2.30.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 4/7] vhost-user-blk: Add Error parameter to vhost_user_blk_start()
2021-06-09 15:46 ` [PATCH 4/7] vhost-user-blk: Add Error parameter to vhost_user_blk_start() Kevin Wolf
@ 2021-06-10 9:11 ` Stefano Garzarella
2021-06-11 19:55 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Stefano Garzarella @ 2021-06-10 9:11 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:55PM +0200, Kevin Wolf wrote:
>Instead of letting the caller make up a meaningless error message, add
>an Error parameter to allow reporting the real error.
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> hw/block/vhost-user-blk.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 4/7] vhost-user-blk: Add Error parameter to vhost_user_blk_start()
2021-06-09 15:46 ` [PATCH 4/7] vhost-user-blk: Add Error parameter to vhost_user_blk_start() Kevin Wolf
2021-06-10 9:11 ` Stefano Garzarella
@ 2021-06-11 19:55 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Raphael Norwitz @ 2021-06-11 19:55 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Raphael Norwitz, qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:55PM +0200, Kevin Wolf wrote:
> Instead of letting the caller make up a meaningless error message, add
> an Error parameter to allow reporting the real error.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
> hw/block/vhost-user-blk.c | 31 +++++++++++++++----------------
> 1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 0cb56baefb..e9382e152a 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -113,7 +113,7 @@ const VhostDevConfigOps blk_ops = {
> .vhost_dev_config_notifier = vhost_user_blk_handle_config_change,
> };
>
> -static int vhost_user_blk_start(VirtIODevice *vdev)
> +static int vhost_user_blk_start(VirtIODevice *vdev, Error **errp)
> {
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> @@ -121,19 +121,19 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
> int i, ret;
>
> if (!k->set_guest_notifiers) {
> - error_report("binding does not support guest notifiers");
> + error_setg(errp, "binding does not support guest notifiers");
> return -ENOSYS;
> }
>
> ret = vhost_dev_enable_notifiers(&s->dev, vdev);
> if (ret < 0) {
> - error_report("Error enabling host notifiers: %d", -ret);
> + error_setg_errno(errp, -ret, "Error enabling host notifiers");
> return ret;
> }
>
> ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, true);
> if (ret < 0) {
> - error_report("Error binding guest notifier: %d", -ret);
> + error_setg_errno(errp, -ret, "Error binding guest notifier");
> goto err_host_notifiers;
> }
>
> @@ -141,27 +141,27 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
>
> ret = vhost_dev_prepare_inflight(&s->dev, vdev);
> if (ret < 0) {
> - error_report("Error set inflight format: %d", -ret);
> + error_setg_errno(errp, -ret, "Error setting inflight format");
> goto err_guest_notifiers;
> }
>
> if (!s->inflight->addr) {
> ret = vhost_dev_get_inflight(&s->dev, s->queue_size, s->inflight);
> if (ret < 0) {
> - error_report("Error get inflight: %d", -ret);
> + error_setg_errno(errp, -ret, "Error getting inflight");
> goto err_guest_notifiers;
> }
> }
>
> ret = vhost_dev_set_inflight(&s->dev, s->inflight);
> if (ret < 0) {
> - error_report("Error set inflight: %d", -ret);
> + error_setg_errno(errp, -ret, "Error setting inflight");
> goto err_guest_notifiers;
> }
>
> ret = vhost_dev_start(&s->dev, vdev);
> if (ret < 0) {
> - error_report("Error starting vhost: %d", -ret);
> + error_setg_errno(errp, -ret, "Error starting vhost");
> goto err_guest_notifiers;
> }
> s->started_vu = true;
> @@ -214,6 +214,7 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
> {
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
> bool should_start = virtio_device_started(vdev, status);
> + Error *local_err = NULL;
> int ret;
>
> if (!vdev->vm_running) {
> @@ -229,10 +230,9 @@ static void vhost_user_blk_set_status(VirtIODevice *vdev, uint8_t status)
> }
>
> if (should_start) {
> - ret = vhost_user_blk_start(vdev);
> + ret = vhost_user_blk_start(vdev, &local_err);
> if (ret < 0) {
> - error_report("vhost-user-blk: vhost start failed: %s",
> - strerror(-ret));
> + error_reportf_err(local_err, "vhost-user-blk: vhost start failed: ");
> qemu_chr_fe_disconnect(&s->chardev);
> }
> } else {
> @@ -270,6 +270,7 @@ static uint64_t vhost_user_blk_get_features(VirtIODevice *vdev,
> static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> {
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
> + Error *local_err = NULL;
> int i, ret;
>
> if (!vdev->start_on_kick) {
> @@ -287,10 +288,9 @@ static void vhost_user_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
> * vhost here instead of waiting for .set_status().
> */
> - ret = vhost_user_blk_start(vdev);
> + ret = vhost_user_blk_start(vdev, &local_err);
> if (ret < 0) {
> - error_report("vhost-user-blk: vhost start failed: %s",
> - strerror(-ret));
> + error_reportf_err(local_err, "vhost-user-blk: vhost start failed: ");
> qemu_chr_fe_disconnect(&s->chardev);
> return;
> }
> @@ -340,9 +340,8 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
>
> /* restore vhost state */
> if (virtio_device_started(vdev, vdev->status)) {
> - ret = vhost_user_blk_start(vdev);
> + ret = vhost_user_blk_start(vdev, errp);
> if (ret < 0) {
> - error_setg_errno(errp, -ret, "vhost start failed");
> return ret;
> }
> }
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 5/7] vhost: Distinguish errors in vhost_dev_get_config()
2021-06-09 15:46 [PATCH 0/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
` (3 preceding siblings ...)
2021-06-09 15:46 ` [PATCH 4/7] vhost-user-blk: Add Error parameter to vhost_user_blk_start() Kevin Wolf
@ 2021-06-09 15:46 ` Kevin Wolf
2021-06-10 9:16 ` Stefano Garzarella
2021-06-11 20:08 ` Raphael Norwitz
2021-06-09 15:46 ` [PATCH 6/7] vhost-user-blk: Factor out vhost_user_blk_realize_connect() Kevin Wolf
` (2 subsequent siblings)
7 siblings, 2 replies; 23+ messages in thread
From: Kevin Wolf @ 2021-06-09 15:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, raphael.norwitz, qemu-devel, mst
Instead of just returning 0/-1 and letting the caller make up a
meaningless error message, add an Error parameter to allow reporting the
real error and switch to 0/-errno so that different kind of errors can
be distinguished in the caller.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/hw/virtio/vhost-backend.h | 2 +-
include/hw/virtio/vhost.h | 4 ++--
hw/block/vhost-user-blk.c | 9 +++++----
hw/display/vhost-user-gpu.c | 6 ++++--
hw/input/vhost-user-input.c | 6 ++++--
hw/net/vhost_net.c | 2 +-
hw/virtio/vhost-user-vsock.c | 9 +++++----
hw/virtio/vhost-user.c | 24 ++++++++++++------------
hw/virtio/vhost-vdpa.c | 2 +-
hw/virtio/vhost.c | 14 +++++++++++---
10 files changed, 46 insertions(+), 32 deletions(-)
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index 728ebb0ed9..8475c5a29d 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -98,7 +98,7 @@ typedef int (*vhost_set_config_op)(struct vhost_dev *dev, const uint8_t *data,
uint32_t offset, uint32_t size,
uint32_t flags);
typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t *config,
- uint32_t config_len);
+ uint32_t config_len, Error **errp);
typedef int (*vhost_crypto_create_session_op)(struct vhost_dev *dev,
void *session_info,
diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
index 2d7aaad67b..045d0fd9f2 100644
--- a/include/hw/virtio/vhost.h
+++ b/include/hw/virtio/vhost.h
@@ -130,8 +130,8 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
struct vhost_vring_file *file);
int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write);
-int vhost_dev_get_config(struct vhost_dev *dev, uint8_t *config,
- uint32_t config_len);
+int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
+ uint32_t config_len, Error **errp);
int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data,
uint32_t offset, uint32_t size, uint32_t flags);
/* notifier callback in case vhost device config space changed
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index e9382e152a..3770f715da 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -91,11 +91,13 @@ static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
int ret;
struct virtio_blk_config blkcfg;
VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
+ Error *local_err = NULL;
ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
- sizeof(struct virtio_blk_config));
+ sizeof(struct virtio_blk_config),
+ &local_err);
if (ret < 0) {
- error_report("get config space failed");
+ error_report_err(local_err);
return -1;
}
@@ -478,9 +480,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
assert(s->connected);
ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
- sizeof(struct virtio_blk_config));
+ sizeof(struct virtio_blk_config), errp);
if (ret < 0) {
- error_setg(errp, "vhost-user-blk: get block config failed");
goto vhost_err;
}
diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 6cdaa1c73b..389199e6ca 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -415,14 +415,16 @@ vhost_user_gpu_get_config(VirtIODevice *vdev, uint8_t *config_data)
VirtIOGPUBase *b = VIRTIO_GPU_BASE(vdev);
struct virtio_gpu_config *vgconfig =
(struct virtio_gpu_config *)config_data;
+ Error *local_err = NULL;
int ret;
memset(config_data, 0, sizeof(struct virtio_gpu_config));
ret = vhost_dev_get_config(&g->vhost->dev,
- config_data, sizeof(struct virtio_gpu_config));
+ config_data, sizeof(struct virtio_gpu_config),
+ &local_err);
if (ret) {
- error_report("vhost-user-gpu: get device config space failed");
+ error_report_err(local_err);
return;
}
diff --git a/hw/input/vhost-user-input.c b/hw/input/vhost-user-input.c
index 63984a8ba7..273e96a7b1 100644
--- a/hw/input/vhost-user-input.c
+++ b/hw/input/vhost-user-input.c
@@ -49,13 +49,15 @@ static void vhost_input_get_config(VirtIODevice *vdev, uint8_t *config_data)
{
VirtIOInput *vinput = VIRTIO_INPUT(vdev);
VHostUserInput *vhi = VHOST_USER_INPUT(vdev);
+ Error *local_err = NULL;
int ret;
memset(config_data, 0, vinput->cfg_size);
- ret = vhost_dev_get_config(&vhi->vhost->dev, config_data, vinput->cfg_size);
+ ret = vhost_dev_get_config(&vhi->vhost->dev, config_data, vinput->cfg_size,
+ &local_err);
if (ret) {
- error_report("vhost-user-input: get device config space failed");
+ error_report_err(local_err);
return;
}
}
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 447b119f85..10a7780a13 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -117,7 +117,7 @@ uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
int vhost_net_get_config(struct vhost_net *net, uint8_t *config,
uint32_t config_len)
{
- return vhost_dev_get_config(&net->dev, config, config_len);
+ return vhost_dev_get_config(&net->dev, config, config_len, NULL);
}
int vhost_net_set_config(struct vhost_net *net, const uint8_t *data,
uint32_t offset, uint32_t size, uint32_t flags)
diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
index b6a4a25ea1..6095ed7349 100644
--- a/hw/virtio/vhost-user-vsock.c
+++ b/hw/virtio/vhost-user-vsock.c
@@ -34,10 +34,12 @@ static void vuv_get_config(VirtIODevice *vdev, uint8_t *config)
static int vuv_handle_config_change(struct vhost_dev *dev)
{
VHostUserVSock *vsock = VHOST_USER_VSOCK(dev->vdev);
+ Error *local_err = NULL;
int ret = vhost_dev_get_config(dev, (uint8_t *)&vsock->vsockcfg,
- sizeof(struct virtio_vsock_config));
+ sizeof(struct virtio_vsock_config),
+ &local_err);
if (ret < 0) {
- error_report("get config space failed");
+ error_report_err(local_err);
return -1;
}
@@ -114,9 +116,8 @@ static void vuv_device_realize(DeviceState *dev, Error **errp)
}
ret = vhost_dev_get_config(&vvc->vhost_dev, (uint8_t *)&vsock->vsockcfg,
- sizeof(struct virtio_vsock_config));
+ sizeof(struct virtio_vsock_config), errp);
if (ret < 0) {
- error_setg_errno(errp, -ret, "get config space failed");
goto err_vhost_dev;
}
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 889559d86a..1ac4a2ebec 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2117,7 +2117,7 @@ static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
}
static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
- uint32_t config_len)
+ uint32_t config_len, Error **errp)
{
VhostUserMsg msg = {
.hdr.request = VHOST_USER_GET_CONFIG,
@@ -2127,32 +2127,32 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
if (!virtio_has_feature(dev->protocol_features,
VHOST_USER_PROTOCOL_F_CONFIG)) {
- return -1;
+ error_setg(errp, "VHOST_USER_PROTOCOL_F_CONFIG not supported");
+ return -EINVAL;
}
- if (config_len > VHOST_USER_MAX_CONFIG_SIZE) {
- return -1;
- }
+ assert(config_len <= VHOST_USER_MAX_CONFIG_SIZE);
msg.payload.config.offset = 0;
msg.payload.config.size = config_len;
if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
- return -1;
+ return -EPROTO;
}
if (vhost_user_read(dev, &msg) < 0) {
- return -1;
+ return -EPROTO;
}
if (msg.hdr.request != VHOST_USER_GET_CONFIG) {
- error_report("Received unexpected msg type. Expected %d received %d",
- VHOST_USER_GET_CONFIG, msg.hdr.request);
- return -1;
+ error_setg(errp,
+ "Received unexpected msg type. Expected %d received %d",
+ VHOST_USER_GET_CONFIG, msg.hdr.request);
+ return -EINVAL;
}
if (msg.hdr.size != VHOST_USER_CONFIG_HDR_SIZE + config_len) {
- error_report("Received bad msg size.");
- return -1;
+ error_setg(errp, "Received bad msg size.");
+ return -EINVAL;
}
memcpy(config, msg.payload.config.region, config_len);
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 71897c1a01..6b6f974a83 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -451,7 +451,7 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data,
}
static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
- uint32_t config_len)
+ uint32_t config_len, Error **errp)
{
struct vhost_vdpa_config *v_config;
unsigned long config_size = offsetof(struct vhost_vdpa_config, buf);
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index c7f9d8bb06..1b66f1de70 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1562,15 +1562,23 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
}
int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
- uint32_t config_len)
+ uint32_t config_len, Error **errp)
{
+ ERRP_GUARD();
+ int ret;
+
assert(hdev->vhost_ops);
if (hdev->vhost_ops->vhost_get_config) {
- return hdev->vhost_ops->vhost_get_config(hdev, config, config_len);
+ ret = hdev->vhost_ops->vhost_get_config(hdev, config, config_len, errp);
+ if (ret < 0 && !*errp) {
+ error_setg_errno(errp, -ret, "vhost_get_config failed");
+ }
+ return ret;
}
- return -1;
+ error_setg(errp, "vhost_dev_get_config not implemented");
+ return -ENOTSUP;
}
int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data,
--
2.30.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 5/7] vhost: Distinguish errors in vhost_dev_get_config()
2021-06-09 15:46 ` [PATCH 5/7] vhost: Distinguish errors in vhost_dev_get_config() Kevin Wolf
@ 2021-06-10 9:16 ` Stefano Garzarella
2021-06-11 20:08 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Stefano Garzarella @ 2021-06-10 9:16 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:56PM +0200, Kevin Wolf wrote:
>Instead of just returning 0/-1 and letting the caller make up a
>meaningless error message, add an Error parameter to allow reporting the
>real error and switch to 0/-errno so that different kind of errors can
>be distinguished in the caller.
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> include/hw/virtio/vhost-backend.h | 2 +-
> include/hw/virtio/vhost.h | 4 ++--
> hw/block/vhost-user-blk.c | 9 +++++----
> hw/display/vhost-user-gpu.c | 6 ++++--
> hw/input/vhost-user-input.c | 6 ++++--
> hw/net/vhost_net.c | 2 +-
> hw/virtio/vhost-user-vsock.c | 9 +++++----
> hw/virtio/vhost-user.c | 24 ++++++++++++------------
> hw/virtio/vhost-vdpa.c | 2 +-
> hw/virtio/vhost.c | 14 +++++++++++---
> 10 files changed, 46 insertions(+), 32 deletions(-)
>
>diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
>index 728ebb0ed9..8475c5a29d 100644
>--- a/include/hw/virtio/vhost-backend.h
>+++ b/include/hw/virtio/vhost-backend.h
>@@ -98,7 +98,7 @@ typedef int (*vhost_set_config_op)(struct vhost_dev *dev, const uint8_t *data,
> uint32_t offset, uint32_t size,
> uint32_t flags);
> typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t *config,
>- uint32_t config_len);
>+ uint32_t config_len, Error **errp);
>
> typedef int (*vhost_crypto_create_session_op)(struct vhost_dev *dev,
> void *session_info,
>diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
>index 2d7aaad67b..045d0fd9f2 100644
>--- a/include/hw/virtio/vhost.h
>+++ b/include/hw/virtio/vhost.h
>@@ -130,8 +130,8 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
> struct vhost_vring_file *file);
>
> int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write);
>-int vhost_dev_get_config(struct vhost_dev *dev, uint8_t *config,
>- uint32_t config_len);
>+int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
>+ uint32_t config_len, Error **errp);
> int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data,
> uint32_t offset, uint32_t size, uint32_t flags);
> /* notifier callback in case vhost device config space changed
>diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>index e9382e152a..3770f715da 100644
>--- a/hw/block/vhost-user-blk.c
>+++ b/hw/block/vhost-user-blk.c
>@@ -91,11 +91,13 @@ static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
> int ret;
> struct virtio_blk_config blkcfg;
> VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
>+ Error *local_err = NULL;
>
> ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
>- sizeof(struct virtio_blk_config));
>+ sizeof(struct virtio_blk_config),
>+ &local_err);
> if (ret < 0) {
>- error_report("get config space failed");
>+ error_report_err(local_err);
> return -1;
> }
>
>@@ -478,9 +480,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> assert(s->connected);
>
> ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
>- sizeof(struct virtio_blk_config));
>+ sizeof(struct virtio_blk_config), errp);
> if (ret < 0) {
>- error_setg(errp, "vhost-user-blk: get block config failed");
> goto vhost_err;
> }
>
>diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
>index 6cdaa1c73b..389199e6ca 100644
>--- a/hw/display/vhost-user-gpu.c
>+++ b/hw/display/vhost-user-gpu.c
>@@ -415,14 +415,16 @@ vhost_user_gpu_get_config(VirtIODevice *vdev, uint8_t *config_data)
> VirtIOGPUBase *b = VIRTIO_GPU_BASE(vdev);
> struct virtio_gpu_config *vgconfig =
> (struct virtio_gpu_config *)config_data;
>+ Error *local_err = NULL;
> int ret;
>
> memset(config_data, 0, sizeof(struct virtio_gpu_config));
>
> ret = vhost_dev_get_config(&g->vhost->dev,
>- config_data, sizeof(struct virtio_gpu_config));
>+ config_data, sizeof(struct virtio_gpu_config),
>+ &local_err);
> if (ret) {
>- error_report("vhost-user-gpu: get device config space failed");
>+ error_report_err(local_err);
> return;
> }
>
>diff --git a/hw/input/vhost-user-input.c b/hw/input/vhost-user-input.c
>index 63984a8ba7..273e96a7b1 100644
>--- a/hw/input/vhost-user-input.c
>+++ b/hw/input/vhost-user-input.c
>@@ -49,13 +49,15 @@ static void vhost_input_get_config(VirtIODevice *vdev, uint8_t *config_data)
> {
> VirtIOInput *vinput = VIRTIO_INPUT(vdev);
> VHostUserInput *vhi = VHOST_USER_INPUT(vdev);
>+ Error *local_err = NULL;
> int ret;
>
> memset(config_data, 0, vinput->cfg_size);
>
>- ret = vhost_dev_get_config(&vhi->vhost->dev, config_data, vinput->cfg_size);
>+ ret = vhost_dev_get_config(&vhi->vhost->dev, config_data, vinput->cfg_size,
>+ &local_err);
> if (ret) {
>- error_report("vhost-user-input: get device config space failed");
>+ error_report_err(local_err);
> return;
> }
> }
>diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>index 447b119f85..10a7780a13 100644
>--- a/hw/net/vhost_net.c
>+++ b/hw/net/vhost_net.c
>@@ -117,7 +117,7 @@ uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> int vhost_net_get_config(struct vhost_net *net, uint8_t *config,
> uint32_t config_len)
> {
>- return vhost_dev_get_config(&net->dev, config, config_len);
>+ return vhost_dev_get_config(&net->dev, config, config_len, NULL);
> }
> int vhost_net_set_config(struct vhost_net *net, const uint8_t *data,
> uint32_t offset, uint32_t size, uint32_t flags)
>diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
>index b6a4a25ea1..6095ed7349 100644
>--- a/hw/virtio/vhost-user-vsock.c
>+++ b/hw/virtio/vhost-user-vsock.c
>@@ -34,10 +34,12 @@ static void vuv_get_config(VirtIODevice *vdev, uint8_t *config)
> static int vuv_handle_config_change(struct vhost_dev *dev)
> {
> VHostUserVSock *vsock = VHOST_USER_VSOCK(dev->vdev);
>+ Error *local_err = NULL;
> int ret = vhost_dev_get_config(dev, (uint8_t *)&vsock->vsockcfg,
>- sizeof(struct virtio_vsock_config));
>+ sizeof(struct virtio_vsock_config),
>+ &local_err);
> if (ret < 0) {
>- error_report("get config space failed");
>+ error_report_err(local_err);
> return -1;
> }
>
>@@ -114,9 +116,8 @@ static void vuv_device_realize(DeviceState *dev, Error **errp)
> }
>
> ret = vhost_dev_get_config(&vvc->vhost_dev, (uint8_t *)&vsock->vsockcfg,
>- sizeof(struct virtio_vsock_config));
>+ sizeof(struct virtio_vsock_config), errp);
> if (ret < 0) {
>- error_setg_errno(errp, -ret, "get config space failed");
> goto err_vhost_dev;
> }
>
>diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>index 889559d86a..1ac4a2ebec 100644
>--- a/hw/virtio/vhost-user.c
>+++ b/hw/virtio/vhost-user.c
>@@ -2117,7 +2117,7 @@ static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
> }
>
> static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
>- uint32_t config_len)
>+ uint32_t config_len, Error **errp)
> {
> VhostUserMsg msg = {
> .hdr.request = VHOST_USER_GET_CONFIG,
>@@ -2127,32 +2127,32 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
>
> if (!virtio_has_feature(dev->protocol_features,
> VHOST_USER_PROTOCOL_F_CONFIG)) {
>- return -1;
>+ error_setg(errp, "VHOST_USER_PROTOCOL_F_CONFIG not supported");
>+ return -EINVAL;
> }
>
>- if (config_len > VHOST_USER_MAX_CONFIG_SIZE) {
>- return -1;
>- }
>+ assert(config_len <= VHOST_USER_MAX_CONFIG_SIZE);
>
> msg.payload.config.offset = 0;
> msg.payload.config.size = config_len;
> if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>- return -1;
>+ return -EPROTO;
> }
>
> if (vhost_user_read(dev, &msg) < 0) {
>- return -1;
>+ return -EPROTO;
> }
>
> if (msg.hdr.request != VHOST_USER_GET_CONFIG) {
>- error_report("Received unexpected msg type. Expected %d received %d",
>- VHOST_USER_GET_CONFIG, msg.hdr.request);
>- return -1;
>+ error_setg(errp,
>+ "Received unexpected msg type. Expected %d received %d",
>+ VHOST_USER_GET_CONFIG, msg.hdr.request);
>+ return -EINVAL;
> }
>
> if (msg.hdr.size != VHOST_USER_CONFIG_HDR_SIZE + config_len) {
>- error_report("Received bad msg size.");
>- return -1;
>+ error_setg(errp, "Received bad msg size.");
>+ return -EINVAL;
> }
>
> memcpy(config, msg.payload.config.region, config_len);
>diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>index 71897c1a01..6b6f974a83 100644
>--- a/hw/virtio/vhost-vdpa.c
>+++ b/hw/virtio/vhost-vdpa.c
>@@ -451,7 +451,7 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data,
> }
>
> static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
>- uint32_t config_len)
>+ uint32_t config_len, Error **errp)
> {
> struct vhost_vdpa_config *v_config;
> unsigned long config_size = offsetof(struct vhost_vdpa_config, buf);
>diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>index c7f9d8bb06..1b66f1de70 100644
>--- a/hw/virtio/vhost.c
>+++ b/hw/virtio/vhost.c
>@@ -1562,15 +1562,23 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
> }
>
> int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
>- uint32_t config_len)
>+ uint32_t config_len, Error **errp)
> {
>+ ERRP_GUARD();
>+ int ret;
>+
> assert(hdev->vhost_ops);
>
> if (hdev->vhost_ops->vhost_get_config) {
>- return hdev->vhost_ops->vhost_get_config(hdev, config, config_len);
>+ ret = hdev->vhost_ops->vhost_get_config(hdev, config, config_len, errp);
>+ if (ret < 0 && !*errp) {
>+ error_setg_errno(errp, -ret, "vhost_get_config failed");
>+ }
>+ return ret;
> }
>
>- return -1;
>+ error_setg(errp, "vhost_dev_get_config not implemented");
^
Maybe I'd replace s/vhost_dev_get_config/vhost_get_config
But it's not that important:
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 5/7] vhost: Distinguish errors in vhost_dev_get_config()
2021-06-09 15:46 ` [PATCH 5/7] vhost: Distinguish errors in vhost_dev_get_config() Kevin Wolf
2021-06-10 9:16 ` Stefano Garzarella
@ 2021-06-11 20:08 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Raphael Norwitz @ 2021-06-11 20:08 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Raphael Norwitz, qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:56PM +0200, Kevin Wolf wrote:
> Instead of just returning 0/-1 and letting the caller make up a
> meaningless error message, add an Error parameter to allow reporting the
> real error and switch to 0/-errno so that different kind of errors can
> be distinguished in the caller.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Just one commmit message suggestion.
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
> include/hw/virtio/vhost-backend.h | 2 +-
> include/hw/virtio/vhost.h | 4 ++--
> hw/block/vhost-user-blk.c | 9 +++++----
> hw/display/vhost-user-gpu.c | 6 ++++--
> hw/input/vhost-user-input.c | 6 ++++--
> hw/net/vhost_net.c | 2 +-
> hw/virtio/vhost-user-vsock.c | 9 +++++----
> hw/virtio/vhost-user.c | 24 ++++++++++++------------
> hw/virtio/vhost-vdpa.c | 2 +-
> hw/virtio/vhost.c | 14 +++++++++++---
> 10 files changed, 46 insertions(+), 32 deletions(-)
>
> diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
> index 728ebb0ed9..8475c5a29d 100644
> --- a/include/hw/virtio/vhost-backend.h
> +++ b/include/hw/virtio/vhost-backend.h
> @@ -98,7 +98,7 @@ typedef int (*vhost_set_config_op)(struct vhost_dev *dev, const uint8_t *data,
> uint32_t offset, uint32_t size,
> uint32_t flags);
> typedef int (*vhost_get_config_op)(struct vhost_dev *dev, uint8_t *config,
> - uint32_t config_len);
> + uint32_t config_len, Error **errp);
>
> typedef int (*vhost_crypto_create_session_op)(struct vhost_dev *dev,
> void *session_info,
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> index 2d7aaad67b..045d0fd9f2 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -130,8 +130,8 @@ int vhost_net_set_backend(struct vhost_dev *hdev,
> struct vhost_vring_file *file);
>
> int vhost_device_iotlb_miss(struct vhost_dev *dev, uint64_t iova, int write);
> -int vhost_dev_get_config(struct vhost_dev *dev, uint8_t *config,
> - uint32_t config_len);
> +int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
> + uint32_t config_len, Error **errp);
> int vhost_dev_set_config(struct vhost_dev *dev, const uint8_t *data,
> uint32_t offset, uint32_t size, uint32_t flags);
> /* notifier callback in case vhost device config space changed
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index e9382e152a..3770f715da 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -91,11 +91,13 @@ static int vhost_user_blk_handle_config_change(struct vhost_dev *dev)
> int ret;
> struct virtio_blk_config blkcfg;
> VHostUserBlk *s = VHOST_USER_BLK(dev->vdev);
> + Error *local_err = NULL;
>
> ret = vhost_dev_get_config(dev, (uint8_t *)&blkcfg,
> - sizeof(struct virtio_blk_config));
> + sizeof(struct virtio_blk_config),
> + &local_err);
> if (ret < 0) {
> - error_report("get config space failed");
> + error_report_err(local_err);
> return -1;
> }
>
> @@ -478,9 +480,8 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> assert(s->connected);
>
> ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
> - sizeof(struct virtio_blk_config));
> + sizeof(struct virtio_blk_config), errp);
> if (ret < 0) {
> - error_setg(errp, "vhost-user-blk: get block config failed");
> goto vhost_err;
> }
>
> diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
> index 6cdaa1c73b..389199e6ca 100644
> --- a/hw/display/vhost-user-gpu.c
> +++ b/hw/display/vhost-user-gpu.c
> @@ -415,14 +415,16 @@ vhost_user_gpu_get_config(VirtIODevice *vdev, uint8_t *config_data)
> VirtIOGPUBase *b = VIRTIO_GPU_BASE(vdev);
> struct virtio_gpu_config *vgconfig =
> (struct virtio_gpu_config *)config_data;
> + Error *local_err = NULL;
> int ret;
>
> memset(config_data, 0, sizeof(struct virtio_gpu_config));
>
> ret = vhost_dev_get_config(&g->vhost->dev,
> - config_data, sizeof(struct virtio_gpu_config));
> + config_data, sizeof(struct virtio_gpu_config),
> + &local_err);
> if (ret) {
> - error_report("vhost-user-gpu: get device config space failed");
> + error_report_err(local_err);
> return;
> }
>
> diff --git a/hw/input/vhost-user-input.c b/hw/input/vhost-user-input.c
> index 63984a8ba7..273e96a7b1 100644
> --- a/hw/input/vhost-user-input.c
> +++ b/hw/input/vhost-user-input.c
> @@ -49,13 +49,15 @@ static void vhost_input_get_config(VirtIODevice *vdev, uint8_t *config_data)
> {
> VirtIOInput *vinput = VIRTIO_INPUT(vdev);
> VHostUserInput *vhi = VHOST_USER_INPUT(vdev);
> + Error *local_err = NULL;
> int ret;
>
> memset(config_data, 0, vinput->cfg_size);
>
> - ret = vhost_dev_get_config(&vhi->vhost->dev, config_data, vinput->cfg_size);
> + ret = vhost_dev_get_config(&vhi->vhost->dev, config_data, vinput->cfg_size,
> + &local_err);
> if (ret) {
> - error_report("vhost-user-input: get device config space failed");
> + error_report_err(local_err);
> return;
> }
> }
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 447b119f85..10a7780a13 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -117,7 +117,7 @@ uint64_t vhost_net_get_features(struct vhost_net *net, uint64_t features)
> int vhost_net_get_config(struct vhost_net *net, uint8_t *config,
> uint32_t config_len)
> {
> - return vhost_dev_get_config(&net->dev, config, config_len);
> + return vhost_dev_get_config(&net->dev, config, config_len, NULL);
> }
> int vhost_net_set_config(struct vhost_net *net, const uint8_t *data,
> uint32_t offset, uint32_t size, uint32_t flags)
> diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> index b6a4a25ea1..6095ed7349 100644
> --- a/hw/virtio/vhost-user-vsock.c
> +++ b/hw/virtio/vhost-user-vsock.c
> @@ -34,10 +34,12 @@ static void vuv_get_config(VirtIODevice *vdev, uint8_t *config)
> static int vuv_handle_config_change(struct vhost_dev *dev)
> {
> VHostUserVSock *vsock = VHOST_USER_VSOCK(dev->vdev);
> + Error *local_err = NULL;
> int ret = vhost_dev_get_config(dev, (uint8_t *)&vsock->vsockcfg,
> - sizeof(struct virtio_vsock_config));
> + sizeof(struct virtio_vsock_config),
> + &local_err);
> if (ret < 0) {
> - error_report("get config space failed");
> + error_report_err(local_err);
> return -1;
> }
>
> @@ -114,9 +116,8 @@ static void vuv_device_realize(DeviceState *dev, Error **errp)
> }
>
> ret = vhost_dev_get_config(&vvc->vhost_dev, (uint8_t *)&vsock->vsockcfg,
> - sizeof(struct virtio_vsock_config));
> + sizeof(struct virtio_vsock_config), errp);
> if (ret < 0) {
> - error_setg_errno(errp, -ret, "get config space failed");
> goto err_vhost_dev;
> }
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 889559d86a..1ac4a2ebec 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -2117,7 +2117,7 @@ static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
> }
>
> static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
> - uint32_t config_len)
> + uint32_t config_len, Error **errp)
> {
> VhostUserMsg msg = {
> .hdr.request = VHOST_USER_GET_CONFIG,
> @@ -2127,32 +2127,32 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
>
> if (!virtio_has_feature(dev->protocol_features,
> VHOST_USER_PROTOCOL_F_CONFIG)) {
> - return -1;
> + error_setg(errp, "VHOST_USER_PROTOCOL_F_CONFIG not supported");
> + return -EINVAL;
> }
>
> - if (config_len > VHOST_USER_MAX_CONFIG_SIZE) {
> - return -1;
> - }
> + assert(config_len <= VHOST_USER_MAX_CONFIG_SIZE);
Maybe just note that you are changing the check to an assert in the
commit message since it is a functional change?
>
> msg.payload.config.offset = 0;
> msg.payload.config.size = config_len;
> if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> - return -1;
> + return -EPROTO;
> }
>
> if (vhost_user_read(dev, &msg) < 0) {
> - return -1;
> + return -EPROTO;
> }
>
> if (msg.hdr.request != VHOST_USER_GET_CONFIG) {
> - error_report("Received unexpected msg type. Expected %d received %d",
> - VHOST_USER_GET_CONFIG, msg.hdr.request);
> - return -1;
> + error_setg(errp,
> + "Received unexpected msg type. Expected %d received %d",
> + VHOST_USER_GET_CONFIG, msg.hdr.request);
> + return -EINVAL;
> }
>
> if (msg.hdr.size != VHOST_USER_CONFIG_HDR_SIZE + config_len) {
> - error_report("Received bad msg size.");
> - return -1;
> + error_setg(errp, "Received bad msg size.");
> + return -EINVAL;
> }
>
> memcpy(config, msg.payload.config.region, config_len);
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 71897c1a01..6b6f974a83 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -451,7 +451,7 @@ static int vhost_vdpa_set_config(struct vhost_dev *dev, const uint8_t *data,
> }
>
> static int vhost_vdpa_get_config(struct vhost_dev *dev, uint8_t *config,
> - uint32_t config_len)
> + uint32_t config_len, Error **errp)
> {
> struct vhost_vdpa_config *v_config;
> unsigned long config_size = offsetof(struct vhost_vdpa_config, buf);
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index c7f9d8bb06..1b66f1de70 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1562,15 +1562,23 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
> }
>
> int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
> - uint32_t config_len)
> + uint32_t config_len, Error **errp)
> {
> + ERRP_GUARD();
> + int ret;
> +
> assert(hdev->vhost_ops);
>
> if (hdev->vhost_ops->vhost_get_config) {
> - return hdev->vhost_ops->vhost_get_config(hdev, config, config_len);
> + ret = hdev->vhost_ops->vhost_get_config(hdev, config, config_len, errp);
> + if (ret < 0 && !*errp) {
> + error_setg_errno(errp, -ret, "vhost_get_config failed");
> + }
> + return ret;
> }
>
> - return -1;
> + error_setg(errp, "vhost_dev_get_config not implemented");
> + return -ENOTSUP;
> }
>
> int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *data,
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 6/7] vhost-user-blk: Factor out vhost_user_blk_realize_connect()
2021-06-09 15:46 [PATCH 0/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
` (4 preceding siblings ...)
2021-06-09 15:46 ` [PATCH 5/7] vhost: Distinguish errors in vhost_dev_get_config() Kevin Wolf
@ 2021-06-09 15:46 ` Kevin Wolf
2021-06-10 9:18 ` Stefano Garzarella
2021-06-11 20:11 ` Raphael Norwitz
2021-06-09 15:46 ` [PATCH 7/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
2021-06-30 12:39 ` [PATCH 0/7] " Kevin Wolf
7 siblings, 2 replies; 23+ messages in thread
From: Kevin Wolf @ 2021-06-09 15:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, raphael.norwitz, qemu-devel, mst
This function is the part that we will want to retry if the connection
is lost during initialisation, so factor it out to keep the following
patch simpler.
The error path for vhost_dev_get_config() forgot disconnecting the
chardev, add this while touching the code.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
hw/block/vhost-user-blk.c | 48 ++++++++++++++++++++++++++-------------
1 file changed, 32 insertions(+), 16 deletions(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index 3770f715da..e49d2e4c83 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -423,6 +423,36 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
}
}
+static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp)
+{
+ DeviceState *dev = &s->parent_obj.parent_obj;
+ int ret;
+
+ s->connected = false;
+
+ ret = qemu_chr_fe_wait_connected(&s->chardev, errp);
+ if (ret < 0) {
+ return ret;
+ }
+
+ ret = vhost_user_blk_connect(dev, errp);
+ if (ret < 0) {
+ qemu_chr_fe_disconnect(&s->chardev);
+ return ret;
+ }
+ assert(s->connected);
+
+ ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
+ sizeof(struct virtio_blk_config), errp);
+ if (ret < 0) {
+ qemu_chr_fe_disconnect(&s->chardev);
+ vhost_dev_cleanup(&s->dev);
+ return ret;
+ }
+
+ return 0;
+}
+
static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
{
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -467,22 +497,10 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
s->inflight = g_new0(struct vhost_inflight, 1);
s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
- s->connected = false;
-
- if (qemu_chr_fe_wait_connected(&s->chardev, errp) < 0) {
- goto virtio_err;
- }
- if (vhost_user_blk_connect(dev, errp) < 0) {
- qemu_chr_fe_disconnect(&s->chardev);
- goto virtio_err;
- }
- assert(s->connected);
-
- ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
- sizeof(struct virtio_blk_config), errp);
+ ret = vhost_user_blk_realize_connect(s, errp);
if (ret < 0) {
- goto vhost_err;
+ goto virtio_err;
}
/* we're fully initialized, now we can operate, so add the handler */
@@ -491,8 +509,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
NULL, true);
return;
-vhost_err:
- vhost_dev_cleanup(&s->dev);
virtio_err:
g_free(s->vhost_vqs);
s->vhost_vqs = NULL;
--
2.30.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 6/7] vhost-user-blk: Factor out vhost_user_blk_realize_connect()
2021-06-09 15:46 ` [PATCH 6/7] vhost-user-blk: Factor out vhost_user_blk_realize_connect() Kevin Wolf
@ 2021-06-10 9:18 ` Stefano Garzarella
2021-06-11 20:11 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Stefano Garzarella @ 2021-06-10 9:18 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:57PM +0200, Kevin Wolf wrote:
>This function is the part that we will want to retry if the connection
>is lost during initialisation, so factor it out to keep the following
>patch simpler.
>
>The error path for vhost_dev_get_config() forgot disconnecting the
>chardev, add this while touching the code.
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> hw/block/vhost-user-blk.c | 48 ++++++++++++++++++++++++++-------------
> 1 file changed, 32 insertions(+), 16 deletions(-)
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 6/7] vhost-user-blk: Factor out vhost_user_blk_realize_connect()
2021-06-09 15:46 ` [PATCH 6/7] vhost-user-blk: Factor out vhost_user_blk_realize_connect() Kevin Wolf
2021-06-10 9:18 ` Stefano Garzarella
@ 2021-06-11 20:11 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Raphael Norwitz @ 2021-06-11 20:11 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Raphael Norwitz, qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:57PM +0200, Kevin Wolf wrote:
> This function is the part that we will want to retry if the connection
> is lost during initialisation, so factor it out to keep the following
> patch simpler.
>
> The error path for vhost_dev_get_config() forgot disconnecting the
> chardev, add this while touching the code.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
> hw/block/vhost-user-blk.c | 48 ++++++++++++++++++++++++++-------------
> 1 file changed, 32 insertions(+), 16 deletions(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index 3770f715da..e49d2e4c83 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -423,6 +423,36 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
> }
> }
>
> +static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp)
> +{
> + DeviceState *dev = &s->parent_obj.parent_obj;
> + int ret;
> +
> + s->connected = false;
> +
> + ret = qemu_chr_fe_wait_connected(&s->chardev, errp);
> + if (ret < 0) {
> + return ret;
> + }
> +
> + ret = vhost_user_blk_connect(dev, errp);
> + if (ret < 0) {
> + qemu_chr_fe_disconnect(&s->chardev);
> + return ret;
> + }
> + assert(s->connected);
> +
> + ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
> + sizeof(struct virtio_blk_config), errp);
> + if (ret < 0) {
> + qemu_chr_fe_disconnect(&s->chardev);
> + vhost_dev_cleanup(&s->dev);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> {
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -467,22 +497,10 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>
> s->inflight = g_new0(struct vhost_inflight, 1);
> s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
> - s->connected = false;
> -
> - if (qemu_chr_fe_wait_connected(&s->chardev, errp) < 0) {
> - goto virtio_err;
> - }
>
> - if (vhost_user_blk_connect(dev, errp) < 0) {
> - qemu_chr_fe_disconnect(&s->chardev);
> - goto virtio_err;
> - }
> - assert(s->connected);
> -
> - ret = vhost_dev_get_config(&s->dev, (uint8_t *)&s->blkcfg,
> - sizeof(struct virtio_blk_config), errp);
> + ret = vhost_user_blk_realize_connect(s, errp);
> if (ret < 0) {
> - goto vhost_err;
> + goto virtio_err;
> }
>
> /* we're fully initialized, now we can operate, so add the handler */
> @@ -491,8 +509,6 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> NULL, true);
> return;
>
> -vhost_err:
> - vhost_dev_cleanup(&s->dev);
> virtio_err:
> g_free(s->vhost_vqs);
> s->vhost_vqs = NULL;
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 7/7] vhost-user-blk: Implement reconnection during realize
2021-06-09 15:46 [PATCH 0/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
` (5 preceding siblings ...)
2021-06-09 15:46 ` [PATCH 6/7] vhost-user-blk: Factor out vhost_user_blk_realize_connect() Kevin Wolf
@ 2021-06-09 15:46 ` Kevin Wolf
2021-06-10 9:23 ` Stefano Garzarella
2021-06-11 20:15 ` Raphael Norwitz
2021-06-30 12:39 ` [PATCH 0/7] " Kevin Wolf
7 siblings, 2 replies; 23+ messages in thread
From: Kevin Wolf @ 2021-06-09 15:46 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, raphael.norwitz, qemu-devel, mst
Commit dabefdd6 removed code that was supposed to try reconnecting
during .realize(), but actually just crashed and had several design
problems.
This adds the feature back without the crash in simple cases while also
fixing some design problems: Reconnection is now only tried if there was
a problem with the connection and not an error related to the content
(which would fail again the same way in the next attempt). Reconnection
is limited to three attempts (four with the initial attempt) so that we
won't end up in an infinite loop if a problem is permanent. If the
backend restarts three times in the very short time window of device
initialisation, we have bigger problems and erroring out is the right
course of action.
In the case that a connection error occurs and we reconnect, the error
message is printed using error_report_err(), but otherwise ignored.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
hw/block/vhost-user-blk.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
index e49d2e4c83..f75a42bc62 100644
--- a/hw/block/vhost-user-blk.c
+++ b/hw/block/vhost-user-blk.c
@@ -455,8 +455,10 @@ static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp)
static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
{
+ ERRP_GUARD();
VirtIODevice *vdev = VIRTIO_DEVICE(dev);
VHostUserBlk *s = VHOST_USER_BLK(vdev);
+ int retries;
int i, ret;
if (!s->chardev.chr) {
@@ -498,7 +500,17 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
s->inflight = g_new0(struct vhost_inflight, 1);
s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
- ret = vhost_user_blk_realize_connect(s, errp);
+ retries = 3;
+ assert(!*errp);
+ do {
+ if (*errp) {
+ error_prepend(errp, "Reconnecting after error: ");
+ error_report_err(*errp);
+ *errp = NULL;
+ }
+ ret = vhost_user_blk_realize_connect(s, errp);
+ } while (ret == -EPROTO && retries--);
+
if (ret < 0) {
goto virtio_err;
}
--
2.30.2
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH 7/7] vhost-user-blk: Implement reconnection during realize
2021-06-09 15:46 ` [PATCH 7/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
@ 2021-06-10 9:23 ` Stefano Garzarella
2021-06-11 20:15 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Stefano Garzarella @ 2021-06-10 9:23 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:58PM +0200, Kevin Wolf wrote:
>Commit dabefdd6 removed code that was supposed to try reconnecting
>during .realize(), but actually just crashed and had several design
>problems.
>
>This adds the feature back without the crash in simple cases while also
>fixing some design problems: Reconnection is now only tried if there was
>a problem with the connection and not an error related to the content
>(which would fail again the same way in the next attempt). Reconnection
>is limited to three attempts (four with the initial attempt) so that we
>won't end up in an infinite loop if a problem is permanent. If the
>backend restarts three times in the very short time window of device
>initialisation, we have bigger problems and erroring out is the right
>course of action.
>
>In the case that a connection error occurs and we reconnect, the error
>message is printed using error_report_err(), but otherwise ignored.
>
>Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>---
> hw/block/vhost-user-blk.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
>diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
>index e49d2e4c83..f75a42bc62 100644
>--- a/hw/block/vhost-user-blk.c
>+++ b/hw/block/vhost-user-blk.c
>@@ -455,8 +455,10 @@ static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp)
>
> static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> {
>+ ERRP_GUARD();
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
>+ int retries;
> int i, ret;
>
> if (!s->chardev.chr) {
>@@ -498,7 +500,17 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> s->inflight = g_new0(struct vhost_inflight, 1);
> s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
>
>- ret = vhost_user_blk_realize_connect(s, errp);
>+ retries = 3;
Maybe we can add a macro for this with a comment with the information
provided in this commit message.
Thanks,
Stefano
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH 7/7] vhost-user-blk: Implement reconnection during realize
2021-06-09 15:46 ` [PATCH 7/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
2021-06-10 9:23 ` Stefano Garzarella
@ 2021-06-11 20:15 ` Raphael Norwitz
1 sibling, 0 replies; 23+ messages in thread
From: Raphael Norwitz @ 2021-06-11 20:15 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Raphael Norwitz, qemu-devel, qemu-block, mst
On Wed, Jun 09, 2021 at 05:46:58PM +0200, Kevin Wolf wrote:
> Commit dabefdd6 removed code that was supposed to try reconnecting
> during .realize(), but actually just crashed and had several design
> problems.
>
> This adds the feature back without the crash in simple cases while also
> fixing some design problems: Reconnection is now only tried if there was
> a problem with the connection and not an error related to the content
> (which would fail again the same way in the next attempt). Reconnection
> is limited to three attempts (four with the initial attempt) so that we
> won't end up in an infinite loop if a problem is permanent. If the
> backend restarts three times in the very short time window of device
> initialisation, we have bigger problems and erroring out is the right
> course of action.
>
> In the case that a connection error occurs and we reconnect, the error
> message is printed using error_report_err(), but otherwise ignored.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
> hw/block/vhost-user-blk.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index e49d2e4c83..f75a42bc62 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -455,8 +455,10 @@ static int vhost_user_blk_realize_connect(VHostUserBlk *s, Error **errp)
>
> static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> {
> + ERRP_GUARD();
> VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> VHostUserBlk *s = VHOST_USER_BLK(vdev);
> + int retries;
> int i, ret;
>
> if (!s->chardev.chr) {
> @@ -498,7 +500,17 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
> s->inflight = g_new0(struct vhost_inflight, 1);
> s->vhost_vqs = g_new0(struct vhost_virtqueue, s->num_queues);
>
> - ret = vhost_user_blk_realize_connect(s, errp);
> + retries = 3;
> + assert(!*errp);
> + do {
> + if (*errp) {
> + error_prepend(errp, "Reconnecting after error: ");
> + error_report_err(*errp);
> + *errp = NULL;
> + }
> + ret = vhost_user_blk_realize_connect(s, errp);
> + } while (ret == -EPROTO && retries--);
> +
> if (ret < 0) {
> goto virtio_err;
> }
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 0/7] vhost-user-blk: Implement reconnection during realize
2021-06-09 15:46 [PATCH 0/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
` (6 preceding siblings ...)
2021-06-09 15:46 ` [PATCH 7/7] vhost-user-blk: Implement reconnection during realize Kevin Wolf
@ 2021-06-30 12:39 ` Kevin Wolf
7 siblings, 0 replies; 23+ messages in thread
From: Kevin Wolf @ 2021-06-30 12:39 UTC (permalink / raw)
To: qemu-block; +Cc: mst, qemu-devel, raphael.norwitz
Am 09.06.2021 um 17:46 hat Kevin Wolf geschrieben:
> My previous series removed the broken implementation of automatic
> reconnection during .realize(). This series adds some error reporting
> improvements that allow distinguishing cases where reconnecting could
> help from permanent errors, and then uses it to re-implement the
> automatic reconnection during .realize(), as was requested during review
> of the previous series.
>
> Kevin Wolf (7):
> vhost: Add Error parameter to vhost_dev_init()
> vhost: Distinguish errors in vhost_backend_init()
> vhost: Return 0/-errno in vhost_dev_init()
> vhost-user-blk: Add Error parameter to vhost_user_blk_start()
> vhost: Distinguish errors in vhost_dev_get_config()
> vhost-user-blk: Factor out vhost_user_blk_realize_connect()
> vhost-user-blk: Implement reconnection during realize
Thanks for the review, fixed up the series according to the minor
comments you had and applied it to my block branch.
Kevin
^ permalink raw reply [flat|nested] 23+ messages in thread