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 <[email protected]>
>> ---
>> 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
