[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] command: avoid fd leak on failure



At 07/13/2011 10:43 AM, Daniel Veillard Write:
> On Tue, Jul 12, 2011 at 02:09:34PM -0600, Eric Blake wrote:
>> virCommandTransferFD promises that the fd is no longer owned by
>> the caller.  Normally, we want the fd to remain open until the
>> child runs, but in error situations, we must close it earlier.
>>
>> * src/util/command.c (virCommandTransferFD): Close fd now if we
>> can't track it to close later.
>> ---
>>  src/util/command.c |   10 ++++++++--
>>  1 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/util/command.c b/src/util/command.c
>> index 3c516ec..83d4e88 100644
>> --- a/src/util/command.c
>> +++ b/src/util/command.c
>> @@ -738,7 +738,7 @@ virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer)
>>  void
>>  virCommandPreserveFD(virCommandPtr cmd, int fd)
>>  {
>> -    return virCommandKeepFD(cmd, fd, false);
>> +    virCommandKeepFD(cmd, fd, false);
>>  }
> 
>   I must admit I'm surprized, the compiler really doesn't warn if
> one does "return f()" if the caller is a void function. I.e. void

If f() and caller are void functions, gcc does not warn 'return f()'.
If f() is not a void function, and caller is a void function, gcc will
warn 'return f()'

I guess gcc check the caller's return type and f()'s return type. If
they are the same type, gcc does not warn.

> is actually considered a value ??? I guess my brain still follows
> Pascal where procedure and functions were not the same...
> 
>>  /*
>> @@ -749,7 +749,13 @@ virCommandPreserveFD(virCommandPtr cmd, int fd)
>>  void
>>  virCommandTransferFD(virCommandPtr cmd, int fd)
>>  {
>> -    return virCommandKeepFD(cmd, fd, true);
>> +    virCommandKeepFD(cmd, fd, true);
>> +    if ((!cmd || cmd->has_error) && fd > STDERR_FILENO) {
>> +        /* We must close the fd now, instead of waiting for
>> +         * virCommandRun, if there was an error that prevented us from
>> +         * adding the fd to cmd.  */
>> +        VIR_FORCE_CLOSE(fd);
>> +    }
>>  }
> 
>   Hum, it's a bit like memory allocation, it's better to have the one
> who allocates it to free it to avoid double frees. Maybe we could
> instead raise and error in the caller chain, and have it freed at teh
> right place (unless it really get messy).
> 
>    opinion ?
> 
> Daniel
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]