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

[Eric Blake]

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.

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