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

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

[Max Reitz]

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.

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) {
+        if (ret) {
+            error_report("Check failed: %s", strerror(-ret));
+        } else {
+            error_report("Check failed");
+        }
         ret = 1;
         goto fail;
     }
-- 
1.9.3

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

[Eric Blake]

[-- 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 --]

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

[Max Reitz]

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

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

[Eric Blake]

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

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 <eblake@redhat.com>

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

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

[Markus Armbruster]

Max Reitz <mreitz@redhat.com> writes:

> 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.
>
> 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) {
> +        if (ret) {
> +            error_report("Check failed: %s", strerror(-ret));
> +        } else {
> +            error_report("Check failed");
> +        }
>          ret = 1;
>          goto fail;
>      }

Non-zero ret can only come from bdrv_check() via collect_image_check().
bdrv_check() is specified to return zero or -errno.  Okay.

Aside: ret is used for error codes, 0/-1 and for the return value
0/1/2/3/63.  Unclean, but not this patch's fault.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Max Reitz]

On 2014-10-23 at 15:29, 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.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   qemu-img.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)

This feels greedy, but Kevin encouraged me to on IRC, so:

Applied to my block tree:
https://github.com/XanClic/qemu/commits/block

Max