Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 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));
     }
 
     ret = qcow2_cache_flush(bs, s->l2_table_cache);
-- 
2.11.1

On 06/26/2018 08:50 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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 <vsementsov@virtuozzo.com>
>> ---
>>   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