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

[Markus Armbruster <armbru@redhat.com>]

Max Reitz <mreitz@redhat.com> writes:

> 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 <berto@igalia.com>
>> ---
>>   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?

I've been chipping away at qerror.h for a while.  I think I can empty it
out in the next development cycle.  So every new QERR_ macro you add
I'll have to replace again.  Using literal strings instead will save me
the trouble.

If you find yourself using the same error string in many places, chances
are that some of these places do the same thing.  Consider factoring
out.

Other than that, I wouldn't worry about duplicating error strings.  Drop
in the bucket.