On 04/25/2014 12:06 PM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
