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

[Max Reitz]

On 2015-03-20 at 06:19, 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>
> ---
>   block.c                   | 21 +++++++++++----------
>   block/qcow.c              |  4 ++--
>   block/qcow2.c             |  2 +-
>   block/qed.c               |  2 +-
>   block/vdi.c               |  2 +-
>   block/vhdx.c              |  2 +-
>   block/vmdk.c              |  4 ++--
>   block/vpc.c               |  2 +-
>   block/vvfat.c             |  3 ++-
>   blockdev.c                |  4 ++--
>   include/qapi/qmp/qerror.h |  8 ++++----
>   11 files changed, 28 insertions(+), 26 deletions(-)
>
> diff --git a/block.c b/block.c
> index 98b90b4..8247f0a 100644
> --- a/block.c
> +++ b/block.c
> @@ -1224,8 +1224,8 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
> BlockDriverState *backing_hd)
>           bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker);
>       } else if (backing_hd) {
>           error_setg(&bs->backing_blocker,
> -                   "device is used as backing hd of '%s'",
> -                   bdrv_get_device_name(bs));
> +                   "node is used as backing hd of '%s'",
> +                   bdrv_get_device_or_node_name(bs));

Actually, the change of "device" to "node" here is independent of the 
use of bdrv_get_device_or_node_name(). But it's correct anyway.

>       }
>   
>       bs->backing_hd = backing_hd;
> @@ -1812,8 +1812,8 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
> BlockReopenQueue *queue,
>        * to r/w */
>       if (!(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR) &&
>           reopen_state->flags & BDRV_O_RDWR) {
> -        error_set(errp, QERR_DEVICE_IS_READ_ONLY,
> -                  bdrv_get_device_name(reopen_state->bs));
> +        error_set(errp, QERR_NODE_IS_READ_ONLY,
> +                  bdrv_get_device_or_node_name(reopen_state->bs));

I think Markus would be happier with just removing the macro.

(Make it error_setg(errp, "Node '%s' is read only", 
bdrv_get_device_or_node_name(reopen_state->bs)) and remove the 
QERR_DEVICE_IS_READ_ONLY macro from qerror.h)

[snip]

> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 57a62d4..46cad14 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -38,7 +38,7 @@ void qerror_report_err(Error *err);
>       ERROR_CLASS_GENERIC_ERROR, "Base '%s' not found"
>   
>   #define QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED \
> -    ERROR_CLASS_GENERIC_ERROR, "Block format '%s' used by device '%s' does not 
> support feature '%s'"
> +    ERROR_CLASS_GENERIC_ERROR, "Block format '%s' used by node '%s' does not 
> support feature '%s'"

Preexisting, but first: This line has over 80 characters.

Second: This seems like a good opportunity to drop this macro and 
replace its occurrences by error_setg(errp, "Block format...") and the like.

>    #define QERR_BLOCK_JOB_NOT_READY \
>       ERROR_CLASS_GENERIC_ERROR, "The active block job for device '%s' cannot be 
> completed"
> @@ -58,9 +58,6 @@ void qerror_report_err(Error *err);
>   #define QERR_DEVICE_IN_USE \
>       ERROR_CLASS_GENERIC_ERROR, "Device '%s' is in use"
>   
> -#define QERR_DEVICE_IS_READ_ONLY \
> -    ERROR_CLASS_GENERIC_ERROR, "Device '%s' is read only"
> -
>   #define QERR_DEVICE_NO_HOTPLUG \
>       ERROR_CLASS_GENERIC_ERROR, "Device '%s' does not support hotplugging"
>   
> @@ -103,6 +100,9 @@ void qerror_report_err(Error *err);
>   #define QERR_MISSING_PARAMETER \
>       ERROR_CLASS_GENERIC_ERROR, "Parameter '%s' is missing"
>   
> +#define QERR_NODE_IS_READ_ONLY \
> +    ERROR_CLASS_GENERIC_ERROR, "Node '%s' is read only"
> +
>   #define QERR_PERMISSION_DENIED \
>       ERROR_CLASS_GENERIC_ERROR, "Insufficient permission to perform this 
> operation"

As said above, the same goes for this macro.

I'm not so ardent about this, so if you want to keep the macros, it's 
fine with me; but the overly long line need to be fixed (only the one 
you're touching, not necessarily the rest, because I guess they'll be 
dropped in the future anyway).

Markus?

Max