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 <be...@igalia.com> > Reviewed-by: Max Reitz <mre...@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 <ebl...@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
