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

[Eric Blake]

On 07/12/2011 08:43 PM, Daniel Veillard wrote:
> 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.
>> ---

>> @@ -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).

That was the whole reason we introduced virCommandPreserveFD vs.
virCommandTransferFD.  With preserve, the caller both opens and closes
fd.  With transfer, the caller opens fd, then transfers it to virCommand
and must not touch it again; virCommand then guarantees that it will be
closed after the child runs.  The problem was that we were not closing
the fd in the few cases where either the child could not be run (due to
a previous error, or because the fd was out of range of fdset).

But I'm open to the idea of changing the signature:

virCommandPreserveFD(virCommandPtr, int) - remains as-is

virCommandTransferFD(virCommandPtr, int *) - change to passing the
address of an fd, so that fd in the caller is set to -1 as part of the
transfer process

I'll post a v2 along those lines, so you can compare the difference (and
also so that you'll see that existing callers are already abandoning fd
after calling transfer, so setting it to -1 is only a safety valve).

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110713/5342191c/attachment-0001.sig>