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

[Markus Armbruster]

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.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>  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.