Re: [RFC PATCH-for-5.2 v2 2/2] block/block-backend: Let blk_attach_dev() provide helpful error

[Daniel P. Berrangé <berrange@xxxxxxxxxx>]

On Thu, Jul 16, 2020 at 02:37:04PM +0200, Philippe Mathieu-Daudé wrote:
> Let blk_attach_dev() take an Error* object to return helpful
> information. Adapt the callers.
> 
>   $ qemu-system-arm -M n800
>   qemu-system-arm: sd_init failed: cannot attach blk 'sd0' to device 'sd-card'
>   blk 'sd0' is already attached by device 'omap2-mmc'
>   Drive 'sd0' is already in use because it has been automatically connected 
> to another device
>   (do you need 'if=none' in the drive options?)
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@xxxxxxxxx>
> ---
> v2: Rebased after 668f62ec62 ("error: Eliminate error_propagate()")
> ---
>  include/sysemu/block-backend.h   |  2 +-
>  block/block-backend.c            | 11 ++++++++++-
>  hw/block/fdc.c                   |  4 +---
>  hw/block/swim.c                  |  4 +---
>  hw/block/xen-block.c             |  5 +++--
>  hw/core/qdev-properties-system.c | 16 +++++++++-------
>  hw/ide/qdev.c                    |  4 +---
>  hw/scsi/scsi-disk.c              |  4 +---
>  8 files changed, 27 insertions(+), 23 deletions(-)
> 
> diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
> index 8203d7f6f9..118fbad0b4 100644
> --- a/include/sysemu/block-backend.h
> +++ b/include/sysemu/block-backend.h
> @@ -113,7 +113,7 @@ BlockDeviceIoStatus blk_iostatus(const BlockBackend *blk);
>  void blk_iostatus_disable(BlockBackend *blk);
>  void blk_iostatus_reset(BlockBackend *blk);
>  void blk_iostatus_set_err(BlockBackend *blk, int error);
> -int blk_attach_dev(BlockBackend *blk, DeviceState *dev);
> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp);
>  void blk_detach_dev(BlockBackend *blk, DeviceState *dev);
>  DeviceState *blk_get_attached_dev(BlockBackend *blk);
>  char *blk_get_attached_dev_id(BlockBackend *blk);
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 63ff940ef9..b7be0a4619 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -884,12 +884,21 @@ void blk_get_perm(BlockBackend *blk, uint64_t *perm, 
> uint64_t *shared_perm)
>  
>  /*
>   * Attach device model @dev to @blk.
> + *
> + * @blk: Block backend
> + * @dev: Device to attach the block backend to
> + * @errp: pointer to NULL initialized error object
> + *
>   * Return 0 on success, -EBUSY when a device model is attached already.
>   */
> -int blk_attach_dev(BlockBackend *blk, DeviceState *dev)
> +int blk_attach_dev(BlockBackend *blk, DeviceState *dev, Error **errp)
>  {
>      trace_blk_attach_dev(blk_name(blk), object_get_typename(OBJECT(dev)));
>      if (blk->dev) {
> +        error_setg(errp, "cannot attach blk '%s' to device '%s'",
> +                   blk_name(blk), object_get_typename(OBJECT(dev)));
> +        error_append_hint(errp, "blk '%s' is already attached by device 
> '%s'\n",
> +                          blk_name(blk), 
> object_get_typename(OBJECT(blk->dev)));

I would have a preference for expanding the main error message and not
using a hint.  Any hint is completely thrown away when using QMP :-(


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|