[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 01/12] qemu-img: introduce qemu_img_handle_er
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 01/12] qemu-img: introduce qemu_img_handle_error() |
Date: |
Wed, 24 Apr 2013 21:00:03 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130402 Thunderbird/17.0.5 |
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 <address@hidden>
>>
> 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 <address@hidden>
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v2 04/12] qapi: Convert delvm, Pavel Hrdina, 2013/04/24