Eric Blake <[email protected]> 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.
>>
>> Signed-off-by: Markus Armbruster <[email protected]>
>> ---
>> qga/commands-posix.c | 6 +++---
>> qga/main.c | 1 +
>> 2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
>> index f6af7d1..6af974f 100644
>> --- a/qga/commands-posix.c
>> +++ b/qga/commands-posix.c
>> @@ -223,8 +223,8 @@ static int64_t guest_file_handle_add(FILE *fh, Error
>> **errp)
>> int64_t handle;
>>
>> 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.
>
>> +++ b/qga/main.c
>> @@ -910,6 +910,7 @@ int64_t ga_get_fd_handle(GAState *s, Error **errp)
>>
>> if (!write_persistent_state(&s->pstate, s->pstate_filepath)) {
>> error_setg(errp, "failed to commit persistent state to disk");
>> + return -1;
>> }
>
> Same here.
Likewise.
>
>>
>> return handle;
>>
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.
