Re: [PATCH 2/5] vhost-user-blk: Use Error more consistently

[Raphael Norwitz <raphael.norwitz@nutanix.com>]

Code looks ok - question about the commit message.

Acked-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

On Thu, Apr 22, 2021 at 07:02:18PM +0200, Kevin Wolf wrote:
> Now that vhost_user_blk_connect() is not called from an event handler
> any more, but directly from vhost_user_blk_device_realize(), we don't
> have to resort to error_report() any more, but can use Error again.

vhost_user_blk_connect() is still called by vhost_user_blk_event() which
is registered as an event handler. I don't understand your point around
us having had to use error_report() before, but not anymore. Can you
clarify?

> 
> With Error, the callers are responsible for adding context if necessary
> (such as the "-device" option the error refers to). Additional prefixes
> are redundant and better omitted.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/block/vhost-user-blk.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c
> index e824b0a759..8422a29470 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -311,7 +311,7 @@ static void vhost_user_blk_reset(VirtIODevice *vdev)
>      vhost_dev_free_inflight(s->inflight);
>  }
>  
> -static int vhost_user_blk_connect(DeviceState *dev)
> +static int vhost_user_blk_connect(DeviceState *dev, Error **errp)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VHostUserBlk *s = VHOST_USER_BLK(vdev);
> @@ -331,8 +331,7 @@ static int vhost_user_blk_connect(DeviceState *dev)
>  
>      ret = vhost_dev_init(&s->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0);
>      if (ret < 0) {
> -        error_report("vhost-user-blk: vhost initialization failed: %s",
> -                     strerror(-ret));
> +        error_setg_errno(errp, -ret, "vhost initialization failed");
>          return ret;
>      }
>  
> @@ -340,8 +339,7 @@ static int vhost_user_blk_connect(DeviceState *dev)
>      if (virtio_device_started(vdev, vdev->status)) {
>          ret = vhost_user_blk_start(vdev);
>          if (ret < 0) {
> -            error_report("vhost-user-blk: vhost start failed: %s",
> -                         strerror(-ret));
> +            error_setg_errno(errp, -ret, "vhost start failed");
>              return ret;
>          }
>      }
> @@ -380,10 +378,12 @@ static void vhost_user_blk_event(void *opaque, QEMUChrEvent event)
>      DeviceState *dev = opaque;
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VHostUserBlk *s = VHOST_USER_BLK(vdev);
> +    Error *local_err = NULL;
>  
>      switch (event) {
>      case CHR_EVENT_OPENED:
> -        if (vhost_user_blk_connect(dev) < 0) {
> +        if (vhost_user_blk_connect(dev, &local_err) < 0) {
> +            error_report_err(local_err);
>              qemu_chr_fe_disconnect(&s->chardev);
>              return;
>          }
> @@ -427,7 +427,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>      int i, ret;
>  
>      if (!s->chardev.chr) {
> -        error_setg(errp, "vhost-user-blk: chardev is mandatory");
> +        error_setg(errp, "chardev is mandatory");
>          return;
>      }
>  
> @@ -435,16 +435,16 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>          s->num_queues = 1;
>      }
>      if (!s->num_queues || s->num_queues > VIRTIO_QUEUE_MAX) {
> -        error_setg(errp, "vhost-user-blk: invalid number of IO queues");
> +        error_setg(errp, "invalid number of IO queues");
>          return;
>      }
>  
>      if (!s->queue_size) {
> -        error_setg(errp, "vhost-user-blk: queue size must be non-zero");
> +        error_setg(errp, "queue size must be non-zero");
>          return;
>      }
>      if (s->queue_size > VIRTQUEUE_MAX_SIZE) {
> -        error_setg(errp, "vhost-user-blk: queue size must not exceed %d",
> +        error_setg(errp, "queue size must not exceed %d",
>                     VIRTQUEUE_MAX_SIZE);
>          return;
>      }
> @@ -471,7 +471,7 @@ static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp)
>          goto virtio_err;
>      }
>  
> -    if (vhost_user_blk_connect(dev) < 0) {
> +    if (vhost_user_blk_connect(dev, errp) < 0) {
>          qemu_chr_fe_disconnect(&s->chardev);
>          goto virtio_err;
>      }
> -- 
> 2.30.2
>