[libvirt] [PATCHv2 3/8] Introduce new APIs for spawning processes

[Daniel P. Berrange]

On Wed, Dec 01, 2010 at 10:42:40PM +0100, Matthias Bolte wrote:
> 2010/11/24 Eric Blake <eblake at redhat.com>:
> > +/*
> > + * Manage input and output to the child process.
> > + */
> > +static int
> > +virCommandProcessIO(virCommandPtr cmd)
> > +{
> > +    int infd = -1, outfd = -1, errfd = -1;
> > +    size_t inlen = 0, outlen = 0, errlen = 0;
> > +    size_t inoff = 0;
> > +
> > +    /* With an input buffer, feed data to child
> > +     * via pipe */
> > +    if (cmd->inbuf) {
> > +        inlen = strlen(cmd->inbuf);
> > +        infd = cmd->inpipe;
> > +    }
> > +
> > +    /* With out/err buffer, the outfd/errfd
> > +     * have been filled with an FD for us */
> > +    if (cmd->outbuf)
> > +        outfd = cmd->outfd;
> > +    if (cmd->errbuf)
> > +        errfd = cmd->errfd;
> > +
> 
> > +/*
> > + * Run the command and wait for completion.
> > + * Returns -1 on any error executing the
> > + * command. Returns 0 if the command executed,
> > + * with the exit status set
> > + */
> > +int
> > +virCommandRun(virCommandPtr cmd, int *exitstatus)
> > +{
> > +    int ret = 0;
> > +    char *outbuf = NULL;
> > +    char *errbuf = NULL;
> > +    int infd[2];
> > +
> > +    if (!cmd || cmd->has_error == -1) {
> > +        virCommandError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                        _("invalid use of command API"));
> > +        return -1;
> > +    }
> > +    if (cmd->has_error == ENOMEM) {
> > +        virReportOOMError();
> > +        return -1;
> > +    }
> > +
> > +    /* If we have an input buffer, we need
> > +     * a pipe to feed the data to the child */
> > +    if (cmd->inbuf) {
> > +        if (pipe(infd) < 0) {
> > +            virReportSystemError(errno, "%s",
> > +                                 _("unable to open pipe"));
> > +            return -1;
> > +        }
> > +        cmd->infd = infd[0];
> > +        cmd->inpipe = infd[1];
> > +    }
> > +
> > +    if (virCommandRunAsync(cmd, NULL) < 0) {
> > +        if (cmd->inbuf) {
> > +            int tmpfd = infd[0];
> > +            if (VIR_CLOSE(infd[0]) < 0)
> > +                VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
> > +            tmpfd = infd[1];
> > +            if (VIR_CLOSE(infd[1]) < 0)
> > +                VIR_DEBUG("ignoring failed close on fd %d", tmpfd);
> > +        }
> > +        return -1;
> > +    }
> > +
> > +    /* If caller hasn't requested capture of
> > +     * stdout/err, then capture it ourselves
> > +     * so we can log it
> > +     */
> > +    if (!cmd->outbuf &&
> > +        !cmd->outfdptr) {
> > +        cmd->outfd = -1;
> > +        cmd->outfdptr = &cmd->outfd;
> > +        cmd->outbuf = &outbuf;
> > +    }
> > +    if (!cmd->errbuf &&
> > +        !cmd->errfdptr) {
> > +        cmd->errfd = -1;
> > +        cmd->errfdptr = &cmd->errfd;
> > +        cmd->errbuf = &errbuf;
> > +    }
> > +
> > +    if (cmd->inbuf || cmd->outbuf || cmd->errbuf)
> > +        ret = virCommandProcessIO(cmd);
> 
> In virCommandProcessIO you say that cmd->{out|err}fd is a valid FD
> (created in virExecWithHook called by virCommandRunAsync) when
> cmd->{out|err}buf is not NULL. As far as I can tell from the code that
> is true when the caller had requested to capture stdout and stderr.
> But in case that caller didn't do this then you setup buffers to
> capture stdout and stderr for logging. In that case
> virCommandProcessIO will be called with cmd->{out|err}buf being
> non-NULL but cmd->{out|err}fd being invalid as you explicitly set them
> to -1 in the two if blocks before the call to virCommandProcessIO.

Those two blocks setting errfd/outfd to -1 are not executed
when outbuf/errbuf are non-NULL, so errfd/outfd are still
pointing to the pipe() FDs when virCommandProcesIO is run.

> > diff --git a/tests/commandtest.c b/tests/commandtest.c
> > new file mode 100644
> > index 0000000..46d6ae5
> > --- /dev/null
> > +++ b/tests/commandtest.c
> 
> > +
> > +#ifndef __linux__
> 
> What's Linux specific in this test? Shouldn't the command API and this
> test be working on all systems that support fork/exec?

It should have been #ifndef WIN32 actually.

Daniel