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

2010/11/24 Eric Blake <eblake(a)redhat.com&gt;: > +/* > + * 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.

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