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

[Eric Blake]

On 10/23/2014 07:59 AM, Max Reitz wrote:
> On 2014-10-23 at 15:51, Eric Blake wrote:
>> 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?
> 
> It would involve some blkdebug things which try to break the qcow2 check
> function. I wouldn't rely on it, because this rather exercises the qcow2
> check function than this patch.

Fair enough.

>>> +        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?
> 
> Yes, I think we can. I interpreted that as "Test completed, but there
> were errors". The dump should not be incorrect, because if it was, the
> check function should not have returned 0.
> 
> Therefore, I think we should dump the test result because by returning 0
> the check function says it's valid. If there were check_errors, the dump
> function will show their number, too.

Furthermore, if 'quiet' is true, then the dump_* call probably output
nothing, but the fact that we want to return non-zero to flag that check
detected problems is worth being noisy about.  If I understand
correctly, 'quiet' is only about suppressing stdout, not stderr, and
always being noisy on stderr for non-zero exit is a good idea.

Okay, I think your answers convinced me, and I'm comfortable enough with
the patch as-is to give:
Reviewed-by: Eric Blake <address@hidden>

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

signature.asc
Description: OpenPGP digital signature