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

[Daniel P. Berrange]

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 at 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