Re: [Qemu-devel] [PATCH 06/15] qga: Use return values instead of error_is_set(errp)

[Eric Blake]

On 04/25/2014 12:06 PM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
> 
>> On 04/25/2014 09:05 AM, Markus Armbruster wrote:
>>> Using error_is_set(errp) to check whether a function call failed is
>>> fragile: it breaks when errp is null.  I'm not aware of actual
>>> breakage, but checking return values instead when convenient is more
>>> robust and more obviously correct.
>>>

>>>      handle = ga_get_fd_handle(ga_state, errp);
>>> -    if (error_is_set(errp)) {
>>> -        return 0;
>>> +    if (handle < 0) {
>>> +        return -1;
>>
>> Is this a bug fix that should be pushed separately, or at least called
>> out in the commit message as intentional?
> 
> The return value is only used when no error has been set.  So, it's at
> worst a latent bug.
> 

> 
> What about adding the following to the commit message:
> 
>     qga: Use return values instead of error_is_set(errp)
> 
>     Using error_is_set(errp) to check whether a function call failed is
>     fragile: it breaks when errp is null.  ga_get_fd_handle() and
>     guest_file_handle_add() don't return a useful value when they fail,
>     but that's just stupid.  Fix that, and check them instead.  As far
>     as I can tell, errp can't be null there, but this is more robust and
>     more obviously correct.

Works for me.  I didn't spot anything else odd, so:

Series: Reviewed-by: Eric Blake <address@hidden>

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

signature.asc
Description: OpenPGP digital signature