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

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.

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(-)

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
>

Am 28.04.2021 um 20:08 hat Raphael Norwitz geschrieben:
> 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?

What I meant is that vhost_user_blk_event() can't really make use of an
Error object other than passing it to error_report_err(), which has the
same result as directly using error_report().

With the new code where vhost_user_blk_device_realize() calls the
function directly, we can actually return the error to its caller so
that it ends up in the QMP result or the command line error message.

The result is still not great because vhost_user_blk_connect() doesn't
know the original error message. We'd have to add Error to
vhost_dev_init() and the functions that it calls to get the real error
messages, but at least it's a first step in the right direction.

We already figured that we need to change error reporting so we can know
whether we should retry, so I guess this can be solved at the same time.

Kevin

Makes sense - I see why it makes reporting better at realize time.

Thanks for the clarification.

On Thu, Apr 29, 2021 at 11:26:29AM +0200, Kevin Wolf wrote:
> Am 28.04.2021 um 20:08 hat Raphael Norwitz geschrieben:
> > 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?
> 
> What I meant is that vhost_user_blk_event() can't really make use of an
> Error object other than passing it to error_report_err(), which has the
> same result as directly using error_report().
> 
> With the new code where vhost_user_blk_device_realize() calls the
> function directly, we can actually return the error to its caller so
> that it ends up in the QMP result or the command line error message.
> 
> The result is still not great because vhost_user_blk_connect() doesn't
> know the original error message. We'd have to add Error to
> vhost_dev_init() and the functions that it calls to get the real error
> messages, but at least it's a first step in the right direction.
> 
> We already figured that we need to change error reporting so we can know
> whether we should retry, so I guess this can be solved at the same time.
> 
> Kevin
>