[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 2/7] vhost: Distinguish errors in vhost_backend_init()
From: |
Raphael Norwitz |
Subject: |
Re: [PATCH 2/7] vhost: Distinguish errors in vhost_backend_init() |
Date: |
Fri, 11 Jun 2021 19:43:45 +0000 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
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
>
- [PATCH 0/7] vhost-user-blk: Implement reconnection during realize, Kevin Wolf, 2021/06/09
- [PATCH 1/7] vhost: Add Error parameter to vhost_dev_init(), Kevin Wolf, 2021/06/09
- [PATCH 2/7] vhost: Distinguish errors in vhost_backend_init(), Kevin Wolf, 2021/06/09
- [PATCH 3/7] vhost: Return 0/-errno in vhost_dev_init(), Kevin Wolf, 2021/06/09
- [PATCH 5/7] vhost: Distinguish errors in vhost_dev_get_config(), Kevin Wolf, 2021/06/09
- [PATCH 4/7] vhost-user-blk: Add Error parameter to vhost_user_blk_start(), Kevin Wolf, 2021/06/09
- [PATCH 6/7] vhost-user-blk: Factor out vhost_user_blk_realize_connect(), Kevin Wolf, 2021/06/09