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

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(-)

On 26.08.2014 20:17, 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(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>

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.

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.

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.