[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/3] qemu-img: fix img_commit() error return val
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 1/3] qemu-img: fix img_commit() error return value |
Date: |
Wed, 27 Aug 2014 08:31:59 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) |
John Snow <address@hidden> 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 <address@hidden>
>>> ---
>>> 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."
>
> address@hidden ~/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.
- [Qemu-devel] [PATCH 0/3] qemu-img: error path fixes, Stefan Hajnoczi, 2014/08/26
- [Qemu-devel] [PATCH 2/3] qemu-img: fix img_compare() flags error path, Stefan Hajnoczi, 2014/08/26
- [Qemu-devel] [PATCH 3/3] qemu-img: always goto out in img_snapshot() error paths, Stefan Hajnoczi, 2014/08/26
- Re: [Qemu-devel] [PATCH 0/3] qemu-img: error path fixes, John Snow, 2014/08/26
- Re: [Qemu-devel] [PATCH 0/3] qemu-img: error path fixes, Stefan Hajnoczi, 2014/08/27