John Snow <js...@redhat.com> 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 <stefa...@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.
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.
