Re: [Qemu-devel] [PATCH v2 01/12] qemu-img: introduce qemu_img_handle_error()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

On 04/24/2013 08:53 PM, Wenchao Xia wrote:

>>>
>>> +static int qemu_img_handle_error(Error *err)
>>> +{
>>> +    if (error_is_set(&err)) {
>>> +        error_report("%s", error_get_pretty(err));
>>> +        error_free(err);
>>> +        return 1;
>>> +    }
>>> +    return 0;
>>
>> Maybe it's just me, but I think returning EXIT_SUCCESS/EXIT_FAILURE
>> instead of 0/1 is a bit nicer at expressing why we chose a positive
>> value; but that would be a separate cleanup to all of qemu-img.c.
>> Hence, I have no problems giving:
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>   Maybe an incode comments like:
> +/* Returns 1 on error. */

That would also help.  My main concern was that +1 on error is unusual
compared to most of qemu that returns <0 on error.  The _reason_ it is a
positive number is because we are really returning EXIT_FAILURE (a
well-defined constant from system headers) - but calling the number by
its name is smarter than just using a magic number without explanation.
 But as I already said, that's a bigger problem for a different series.

> 
> Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]