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

[Matthias Bolte]

2010/11/24 Eric Blake <eblake at redhat.com>:
> 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.
>
> * src/util/command.c: New file.
> * src/util/command.h: New header.
> * src/Makefile.am (UTIL_SOURCES): Build it.
> * src/libvirt_private.syms: Export symbols internally.
> * tests/commandtest.c: New test.
> * tests/Makefile.am (check_PROGRAMS): Run it.
> * tests/commandhelper.c: Auxiliary program.
> * tests/commanddata/test2.log - test15.log: New expected outputs.
> * cfg.mk (useless_free_options): Add virCommandFree.
> * po/POTFILES.in: New translation.
> * .x-sc_avoid_write: Add exemption.
> * tests/.gitignore: Ignore new built file.
> ---
>
> v2: add virCommandTransferFD, virCommandToString, virCommandAddArgFormat,
> virCommandNewArgList, virCommandWriteArgLog, virCommandNonblockingFDs.
> Fix virCommandRunAsync and virCommandFree to free transfered FDs.
> Add a bit more test exposure.
>

> diff --git a/cfg.mk b/cfg.mk
> index dea8301..6312632 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -77,6 +77,7 @@ useless_free_options =                                \
>   --name=virCapabilitiesFreeHostNUMACell       \
>   --name=virCapabilitiesFreeMachines           \
>   --name=virCgroupFree                         \
> +  --name=virCommandFree                                \
>   --name=virConfFreeList                       \
>   --name=virConfFreeValue                      \
>   --name=virDomainChrDefFree                   \

You should also add virCommandError to the msg_gen_function list.

> diff --git a/src/util/command.c b/src/util/command.c
> new file mode 100644
> index 0000000..948a957
> --- /dev/null
> +++ b/src/util/command.c

> +/*
> + * Create a new command with a NULL terminated
> + * set of args, taking binary from argv[0]
> + */

s/argv[0]/args[0]/

> +virCommandPtr
> +virCommandNewArgs(const char *const*args)



> +
> +/*
> + * Capture the child's stdout to a string buffer
> + */
> +void
> +virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf)
> +{
> +    if (!cmd || cmd->has_error)
> +        return;
> +
> +    if (cmd->outfd != -1) {

To detect double assignment properly you need to check for this I think:

    if (cmd->outfd != -1 || cmd->outfdptr || cmd->outbuf) {

> +        cmd->has_error = -1;
> +        VIR_DEBUG0("cannot specify output twice");
> +        return;
> +    }
> +
> +    cmd->outbuf = outbuf;
> +    cmd->outfdptr = &cmd->outfd;
> +}
> +
> +
> +/*
> + * Capture the child's stderr to a string buffer
> + */
> +void
> +virCommandSetErrorBuffer(virCommandPtr cmd, char **errbuf)
> +{
> +    if (!cmd || cmd->has_error)
> +        return;
> +
> +    if (cmd->errfd != -1) {

The same pattern as in virCommandSetOutputBuffer:

    if (cmd->errfd != -1 || cmd->errfdptr || cmd->errbuf) {

> +        cmd->has_error = -1;
> +        VIR_DEBUG0("cannot specify stderr twice");
> +        return;
> +    }
> +
> +    cmd->errbuf = errbuf;
> +    cmd->errfdptr = &cmd->errfd;
> +}
> +
> +
> +/*
> + * Attach a file descriptor to the child's stdin
> + */
> +void
> +virCommandSetInputFD(virCommandPtr cmd, int infd)
> +{
> +    if (!cmd || cmd->has_error)
> +        return;
> +
> +    if (infd < 0 || cmd->inbuf) {

I think you meant to check here for this instead:

    if (cmd->infd != -1 || cmd->inbuf) {

> +        cmd->has_error = -1;
> +        VIR_DEBUG0("cannot specify input twice");
> +        return;
> +    }
> +
> +    cmd->infd = infd;
> +}
> +
> +
> +/*
> + * Attach a file descriptor to the child's stdout
> + */
> +void
> +virCommandSetOutputFD(virCommandPtr cmd, int *outfd)
> +{
> +    if (!cmd || cmd->has_error)
> +        return;
> +
> +    if (!outfd || cmd->outfd != -1) {

I think you meant to check here for this instead:

    if (cmd->outfd != -1 || cmd->outfdptr || cmd->outbuf) {

> +        cmd->has_error = -1;
> +        VIR_DEBUG0("cannot specify output twice");
> +        return;
> +    }
> +
> +    cmd->outfdptr = outfd;
> +}
> +
> +
> +/*
> + * Attach a file descriptor to the child's stderr
> + */
> +void
> +virCommandSetErrorFD(virCommandPtr cmd, int *errfd)
> +{
> +    if (!cmd || cmd->has_error)
> +        return;
> +
> +    if (!errfd || cmd->errfd != -1) {

I think you meant to check here for this instead:

    if (cmd->errfd != -1 || cmd->errfdptr || cmd->errbuf) {

> +        cmd->has_error = -1;
> +        VIR_DEBUG0("cannot specify stderr twice");
> +        return;
> +    }
> +
> +    cmd->errfdptr = errfd;
> +}

> +
> +/*
> + * Call after adding all arguments and environment settings, but before
> + * Run/RunAsync, to immediately output the environment and arguments of
> + * cmd to logfd.  If virCommandRun cannot succeed (because of an
> + * out-of-memory condition while building cmd), nothing will be logged.
> + */
> +void
> +virCommandWriteArgLog(virCommandPtr cmd, int logfd)
> +{
> +    int ioError = 0;
> +    size_t i;
> +
> +    /* Any errors will be reported later by virCommandRun, which means
> +     * no command will be run, so there is nothing to log. */
> +    if (!cmd || cmd->has_error)
> +        return;
> +
> +    for (i = 0 ; i < cmd->nenv ; i++) {
> +        if (safewrite(logfd, cmd->env[i], strlen(cmd->env[i])) < 0)
> +            ioError = errno;
> +        if (safewrite(logfd, " ", 1) < 0)
> +            ioError = errno;
> +    }
> +    for (i = 0 ; i < cmd->nargs ; i++) {
> +        if (safewrite(logfd, cmd->args[i], strlen(cmd->args[i])) < 0)
> +            ioError = errno;
> +        if (safewrite(logfd, i == cmd->nargs - 1 ? "\n" : " ", 1) < 0)
> +            ioError = errno;
> +    }

As written this will only save the last occurred error in ioError. Is
this intended? Looks like a best effort approach, try to write as much
as possible ignoring errors.

> +    if (ioError) {
> +        char ebuf[1024];
> +        VIR_WARN("Unable to write command %s args to logfile: %s",
> +                 cmd->args[0], virStrerror(ioError, ebuf, sizeof ebuf));
> +    }
> +}

> +/*
> + * 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.

So, either I don't get the logic here or the two if blocks that setup
stdout and stderr capturing for logging should be moved in front of
the call to virCommandRunAsync in order to give virExecWithHook a
chance to setup the FDs properly.

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

Matthias