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 <ebl...@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 <xiaw...@linux.vnet.ibm.com> > -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
