Re: [Qemu-devel] [PATCH] qemu-img: Print error if check failed

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2248 bytes --]

On 10/23/2014 07:29 AM, Max Reitz wrote:
> Currently, if bdrv_check() fails either by returning -errno or having
> check_errors set, qemu-img check just exits with 1 after having told the
> user that there were no errors on the image. This is bad.
> 
> Instead of printing the check result if there were internal errors which
> were so bad that bdrv_check() could not even complete with 0 as a return
> value, qemu-img check should inform the user about the error.
> 

Is there a way to exercise this in the testsuite?

> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  qemu-img.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 09e7e72..731502c 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -687,16 +687,23 @@ static int img_check(int argc, char **argv)
>          check->corruptions_fixed    = corruptions_fixed;
>      }
>  
> -    switch (output_format) {
> -    case OFORMAT_HUMAN:
> -        dump_human_image_check(check, quiet);
> -        break;
> -    case OFORMAT_JSON:
> -        dump_json_image_check(check, quiet);
> -        break;
> +    if (!ret) {
> +        switch (output_format) {
> +        case OFORMAT_HUMAN:
> +            dump_human_image_check(check, quiet);
> +            break;
> +        case OFORMAT_JSON:
> +            dump_json_image_check(check, quiet);
> +            break;
> +        }
>      }
>  
>      if (ret || check->check_errors) {

Can we ever have ret == 0 (so we attempted dump_*_image_check) AND
check->check_errors?  Will that be confusing output, to have both a
(probably incorrect) dump on stdout and an error message on stderr?

> +        if (ret) {
> +            error_report("Check failed: %s", strerror(-ret));
> +        } else {
> +            error_report("Check failed");
> +        }
>          ret = 1;
>          goto fail;
>      }

Or rephrasing the question, would it be better to hoist this chunk to
occur before the switch (output_format)?  And if so, then you don't need
to reindent that code inside 'if (!ret)'.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]