Re: [libvirt] [PATCH 08/10] Introduce new APIs for spawning processes

On 11/19/2010 05:16 PM, Eric Blake wrote: > On 11/18/2010 02:55 AM, Daniel P. Berrange wrote: >> On Wed, Nov 17, 2010 at 09:29:00PM -0700, Eric Blake wrote: >>> From: Daniel P. Berrange <berrange(a)redhat.com&gt; >>> >>> This introduces a new set of APIs in src/util/command.h >>> to use for invoking commands. This is intended to replace >>> all current usage of virRun and virExec variants, with a >>> more flexible and less error prone API. >>> >> >> >> My code forgot to ever close() the fds in cmd->preserve. We definitely >> need todo it in virCommandFree(), but there's a small argument to say >> we should also do it in virCommandRun/virCommandRunAsync so that if >> the caller keeps the virCommandPtr alive for a long time, we don't >> have the open FDs. > > I'll look into this more. On thinking about this more, I'm thinking that the user should be able to request whether the fd remains open after the command has been executed (for example, the client may want to share an fd among multiple child processes, in which case each virCommandRun must not close the fd); doable by adding another argument to virCommandPreserveFD. > >> >> It would also be useful to have a generic API for logging info about >> the command to an FD (to let us remove that logging code from UML >> and QEMU & LXC drivers). >> >> eg >> >> +void virCommandWriteArgLog(virCommandPtr cmd, int logfd) Okay, I see how you added that in your variant in today's locking series, along with virCommandToString; now folded into my v2.

@@ -117,20 +121,25 @@ virCommandNewArgs(const char *const*args) /* * Preserve the specified file descriptor in the child, instead of * 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, otherwise caller is still responsible for fd. */ void -virCommandPreserveFD(virCommandPtr cmd, int fd) +virCommandPreserveFD(virCommandPtr cmd, int fd, bool transfer)

@@ -868,6 +961,13 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) VIR_DEBUG("Command result %d, with PID %d", ret, (int)cmd->pid); + for (i = STDERR_FILENO + 1; i < FD_SETSIZE; i++) { + if (FD_ISSET(i, &cmd->transfer)) { + int tmpfd = i; + VIR_FORCE_CLOSE(tmpfd); + } + } + if (ret == 0 && pid) *pid = cmd->pid;