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

[Max Reitz <mreitz@redhat.com>]

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.

>> 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?

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.

Max

>> +        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)'.
>