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

[Daniel Veillard]

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
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

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/