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

[Matthias Bolte]

2010/12/2 Daniel P. Berrange <berrange at redhat.com>:
> 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.
>

Yes, that's true, but that's not the case I'm talking about.

The case I'm talking about is when the user of the command API didn't
set the cmd->outfdptr nor the cmd->outbuf, so both are NULL. In that
case when the chain virCommandRunAsync -> virExecWithHook -> __virExec
is called __virExec will bind stdout of the child to /dev/null instead
of a pipe.

So when changing cmd->outfdptr and cmd->outbuf to non-NULL values
after the call to virCommandRunAsync this will not result in having a
valid FD assigned to cmd->outfd before the call to
virCommandProcessIO. Therefore, virCommandProcessIO won't capture
stdout and stderr for logging purpose.

I still think that the two if blocks should be moved in front of
virCommandRunAsync to achieve the described behavior, in the case that
the command API user didn't request to capture stdout/stderr.

Matthias