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

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

 



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
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list


[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]