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@xxxxxxxxxx>
> >>>
> >>> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list