Re: [Qemu-devel] [PATCH 1/3] qemu-img: fix img_commit() error return value

[John Snow <jsnow@redhat.com>]

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

"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.

-- 
—js