[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 2/3] block: use bdrv_get_device_or_node_name() i
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH 2/3] block: use bdrv_get_device_or_node_name() in error messages |
Date: |
Wed, 08 Apr 2015 10:27:34 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 |
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 <address@hidden>
> Reviewed-by: Max Reitz <address@hidden>
> ---
> +++ 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 <address@hidden>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature