On Wed, Oct 12, 2011 at 05:59:40PM -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. > (virCommandKeepFD): Adjust helper to make this easier. > --- > > This leak can only happen on OOM or other extreme error conditions, > but ought to be plugged. When I originally posted this: > https://www.redhat.com/archives/libvir-list/2011-July/msg00674.html > DV was worried that callers might abuse things and use fd > even after this function closed it; but I proved to myself in > writing a (non-working) v2 that all callers were already safe, > and that this v1 was indeed a smaller solution. Okidoc then :-) > src/util/command.c | 17 +++++++++++------ > 1 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/src/util/command.c b/src/util/command.c > index 699ba2d..c3ce361 100644 > --- a/src/util/command.c > +++ b/src/util/command.c > @@ -730,23 +730,26 @@ virCommandNewArgList(const char *binary, ...) > * closing it. FD must not be one of the three standard streams. If > * transfer is true, then fd will be closed in the parent after a call > * to Run/RunAsync/Free, otherwise caller is still responsible for fd. > + * Returns true if a transferring caller should close FD now, and > + * false if the transfer is successfully recorded. > */ > -static void > +static bool > virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer) > { > if (!cmd) > - return; > + return fd > STDERR_FILENO; > > if (fd <= STDERR_FILENO || FD_SETSIZE <= fd) { > if (!cmd->has_error) > cmd->has_error = -1; > VIR_DEBUG("cannot preserve %d", fd); > - return; > + return fd > STDERR_FILENO; > } > > FD_SET(fd, &cmd->preserve); > if (transfer) > FD_SET(fd, &cmd->transfer); > + return false; > } > > /** > @@ -761,7 +764,7 @@ virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer) > void > virCommandPreserveFD(virCommandPtr cmd, int fd) > { > - return virCommandKeepFD(cmd, fd, false); > + virCommandKeepFD(cmd, fd, false); > } > > /** > @@ -771,12 +774,14 @@ virCommandPreserveFD(virCommandPtr cmd, int fd) > * > * Transfer the specified file descriptor > * to the child, instead of closing it on exec. > - * Close the fd in the parent during Run/RunAsync/Free. > + * The parent should no longer use fd, and the parent's copy will > + * be automatically closed no later than during Run/RunAsync/Free. > */ > void > virCommandTransferFD(virCommandPtr cmd, int fd) > { > - return virCommandKeepFD(cmd, fd, true); > + if (virCommandKeepFD(cmd, fd, true)) > + VIR_FORCE_CLOSE(fd); > } ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/ -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list