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

[Markus Armbruster <armbru@redhat.com>]

John Snow <jsnow@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 <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.

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.