Re: [PATCH 2/7] vhost: Distinguish errors in vhost_backend_init()

[Raphael Norwitz]

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
>