From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46238) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMWmA-0004L1-V7 for qemu-devel@nongnu.org; Wed, 27 Aug 2014 02:32:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XMWm4-0000EU-PG for qemu-devel@nongnu.org; Wed, 27 Aug 2014 02:32:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:30178) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMWm4-0000EM-Hi for qemu-devel@nongnu.org; Wed, 27 Aug 2014 02:32:04 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s7R6W3oK004547 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 27 Aug 2014 02:32:03 -0400 From: Markus Armbruster References: <1409077076-29855-1-git-send-email-stefanha@redhat.com> <1409077076-29855-2-git-send-email-stefanha@redhat.com> <53FCE07E.8020308@redhat.com> <53FCEB6A.9000401@redhat.com> Date: Wed, 27 Aug 2014 08:31:59 +0200 In-Reply-To: <53FCEB6A.9000401@redhat.com> (John Snow's message of "Tue, 26 Aug 2014 16:17:46 -0400") Message-ID: <87vbpefnog.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 1/3] qemu-img: fix img_commit() error return value List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: John Snow Cc: Kevin Wolf , Jeff Cody , qemu-devel@nongnu.org, Stefan Hajnoczi John Snow writes: > On 08/26/2014 03:31 PM, Eric Blake wrote: >> On 08/26/2014 12:17 PM, Stefan Hajnoczi wrote: >>> The img_commit() return value is a process exit code. Use 1 for failure >>> instead of -1. The other failure paths in this function already use 1. >>> >>> Signed-off-by: Stefan Hajnoczi >>> --- >>> qemu-img.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/qemu-img.c b/qemu-img.c >>> index c843420..dc3adb5 100644 >>> --- a/qemu-img.c >>> +++ b/qemu-img.c >>> @@ -771,7 +771,7 @@ static int img_commit(int argc, char **argv) >>> ret = bdrv_parse_cache_flags(cache, &flags); >>> if (ret < 0) { >>> error_report("Invalid cache option: %s", cache); >>> - return -1; >>> + return 1; >> >> Nothing against this patch (you're consistent with the surrounding code, >> and most of qemu for that matter), but it highlights why I'm a fan of >> 'return EXIT_FAILURE' instead of 'return 1' in functions that return an >> exit status, because that makes it a lot more obvious _why_ I'm >> returning a non-negative number to represent failure. For me, EXIT_FAILURE carries a smell. In main(), the fact that we're returning an exit code is trivial. Elsewhere, if all you want to return is "succeeded / failed" information encoded as integer, stick to the usual 0/-1 convention, and leave mapping that to exit codes to callers. If you need to return one of several error codes, EXIT_FAILURE is of no use. > "Hey, good point." > > jsnow@local ~/s/qemu> git grep 'return 1;' | wc -l > 842 > > "oh." > > This patch is probably fine -- is there some larger scheme we want to > cook up within the style guide for advocating things like return code > and error reporting standards? > > It seems to me like the preferred style for errors and returns has > changed several times and there's not a good concrete rule (written) > at the moment. It might be worth touching the CODING_GUIDE and/or > HACKING files with recommendations, if we can agree on some consistent > set of rules, like getting rid of error_set(...) and not using > positive, un-named integers to represent errors. Yes, we could use some written guidance on returning errors. I haven't found the time to write. When I can spare a bit of time for errors, I tend to invest it in killing obsolete error reporting interfaces, so I don't have to write guidance on them.