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