Re: [Qemu-block] [Qemu-devel] [PATCH 2/6] block/qcow2: improve error message in qcow2_inactivate

[John Snow]

On 06/28/2018 08:16 AM, Eric Blake wrote:
> On 06/26/2018 08:50 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   block/qcow2.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 945132f692..46194a33ca 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -2114,9 +2114,9 @@ static int qcow2_inactivate(BlockDriverState *bs)
>>       qcow2_store_persistent_dirty_bitmaps(bs, &local_err);
>>       if (local_err != NULL) {
>>           result = -EINVAL;
>> -        error_report_err(local_err);
>> -        error_report("Persistent bitmaps are lost for node '%s'",
>> -                     bdrv_get_device_or_node_name(bs));
>> +        error_reportf_err(local_err, "Persistent bitmaps are lost for
>> node "
>> +                          "'%s', because failed to store them on qcow2 "
>> +                          "inactivation: ",
>> bdrv_get_device_or_node_name(bs));
> 
> That's longer, and has awkward grammar.
> 
> Also, for a patch designed to improve an error message, it's nice if the
> commit message demonstrates a before-and-after comparison of the two
> different wordings, along with a formula for reproducing the error (not
> mandatory, but it can sure help in reviewing).
> 
> Shorter might be:
> 
> "Lost persistent bitmaps during inactivation of node '%s': "
> 
> since the local_err text appended after the colon will then make it
> obvious what the error was during inactivation.
> 

I like Eric's phrasing suggestion.

If you haven't actually managed to trigger this error in real life, I
won't demand you artificially do so for the sake of demonstration.

--js