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 <address@hidden>
> > ---
> >   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)'.