On Mon, Nov 22, 2010 at 03:30:35PM -0700, Eric Blake wrote:
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@redhat.com>
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.
Ah yes, I forgot to mention that - it came in useful when converting the QEMU driver to the new APIs.
@@ -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)
How about having two separate methods ? virCommandPreserveFD virCommandTransferFD Means you don't have to remember whether true means transfer or don't 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;
I think we also need duplicate this in virCommandFree(), because there may be scenarios where we get 1/2 way through populating a virCommandPtr instance, and then some other error means we have to stop & free it, without ever getting to virCommandRunAsync. Daniel