Re: [Qemu-devel] [PATCH 2/3] block: use bdrv_get_device_or_node_name() in error messages

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2980 bytes --]

On 04/08/2015 03:29 AM, Alberto Garcia wrote:
> There are several error messages that identify a BlockDriverState by
> its device name. However those errors can be produced in nodes that
> don't have a device name associated.
> 
> In those cases we should use bdrv_get_device_or_node_name() to fall
> back to the node name and produce a more meaningful message. The
> messages are also updated to use the more generic term 'node' instead
> of 'device'.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---

> +++ b/block/snapshot.c
> @@ -246,9 +246,9 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
>      if (bs->file) {
>          return bdrv_snapshot_delete(bs->file, snapshot_id, name, errp);
>      }
> -    error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
> -              drv->format_name, bdrv_get_device_name(bs),
> -              "internal snapshot deletion");
> +    error_setg(errp, "Block format '%s' used by device '%s' "
> +               "does not support internal snapshot deletion",
> +               drv->format_name, bdrv_get_device_name(bs));
>      return -ENOTSUP;
>  }
>  
> @@ -329,9 +329,9 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>      if (drv->bdrv_snapshot_load_tmp) {
>          return drv->bdrv_snapshot_load_tmp(bs, snapshot_id, name, errp);
>      }
> -    error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
> -              drv->format_name, bdrv_get_device_name(bs),
> -              "temporarily load internal snapshot");
> +    error_setg(errp, "Block format '%s' used by device '%s' "
> +               "does not support temporarily loading internal snapshots",
> +               drv->format_name, bdrv_get_device_name(bs));

Should these two messages use 'node' instead of 'device'?  After all, a
format is tied to a node (as a backing chain can involve multiple nodes
using different formats)


> +++ b/blockdev.c
> @@ -1248,13 +1248,14 @@ static void internal_snapshot_prepare(BlkTransactionState *common,
>      }
>  
>      if (bdrv_is_read_only(bs)) {
> -        error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
> +        error_setg(errp, "Device '%s' is read only", device);
>          return;
>      }

This one is probably fine as device;

>  
>      if (!bdrv_can_snapshot(bs)) {
> -        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
> -                  bs->drv->format_name, device, "internal snapshot");
> +        error_setg(errp, "Block format '%s' used by device '%s' "
> +                   "does not support internal snapshots",
> +                   bs->drv->format_name, device);

but this is probably another one where node may be better.

But it's already a strict improvement, so I can live with:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]