From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XhIv7-0004Bp-ST for qemu-devel@nongnu.org; Thu, 23 Oct 2014 09:59:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XhIv1-0000ZN-La for qemu-devel@nongnu.org; Thu, 23 Oct 2014 09:59:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21029) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XhIv1-0000ZI-Dz for qemu-devel@nongnu.org; Thu, 23 Oct 2014 09:59:11 -0400 Message-ID: <544909A6.2020204@redhat.com> Date: Thu, 23 Oct 2014 15:59:02 +0200 From: Max Reitz MIME-Version: 1.0 References: <1414070952-25865-1-git-send-email-mreitz@redhat.com> <544907F9.8010302@redhat.com> In-Reply-To: <544907F9.8010302@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qemu-img: Print error if check failed List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Peter Lieven , =?UTF-8?B?QmVub8OudCBDYW5ldA==?= , Stefan Hajnoczi 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 >> --- >> 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)'. >