On 05/25/2010 07:24 AM, Daniel P. Berrange wrote:
> 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/Makefile.am | 1 +
> src/util/command.c | 782 ++++++++++++++++++++++++++++++++++++++++++
> src/util/command.h | 197 +++++++++++
> tests/Makefile.am | 14 +-
> tests/commanddata/test1.log | 12 +
> tests/commanddata/test10.log | 14 +
> tests/commanddata/test11.log | 12 +
> tests/commanddata/test12.log | 12 +
> tests/commanddata/test13.log | 12 +
> tests/commanddata/test2.log | 14 +
> tests/commanddata/test3.log | 12 +
> tests/commanddata/test4.log | 10 +
> tests/commanddata/test5.log | 6 +
> tests/commanddata/test6.log | 11 +
> tests/commanddata/test7.log | 7 +
> tests/commanddata/test8.log | 14 +
> tests/commanddata/test9.log | 14 +
> tests/commandhelper.c | 112 ++++++
> tests/commandtest.c | 494 ++++++++++++++++++++++++++
Nice to see as much test additions as the code itself.
> +
> +struct _virCommand {
> + int has_error;
s/int/bool/?
> +
> + int nargs;
s/int/size_t/
> + char **args;
> +
> + int nenv;
s/int/size_t/
> + char **env;
> +
> + fd_set preserve;
This implicitly limits us to FD_SETSIZE fd's (aka 32 on some platforms);
is that acceptable, or should we be using a better structure?
> + unsigned int flags;
> +
> + const char *inbuf;
> + char **outbuf;
> + char **errbuf;
> +
> + int infd;
> + int inpipe;
s/int/bool/?
> + int outfd;
> + int errfd;
> + int *outfdptr;
> + int *errfdptr;
> +
> + virExecHook hook;
> + void *opaque;
> +
> + pid_t pid;
> + char *pidfile;
Do we want to allow the parent process to control which directory the
child will start in (defaulting to the parent's cwd)?
> +};
> +
> +/*
> + * Create a new command for named binary
> + */
> +virCommandPtr virCommandNew(const char *binary)
> +{
> + const char *const args[] = { binary, NULL };
> +
> + return virCommandNewArgs(args);
> +}
> +
> +/*
> + * Create a new command with a NULL terminated
> + * set of args, taking binary from argv[0]
> + */
> +virCommandPtr virCommandNewArgs(const char *const*args)
> +{
> + virCommandPtr cmd;
> +
> + if (VIR_ALLOC(cmd) < 0)
> + return NULL;
> +
> + virCommandAddArgSet(cmd, args);
> +
> + if (cmd->has_error) {
> + virCommandFree(cmd);
Ouch - virCommandFree will call close(cmd->outfd), since at this point,
it is still 0 instead of -1, nuking the parent's stdin. You need to
float the cmd->has_error checking down.
> + return NULL;
> + }
> +
> + FD_ZERO(&cmd->preserve);
> + cmd->infd = cmd->outfd = cmd->errfd = -1;
> + cmd->inpipe = -1;
> + cmd->pid = -1;
> +
> + return cmd;
> +}
> +
> +
> +/*
> + * Preserve the specified file descriptor
> + * in the child, instead of closing it
> + */
> +void virCommandPreserveFD(virCommandPtr cmd,
> + int fd)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + FD_SET(fd, &cmd->preserve);
No check for 0 <= fd < FD_SETSIZE?
> +}
> +
> +/*
> + * Save the child PID in a pidfile
> + */
> +void virCommandSetPidFile(virCommandPtr cmd,
> + const char *pidfile)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + VIR_FREE(cmd->pidfile);
> + if (!(cmd->pidfile = strdup(pidfile)))
> + cmd->has_error = 1;
s/1/true/, if you changed has_error to bool above.
> +}
> +
> +/*
> + * Remove all capabilities from the child
> + */
> +void virCommandClearCaps(virCommandPtr cmd)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + cmd->flags |= VIR_EXEC_CLEAR_CAPS;
> +}
> +
> +
> +/*
> + * Re-allow a specific capability
> + */
> +void virCommandAllowCap(virCommandPtr cmd,
> + int capability ATTRIBUTE_UNUSED)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + /* XXX ? */
> +}
Do we have a use case for this yet, or should we just #if 0 this section
until there is a need and a reasonable implementation?
> +
> +
> +/*
> + * Daemonize the child process
> + */
> +void virCommandDaemonize(virCommandPtr cmd)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + cmd->flags |= VIR_EXEC_DAEMON;
> +}
> +
> +
> +/*
> + * Add an environment variable to the child
> + * using separate name & value strings
> + */
> +void virCommandAddEnvPair(virCommandPtr cmd,
> + const char *name,
> + const char *value)
> +{
> + char *env;
> +
> + if (!cmd || cmd->has_error)
> + return;
> +
> + if (virAsprintf(&env, "%s=%s", name, value ? value : "") < 0) {
> + cmd->has_error = 1;
> + return;
> + }
> +
> + if (VIR_REALLOC_N(cmd->env, cmd->nenv + 2) < 0) {
This scales quadratically, and can lead to noticeable performance
degredation if there are lots of calls to virCommandAddEnvPair. Better
would be to amortize the allocation costs by tracking the allocation
size of cmd->env (in addition to cmd->nenv), and geometrically enlarging
it as-needed (by a factor of 1.5x or 2x), rather than a reallocation on
every addition.
> + VIR_FREE(env);
> + cmd->has_error = 1;
> + return;
> + }
> +
> + cmd->env[cmd->nenv] = env;
> + cmd->env[cmd->nenv+1] = NULL;
Formatting nit - I would have written cmd->env[cmd->nenv + 1]
> + cmd->nenv++;
> +}
> +
> +
> +/*
> + * Add an environemnt variable to the child
s/environemnt/environment/
> + * using a preformated env string FOO=BAR
s/preformated/preformatted/
> + */
> +void virCommandAddEnvString(virCommandPtr cmd,
> + const char *str)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + char *env;
> +
> + if (!cmd || cmd->has_error)
> + return;
> +
> + if (!(env = strdup(str))) {
> + cmd->has_error = 1;
> + return;
> + }
> +
> + if (VIR_REALLOC_N(cmd->env, cmd->nenv + 2) < 0) {
Same comment as for virCommandAddEnvStringPair; probably worth a helper
function that re-allocates either cmd->env or cmd->args as needed.
> + VIR_FREE(env);
> + cmd->has_error = 1;
> + return;
> + }
> +
> + cmd->env[cmd->nenv] = env;
> + cmd->env[cmd->nenv+1] = NULL;
> + cmd->nenv++;
> +}
> +
> +
> +/*
> + * Pass an environment variable to the child
> + * using current process' value
> + */
> +void virCommandAddEnvPass(virCommandPtr cmd,
> + const char *name)
> +{
> + char *value;
> + if (!cmd || cmd->has_error)
> + return;
> +
> + value = getenv(name);
> + if (value)
> + virCommandAddEnvPair(cmd, name, value);
> +}
> +
> +
> +void virCommandAddEnvPassCommon(virCommandPtr cmd)
Documentation?
> +{
> + virCommandAddEnvPair(cmd, "LC_ALL", "C");
> +
> + virCommandAddEnvPass(cmd, "LD_PRELOAD");
> + virCommandAddEnvPass(cmd, "LD_LIBRARY_PATH");
> + virCommandAddEnvPass(cmd, "PATH");
> + virCommandAddEnvPass(cmd, "HOME");
> + virCommandAddEnvPass(cmd, "USER");
> + virCommandAddEnvPass(cmd, "LOGNAME");
> + virCommandAddEnvPass(cmd, "TMPDIR");
> +}
> +
> +/*
> + * Add a command line argument to the child
> + */
> +void virCommandAddArg(virCommandPtr cmd,
> + const char *val)
> +{
> + char *arg;
> +
> + if (!cmd || cmd->has_error)
> + return;
> +
> + if (!(arg = strdup(val))) {
> + cmd->has_error = 1;
> + return;
> + }
> +
> + if (VIR_REALLOC_N(cmd->args, cmd->nargs + 2) < 0) {
Same comment as for virCommandAddEnvStringPair on quadratic scaling.
> + VIR_FREE(arg);
> + cmd->has_error = 1;
> + return;
> + }
> +
> + cmd->args[cmd->nargs] = arg;
> + cmd->args[cmd->nargs+1] = NULL;
> + cmd->nargs++;
> +}
> +
> +
> +void virCommandAddArgPair(virCommandPtr cmd,
Documentation?
> + const char *name,
> + const char *val)
> +{
> + char *arg;
> +
> + if (!cmd || cmd->has_error)
> + return;
> +
> + if (virAsprintf(&arg, "%s=%s", name, val) < 0) {
> + cmd->has_error = 1;
> + return;
> + }
> +
> + if (VIR_REALLOC_N(cmd->args, cmd->nargs + 2) < 0) {
> + VIR_FREE(arg);
> + cmd->has_error = 1;
> + return;
> + }
> +
> + cmd->args[cmd->nargs] = arg;
> + cmd->args[cmd->nargs+1] = NULL;
> + cmd->nargs++;
> +}
> +
> +/*
> + * Add a NULL terminated list of args
> + */
> +void virCommandAddArgSet(virCommandPtr cmd,
> + const char *const*vals)
Is it worth a varargs variant of this:
virCommandAddArgList(cmd, "a", "b", NULL)?
> +{
> + int narg = 0;
> +
> + if (!cmd || cmd->has_error)
> + return;
> +
> + while (vals[narg] != NULL)
> + narg++;
> +
> + if (VIR_REALLOC_N(cmd->args, cmd->nargs + narg + 1) < 0) {
> + cmd->has_error = 1;
> + return;
> + }
> +
> + narg = 0;
> + while (vals[narg] != NULL) {
> + char *arg = strdup(vals[narg]);
> + if (!arg) {
> + cmd->has_error = 1;
> + return;
> + }
> + cmd->args[cmd->nargs + narg] = arg;
> + narg++;
> + }
> + cmd->args[cmd->nargs + narg] = NULL;
> + cmd->nargs += narg;
> +}
> +
> +
> +/*
> + * Feed the child's stdin from a string buffer
> + */
> +void virCommandSetInputBuffer(virCommandPtr cmd,
> + const char *inbuf)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + cmd->inbuf = inbuf;
> + cmd->infd = -1;
> + cmd->inpipe = -1;
Should we set has_error if the user calls both virCommandSetInputBuffer
and virCommandSetInputFD? (Detectable if infd or inpipe are not -1
here; likewise detectable in virCommandSetInputFD if input is not NULL).
> +}
> +
> +
> +/*
> + * Capture the child's stdout to a string buffer
> + */
> +void virCommandSetOutputBuffer(virCommandPtr cmd,
> + char **outbuf)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + cmd->outbuf = outbuf;
> + cmd->outfdptr = &cmd->outfd;
Likewise, should we detect an error if both virCommandSetOutputBuffer
and virCommandSetOutputFD are called?
> +}
> +
> +
> +/*
> + * Capture the child's stderr to a string buffer
> + */
> +void virCommandSetErrorBuffer(virCommandPtr cmd,
> + char **errbuf)
> +{
> + if (!cmd || cmd->has_error)
> + 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;
Set has_error if infd < 0. Should we also require that infd be a
currently open fd?
> +
> + cmd->inbuf = NULL;
> + cmd->inpipe = -1;
> + cmd->infd = infd;
> +}
> +
> +
> +/*
> + * Attach a file descriptor to the child's stdout
> + */
> +void virCommandSetOutputFD(virCommandPtr cmd,
> + int *outfd)
> +{
> + if (!cmd || cmd->has_error)
> + return;
Same comment as for virCommandSetInputFD
> +
> + cmd->outbuf = NULL;
> + cmd->outfdptr = outfd;
> +}
> +
> +
> +/*
> + * Attach a file descriptor to the child's stderr
> + */
> +void virCommandSetErrorFD(virCommandPtr cmd,
> + int *errfd)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + cmd->errbuf = NULL;
> + cmd->errfdptr = errfd;
> +}
> +
> +
> +void virCommandSetPreExecHook(virCommandPtr cmd,
Documentation?
> + virExecHook hook,
> + void *opaque)
> +{
> + if (!cmd || cmd->has_error)
> + return;
> +
> + cmd->hook = hook;
> + cmd->opaque = opaque;
Didn't see this one documented in the html.
> +}
> +
> +
> +static int
> +virCommandProcessIO(virCommandPtr cmd)
Documentation?
> +{
> + 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, we're the outfd/errfd
s/we're //
> + * have been filled with an FD for us */
> + if (cmd->outbuf)
> + outfd = cmd->outfd;
> + if (cmd->errbuf)
> + errfd = cmd->errfd;
> +
> + for (;;) {
> + int i;
> + struct pollfd fds[3];
> + int nfds = 0;
> +
> + if (infd != -1) {
> + fds[nfds].fd = infd;
> + fds[nfds].events = POLLOUT;
> + nfds++;
> + }
> + if (outfd != -1) {
> + fds[nfds].fd = outfd;
> + fds[nfds].events = POLLIN;
> + nfds++;
> + }
> + if (errfd != -1) {
> + fds[nfds].fd = errfd;
> + fds[nfds].events = POLLIN;
> + nfds++;
> + }
Should this initialization be floated outside of the for(;;) loop?
> +
> + if (nfds == 0)
> + break;
> +
> + if (poll(fds,nfds, -1) < 0) {
> + if ((errno == EAGAIN) || (errno == EINTR))
> + continue;
> + virReportSystemError(errno, "%s",
> + _("unable to poll on child"));
> + return -1;
> + }
> +
> + for (i = 0; i < nfds ; i++) {
> + if (fds[i].fd == errfd ||
> + fds[i].fd == outfd) {
> + char data[1024];
> + char **buf;
> + size_t *len;
> + int done;
> + if (fds[i].fd == outfd) {
> + buf = cmd->outbuf;
> + len = &outlen;
> + } else {
> + buf = cmd->errbuf;
> + len = &errlen;
> + }
> +
> + done = read(fds[i].fd, data, sizeof(data));
> + if (done < 0) {
> + if (errno != EINTR &&
> + errno != EAGAIN) {
> + virReportSystemError(errno, "%s",
> + _("unable to write to child input"));
> + return -1;
> + }
> + } else if (done == 0) {
> + if (fds[i].fd == outfd)
> + outfd = -1;
> + else
> + errfd = -1;
> + } else {
> + if (VIR_REALLOC_N(*buf, *len + done + 1) < 0) {
> + virReportOOMError();
> + return -1;
> + }
> + memmove(*buf + *len, data, done);
s/memmove/memcpy/ - there's no overlap between data and buf
> + *len += done;
> + (*buf)[*len] = '\0';
> + }
> + } else {
> + int done;
> +
> + done = write(infd, cmd->inbuf + inoff,
> + inlen - inoff);
> + if (done < 0) {
> + if (errno != EINTR &&
> + errno != EAGAIN) {
> + virReportSystemError(errno, "%s",
> + _("unable to write to child input"));
> + return -1;
> + }
> + } else {
> + inoff += done;
> + if (inoff == inlen) {
> + close(infd);
> + infd = -1;
> + }
> + }
> + }
> +
> + }
> + }
> +
> + return 0;
> +
> +}
> +
> +
> +/*
> + * 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) {
> + 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) {
> + close(infd[0]);
> + close(infd[1]);
> + }
> + 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);
> +
> + if (virCommandWait(cmd, exitstatus) < 0)
> + ret = -1;
> +
> + VIR_DEBUG("Result stdout: '%s' stderr: '%s'",
> + NULLSTR(*cmd->outbuf),
> + NULLSTR(*cmd->errbuf));
> +
> + /* Reset any capturing, in case caller runs
> + * this identical command again */
> + if (cmd->inbuf) {
> + close(infd[0]);
> + close(infd[1]);
Should we VIR_DEBUG if any of these close() calls fail?
> + }
> + if (cmd->outbuf == &outbuf) {
> + if (cmd->outfd != -1)
> + close(cmd->outfd);
> + cmd->outfd = -1;
> + cmd->outfdptr = NULL;
> + cmd->outbuf = NULL;
> + }
> + if (cmd->errbuf == &errbuf) {
> + if (cmd->errfd != -1)
> + close(cmd->errfd);
> + cmd->errfd = -1;
> + cmd->errfdptr = NULL;
> + cmd->errbuf = NULL;
> + }
> +
> + return ret;
> +}
> +
> +
> +/*
> + * Run the command asynchronously
> + * Returns -1 on any error executing the
> + * command. Returns 0 if the command executed.
> + */
> +int virCommandRunAsync(virCommandPtr cmd,
> + pid_t *pid)
> +{
> + int ret;
> + char *str;
> +
> + if (!cmd || cmd->has_error) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + if (cmd->pid != -1) {
> + virCommandError(VIR_ERR_INTERNAL_ERROR,
> + _("command is already running as pid %d"),
> + cmd->pid);
> + return -1;
> + }
> +
> + str = virArgvToString((const char *const *)cmd->args);
> + VIR_DEBUG("About to run %s", str ? str : cmd->args[0]);
> + VIR_FREE(str);
> +
> + ret = virExecWithHook((const char *const *)cmd->args,
> + (const char *const *)cmd->env,
> + &cmd->preserve,
> + &cmd->pid,
> + cmd->infd,
> + cmd->outfdptr,
> + cmd->errfdptr,
> + cmd->flags,
> + cmd->hook,
> + cmd->opaque,
> + cmd->pidfile);
Should virExecWithHook be moved into this new file?
> +
> + VIR_DEBUG("Command result %d, with PID is %d",
> + ret, (int)cmd->pid);
> +
> + if (ret == 0 && pid)
> + *pid = cmd->pid;
> +
> + return ret;
> +}
> +
> +
> +/*
> + * Wait for the async command to complete.
> + * Return -1 on any error waiting for
> + * completion. Returns 0 if the command
> + * finished with the exit status set
> + */
> +int virCommandWait(virCommandPtr cmd,
> + int *exitstatus)
> +{
> + int ret;
> + int status;
> +
> + if (!cmd || cmd->has_error) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + if (cmd->pid == -1) {
> + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("command is not yet running"));
> + return -1;
> + }
> +
> +
> + /* Wait for intermediate process to exit */
> + while ((ret = waitpid(cmd->pid, &status, 0)) == -1 &&
> + errno == EINTR);
> +
> + if (ret == -1) {
> + virReportSystemError(errno, _("unable to wait for process %d"),
> + cmd->pid);
> + return -1;
> + }
> +
> + cmd->pid = -1;
> +
> + if (exitstatus == NULL) {
> + if (status != 0) {
> + virCommandError(VIR_ERR_INTERNAL_ERROR,
> + _("Intermediate daemon process exited with status %d."),
> + WEXITSTATUS(status));
We have no guarantee that WEXITSTATUS is valid; the child could have
died from a signal. You probably want to check WIFSIGNALED as well.
> + return -1;
> + }
> + } else {
> + *exitstatus = status;
> + }
> +
> + return 0;
> +}
> +
> +
> +/*
> + * Release all resources
> + */
> +void virCommandFree(virCommandPtr cmd)
> +{
> + int i;
> + if (!cmd)
> + return;
> +
> + if (cmd->outfd != -1)
> + close(cmd->outfd);
> + if (cmd->errfd != -1)
> + close(cmd->errfd);
> +
> + for (i = 0 ; i < cmd->nargs ; i++)
> + VIR_FREE(cmd->args[i]);
> + VIR_FREE(cmd->args);
> +
> + for (i = 0 ; i < cmd->nenv ; i++)
> + VIR_FREE(cmd->env[i]);
> + VIR_FREE(cmd->env);
> +
> + VIR_FREE(cmd->pidfile);
> +
> + VIR_FREE(cmd);
> +}
> diff --git a/src/util/command.h b/src/util/command.h
> new file mode 100644
> index 0000000..f17767f
> --- /dev/null
> +++ b/src/util/command.h
> @@ -0,0 +1,197 @@
> +/*
> + * command.h: Child command execution
> + *
> + * Copyright (C) 2010 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#ifndef __VIR_COMMAND_H__
> +# define __VIR_COMMAND_H__
> +
> +# include "internal.h"
> +# include "util.h"
> +
> +typedef struct _virCommand virCommand;
> +typedef virCommand *virCommandPtr;
> +
> +/*
> + * Create a new command for named binary
> + */
> +virCommandPtr virCommandNew(const char *binary);
ATTRIBUTE_NONNULL(1)
> +
> +/*
> + * Create a new command with a NULL terminated
> + * set of args, taking binary from argv[0]
> + */
> +virCommandPtr virCommandNewArgs(const char *const*args);
ATTRIBUTE_NONNULL(1)
> +
> +/* All error report from these setup APIs is
> + * delayed until the Run/Exec/Wait methods
> + */
> +
> +/*
> + * Preserve the specified file descriptor
> + * in the child, instead of closing it
> + */
> +void virCommandPreserveFD(virCommandPtr cmd,
> + int fd);
ATTRIBUTE_NONNULL(1) here (and on all the other void functions) would
let you skip the check for cmd==NULL in all the implementations.
> +
> +/*
> + * Save the child PID in a pidfile
> + */
> +void virCommandSetPidFile(virCommandPtr cmd,
> + const char *pidfile);
ATTRIBUTE_NONNULL(2)
> +
> +/*
> + * Remove all capabilities from the child
> + */
> +void virCommandClearCaps(virCommandPtr cmd);
> +
> +/*
> + * Re-allow a specific capability
> + */
> +void virCommandAllowCap(virCommandPtr cmd,
> + int capability);
Comment out as long as the implementation is XXX and there are no callers?
> +
> +/*
> + * Daemonize the child process
> + */
> +void virCommandDaemonize(virCommandPtr cmd);
> +
> +
> +/*
> + * Add an environment variable to the child
> + * using separate name & value strings
> + */
> +void virCommandAddEnvPair(virCommandPtr cmd,
> + const char *name,
> + const char *value);
ATTRIBUTE_NONNULL(2), but arg 3 can be NULL per your implementation.
> +
> +/*
> + * Add an environemnt variable to the child
> + * using a preformated env string FOO=BAR
s/preformated/preformatted/
> + */
> +void virCommandAddEnvString(virCommandPtr cmd,
> + const char *str);
> +/*
> + * Pass an environment variable to the child
> + * using current process' value
> + */
> +void virCommandAddEnvPass(virCommandPtr cmd,
> + const char *name);
> +/*
> + * Pass a common set of environment variables
> + * to the child using current process' values
> + */
> +void virCommandAddEnvPassCommon(virCommandPtr cmd);
> +
> +/*
> + * Add a command line argument to the child
> + */
> +void virCommandAddArg(virCommandPtr cmd,
> + const char *val);
> +/*
> + * Add a command line argument to the child
> + */
> +void virCommandAddArgPair(virCommandPtr cmd,
> + const char *name,
> + const char *val);
ATTRIBUTE_NONNULL for both 2 and 3
> +/*
> + * Add a NULL terminated list of args
> + */
> +void virCommandAddArgSet(virCommandPtr cmd,
> + const char *const*vals);
> +
> +
> +/*
> + * Feed the child's stdin from a string buffer.
> + *
> + * NB: Only works with virCommandRun()
Can we detect if the user requested this and an async run, and error out
in that case?
> + */
> +void virCommandSetInputBuffer(virCommandPtr cmd,
> + const char *inbuf);
> +/*
> + * Capture the child's stdout to a string buffer
> + *
> + * NB: Only works with virCommandRun()
> + */
> +void virCommandSetOutputBuffer(virCommandPtr cmd,
> + char **outbuf);
> +/*
> + * Capture the child's stderr to a string buffer
> + *
> + * NB: Only works with virCommandRun()
> + */
> +void virCommandSetErrorBuffer(virCommandPtr cmd,
> + char **errbuf);
> +
> +/*
> + * Set a file descriptor as the child's stdin
> + */
> +void virCommandSetInputFD(virCommandPtr cmd,
> + int infd);
> +/*
> + * Set a file descriptor as the child's stdout
It would be helpful to add this:
"; if *outfd is -1, then Run will create a pipe and set *outfd"
> + */
> +void virCommandSetOutputFD(virCommandPtr cmd,
> + int *outfd);
> +/*
> + * Set a file descriptor as the child's stderr
> + */
> +void virCommandSetErrorFD(virCommandPtr cmd,
> + int *errfd);
> +
> +/*
> + * A hook function to run between fork + exec
> + */
> +void virCommandSetPreExecHook(virCommandPtr cmd,
> + virExecHook hook,
> + void *opaque);
> +
> +/*
> + * 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) ATTRIBUTE_RETURN_CHECK;
ATTRIBUTE_NONNULL(1) but arg 2 may be NULL
> +
> +/*
> + * Run the command asynchronously
> + * Returns -1 on any error executing the
> + * command. Returns 0 if the command executed.
> + */
> +int virCommandRunAsync(virCommandPtr cmd,
> + pid_t *pid) ATTRIBUTE_RETURN_CHECK;
Likewise
> +
> +/*
> + * Wait for the async command to complete.
> + * Return -1 on any error waiting for
> + * completion. Returns 0 if the command
> + * finished with the exit status set
> + */
> +int virCommandWait(virCommandPtr cmd,
> + int *exitstatus) ATTRIBUTE_RETURN_CHECK;
Likewise.
> +
> +/*
> + * Release all resources
> + */
> +void virCommandFree(virCommandPtr cmd);
List virCommandFree in cfg.mk's list of free-like functions.
> +
> +
> +#endif /* __VIR_COMMAND_H__ */
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index b5e09e3..5bd36f2 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -78,7 +78,8 @@ EXTRA_DIST = \
> $(patsubst %,qemuhelpdata/%,$(qemuhelpdata))
>
> noinst_PROGRAMS = virshtest conftest \
> - nodeinfotest statstest qparamtest
> + nodeinfotest statstest qparamtest \
> + commandtest commandhelper
>
> if WITH_XEN
> noinst_PROGRAMS += xml2sexprtest sexpr2xmltest \
> @@ -155,6 +156,7 @@ TESTS = virshtest \
> nodeinfotest \
> statstest \
> qparamtest \
> + commandtest \
> $(test_scripts)
>
> if WITH_XEN
> @@ -332,6 +334,16 @@ nodeinfotest_SOURCES = \
> nodeinfotest.c testutils.h testutils.c
> nodeinfotest_LDADD = $(LDADDS)
>
> +commandtest_SOURCES = \
> + commandtest.c testutils.h testutils.c
> +commandtest_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\""
> +commandtest_LDADD = $(LDADDS)
> +
> +commandhelper_SOURCES = \
> + commandhelper.c
> +commandhelper_CFLAGS = -Dabs_builddir="\"$(abs_builddir)\""
> +commandhelper_LDADD = $(LDADDS)
> +
> statstest_SOURCES = \
> statstest.c testutils.h testutils.c
> statstest_LDADD = $(LDADDS)
> diff --git a/tests/commanddata/test1.log b/tests/commanddata/test1.log
> new file mode 100644
> index 0000000..1b59206
> --- /dev/null
> +++ b/tests/commanddata/test1.log
> @@ -0,0 +1,12 @@
> +ENV:DISPLAY=:0.0
> +ENV:HOME=/home/test
> +ENV:HOSTNAME=test
> +ENV:LANG=C
> +ENV:LOGNAME=testTMPDIR=/tmp
> +ENV:PATH=/usr/bin:/bin
> +ENV:USER=test
> +FD:0
> +FD:1
> +FD:2
> +DAEMON:no
> +CWD:/tmp
> diff --git a/tests/commanddata/test10.log b/tests/commanddata/test10.log
> new file mode 100644
> index 0000000..e1d6092
> --- /dev/null
> +++ b/tests/commanddata/test10.log
> @@ -0,0 +1,14 @@
> +ARG:-version
> +ARG:-log=bar.log
> +ENV:DISPLAY=:0.0
> +ENV:HOME=/home/test
> +ENV:HOSTNAME=test
> +ENV:LANG=C
> +ENV:LOGNAME=testTMPDIR=/tmp
> +ENV:PATH=/usr/bin:/bin
> +ENV:USER=test
> +FD:0
> +FD:1
> +FD:2
> +DAEMON:no
> +CWD:/tmp
> diff --git a/tests/commanddata/test11.log b/tests/commanddata/test11.log
> new file mode 100644
> index 0000000..1b59206
> --- /dev/null
> +++ b/tests/commanddata/test11.log
> @@ -0,0 +1,12 @@
> +ENV:DISPLAY=:0.0
> +ENV:HOME=/home/test
> +ENV:HOSTNAME=test
> +ENV:LANG=C
> +ENV:LOGNAME=testTMPDIR=/tmp
> +ENV:PATH=/usr/bin:/bin
> +ENV:USER=test
> +FD:0
> +FD:1
> +FD:2
> +DAEMON:no
> +CWD:/tmp
> diff --git a/tests/commanddata/test12.log b/tests/commanddata/test12.log
> new file mode 100644
> index 0000000..1b59206
> --- /dev/null
> +++ b/tests/commanddata/test12.log
> @@ -0,0 +1,12 @@
> +ENV:DISPLAY=:0.0
> +ENV:HOME=/home/test
> +ENV:HOSTNAME=test
> +ENV:LANG=C
> +ENV:LOGNAME=testTMPDIR=/tmp
> +ENV:PATH=/usr/bin:/bin
> +ENV:USER=test
> +FD:0
> +FD:1
> +FD:2
> +DAEMON:no
> +CWD:/tmp
> diff --git a/tests/commanddata/test13.log b/tests/commanddata/test13.log
> new file mode 100644
> index 0000000..1b59206
> --- /dev/null
> +++ b/tests/commanddata/test13.log
> @@ -0,0 +1,12 @@
> +ENV:DISPLAY=:0.0
> +ENV:HOME=/home/test
> +ENV:HOSTNAME=test
> +ENV:LANG=C
> +ENV:LOGNAME=testTMPDIR=/tmp
> +ENV:PATH=/usr/bin:/bin
> +ENV:USER=test
> +FD:0
> +FD:1
> +FD:2
> +DAEMON:no
> +CWD:/tmp
> diff --git a/tests/commanddata/test2.log b/tests/commanddata/test2.log
> new file mode 100644
> index 0000000..6bd7974
> --- /dev/null
> +++ b/tests/commanddata/test2.log
> @@ -0,0 +1,14 @@
> +ENV:DISPLAY=:0.0
> +ENV:HOME=/home/test
> +ENV:HOSTNAME=test
> +ENV:LANG=C
> +ENV:LOGNAME=testTMPDIR=/tmp
> +ENV:PATH=/usr/bin:/bin
> +ENV:USER=test
> +FD:0
> +FD:1
> +FD:2
> +FD:3
> +FD:5
> +DAEMON:no
> +CWD:/tmp
> diff --git a/tests/commanddata/test3.log b/tests/commanddata/test3.log
> new file mode 100644
> index 0000000..1876685
> --- /dev/null
> +++ b/tests/commanddata/test3.log
> @@ -0,0 +1,12 @@
> +ENV:DISPLAY=:0.0
> +ENV:HOME=/home/test
> +ENV:HOSTNAME=test
> +ENV:LANG=C
> +ENV:LOGNAME=testTMPDIR=/tmp
> +ENV:PATH=/usr/bin:/bin
> +ENV:USER=test
> +FD:0
> +FD:1
> +FD:2
> +DAEMON:yes
> +CWD:/
> diff --git a/tests/commanddata/test4.log b/tests/commanddata/test4.log
> new file mode 100644
> index 0000000..f745c3f
> --- /dev/null
> +++ b/tests/commanddata/test4.log
> @@ -0,0 +1,10 @@
> +ENV:HOME=/home/test
> +ENV:LC_ALL=C
> +ENV:LOGNAME=testTMPDIR=/tmp
> +ENV:PATH=/usr/bin:/bin
> +ENV:USER=test
> +FD:0
> +FD:1
> +FD:2
> +DAEMON:no
> +CWD:/tmp
> diff --git a/tests/commanddata/test5.log b/tests/commanddata/test5.log
> new file mode 100644
> index 0000000..5394428
> --- /dev/null
> +++ b/tests/commanddata/test5.log
> @@ -0,0 +1,6 @@
> +ENV:DISPLAY=:0.0
> +FD:0
> +FD:1
> +FD:2
> +DAEMON:no
> +CWD:/tmp
> diff --git a/tests/commanddata/test6.log b/tests/commanddata/test6.log
> new file mode 100644
> index 0000000..cdfe445
> --- /dev/null
> +++ b/tests/commanddata/test6.log
> @@ -0,0 +1,11 @@
> +ENV:DISPLAY=:0.0
> +ENV:HOME=/home/test
> +ENV:LC_ALL=C
> +ENV:LOGNAME=testTMPDIR=/tmp
> +ENV:PATH=/usr/bin:/bin
> +ENV:USER=test
> +FD:0
> +FD:1
> +FD:2
> +DAEMON:no
> +CWD:/tmp
> diff --git a/tests/commanddata/test7.log b/tests/commanddata/test7.log
> new file mode 100644
> index 0000000..87874fd
> --- /dev/null
> +++ b/tests/commanddata/test7.log
> @@ -0,0 +1,7 @@
> +ENV:LANG=C
> +ENV:USER=test
> +FD:0
> +FD:1
> +FD:2
> +DAEMON:no
> +CWD:/tmp
> diff --git a/tests/commanddata/test8.log b/tests/commanddata/test8.log
> new file mode 100644
> index 0000000..e1d6092
> --- /dev/null
> +++ b/tests/commanddata/test8.log
> @@ -0,0 +1,14 @@
> +ARG:-version
> +ARG:-log=bar.log
> +ENV:DISPLAY=:0.0
> +ENV:HOME=/home/test
> +ENV:HOSTNAME=test
> +ENV:LANG=C
> +ENV:LOGNAME=testTMPDIR=/tmp
> +ENV:PATH=/usr/bin:/bin
> +ENV:USER=test
> +FD:0
> +FD:1
> +FD:2
> +DAEMON:no
> +CWD:/tmp
> diff --git a/tests/commanddata/test9.log b/tests/commanddata/test9.log
> new file mode 100644
> index 0000000..e1d6092
> --- /dev/null
> +++ b/tests/commanddata/test9.log
> @@ -0,0 +1,14 @@
> +ARG:-version
> +ARG:-log=bar.log
> +ENV:DISPLAY=:0.0
> +ENV:HOME=/home/test
> +ENV:HOSTNAME=test
> +ENV:LANG=C
> +ENV:LOGNAME=testTMPDIR=/tmp
> +ENV:PATH=/usr/bin:/bin
> +ENV:USER=test
> +FD:0
> +FD:1
> +FD:2
> +DAEMON:no
> +CWD:/tmp
> diff --git a/tests/commandhelper.c b/tests/commandhelper.c
> new file mode 100644
> index 0000000..f22a4d3
> --- /dev/null
> +++ b/tests/commandhelper.c
> @@ -0,0 +1,112 @@
> +
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <fcntl.h>
#include <string.h>, for strcmp and friends
> +
> +#include "internal.h"
> +#include "util.h"
> +#include "memory.h"
> +
> +
> +static int envsort(const void *a, const void *b) {
> + const char *const*astrptr = a;
> + const char *const*bstrptr = b;
> + const char *astr = *astrptr;
> + const char *bstr = *bstrptr;
> + char *aeq = strchr(astr, '=');
> + char *beq = strchr(bstr, '=');
> + char *akey = strndup(astr, aeq - astr);
> + char *bkey = strndup(bstr, beq - bstr);
> + int ret = strcmp(akey, bkey);
> + free(akey);
> + free(bkey);
> + return ret;
> +}
> +
> +int main(int argc, char **argv) {
> + int i, n;
> + char **origenv;
> + char **newenv;
> + FILE *log = fopen(abs_builddir "/commandhelper.log", "w");
> +
> + if (!log)
> + goto error;
> +
> + for (i = 1 ; i < argc ; i++) {
> + fprintf(log, "ARG:%s\n", argv[i]);
> + }
> +
> + origenv = environ;
> + n = 0;
> + while (*origenv != NULL) {
> + n++;
> + origenv++;
> + }
> +
> + if (VIR_ALLOC_N(newenv, n) < 0) {
> + exit(EXIT_FAILURE);
> + }
> +
> + origenv = environ;
> + n = i = 0;
> + while (*origenv != NULL) {
> + newenv[i++] = *origenv;
> + n++;
Why set i = 0, and increment both i and n, when you could have just done
newenv[n++] = *origenv
> + origenv++;
> + }
> + qsort(newenv, n, sizeof(newenv[0]), envsort);
> +
> + for (i = 0 ; i < n ; i++) {
> + fprintf(log, "ENV:%s\n", newenv[i]);
> + }
> +
> + for (i = 0 ; i < sysconf(_SC_OPEN_MAX) ; i++) {
Will this even compile on mingw? Gnulib provides getdtablesize() (oops,
it's still LGPLv3) which portably lets you determine a nice maximum
bound on the number of open files, even when sysconf() is not available.
> + int f;
> + int closed;
> + if (i == fileno(log))
> + continue;
> + closed = fcntl(i, F_GETFD, &f) == -1 &&
fcntl(i, F_GETFD) is sufficient - the third argument is optional if you
are querying flag status.
> + errno == EBADF;
Probably sufficient to just check fcntl()==-1, without also checking errno.
> + if (!closed)
> + fprintf(log, "FD:%d\n", i);
> + }
> +
> + fprintf(log, "DAEMON:%s\n", getppid() == 1 ? "yes" : "no");
Is this portable to mingw?
> + char cwd[1024];
> + fprintf(log, "CWD:%s\n", getcwd(cwd, sizeof(cwd)));
Gnulib guarantees that getcwd(NULL,0) is portable (oops, it's currently
GPL). It is unlikely that a tester will ever run this in a super-deep
hierarchy, but just in case, should you use NULLSTR(getcwd) in case
getcwd() fails?
> +
> + fclose(log);
I guess it's okay to skip checking for errors writing to log, since the
test will later fail if our comparison to expected log contents fails.
> +
> + char buf[1024];
> + ssize_t got;
> +
> + fprintf(stdout, "BEGIN STDOUT\n");
> + fflush(stdout);
> + fprintf(stderr, "BEGIN STDERR\n");
> + fflush(stderr);
> +
> + for (;;) {
> + got = read(STDIN_FILENO, buf, sizeof(buf));
> + if (got < 0)
> + goto error;
> + if (got == 0)
> + break;
> + if (safewrite(STDOUT_FILENO, buf, got) != got)
> + goto error;
> + if (safewrite(STDERR_FILENO, buf, got) != got)
> + goto error;
> + }
> +
> + fprintf(stdout, "END STDOUT\n");
> + fflush(stdout);
> + fprintf(stderr, "END STDERR\n");
> + fflush(stderr);
> +
> + return EXIT_SUCCESS;
> +
> +error:
> + return EXIT_FAILURE;
> +}
> diff --git a/tests/commandtest.c b/tests/commandtest.c
> new file mode 100644
> index 0000000..c66d345
> --- /dev/null
> +++ b/tests/commandtest.c
> @@ -0,0 +1,494 @@
> +#include <config.h>
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <signal.h>
> +
> +#include "testutils.h"
> +#include "internal.h"
> +#include "nodeinfo.h"
> +#include "util.h"
> +#include "memory.h"
> +#include "command.h"
> +
> +#ifndef __linux__
Is this test really linux-specific? It seems like other capable
platforms, like cygwin, might be able to run it just fine.
> +
> +static int
> +mymain(int argc ATTRIBUTE_UNUSED, char **argv ATTRIBUTE_UNUSED)
> +{
> + exit (EXIT_AM_SKIP);
> +}
> +
> +#else
> +
> +static char *progname;
> +static char *abs_srcdir;
> +
> +
> +static int checkoutput(const char *testname) {
> + int ret = -1;
> + char cwd[1024];
> + char *expectname = NULL;
> + char *expectlog = NULL;
> + char *actualname = NULL;
> + char *actuallog = NULL;
> +
> + if (!getcwd(cwd, sizeof(cwd)))
> + return -1;
> +
> + if (virAsprintf(&expectname, "%s/commanddata/%s.log", abs_srcdir, testname) < 0)
> + goto cleanup;
> + if (virAsprintf(&actualname, "%s/commandhelper.log", abs_builddir) < 0)
> + goto cleanup;
> +
> + if (virFileReadAll(expectname, 1024*64, &expectlog) < 0) {
> + fprintf(stderr, "cannot read %s\n", expectname);
> + goto cleanup;
> + }
> +
> + if (virFileReadAll(actualname, 1024*64, &actuallog) < 0) {
> + fprintf(stderr, "cannot read %s\n", actualname);
> + goto cleanup;
> + }
> +
> + if (STRNEQ(expectlog, actuallog)) {
> + virtTestDifference(stderr, expectlog, actuallog);
> + goto cleanup;
> + }
> +
> +
> + ret = 0;
> +
> +cleanup:
> + unlink(actuallog);
> + VIR_FREE(actuallog);
> + VIR_FREE(actualname);
> + VIR_FREE(expectlog);
> + VIR_FREE(expectname);
> + return ret;
> +}
> +
> +/*
> + * Run program, no args, inherit all ENV, keep CWD.
> + * Only stdin/out/err open
> + */
> +static int test0(const void *unused ATTRIBUTE_UNUSED) {
> + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist");
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + return 0;
> + }
> +
> + virCommandFree(cmd);
> +
> + return -1;
> +}
> +
> +/*
> + * Run program, no args, inherit all ENV, keep CWD.
> + * Only stdin/out/err open
> + */
> +static int test1(const void *unused ATTRIBUTE_UNUSED) {
> + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virErrorPtr err = virGetLastError();
> + printf("Cannot run child %s\n", err->message);
> + return -1;
> + }
> +
> + virCommandFree(cmd);
> +
> + return checkoutput("test1");
> +}
> +
> +/*
> + * Run program, no args, inherit all ENV, keep CWD.
> + * stdin/out/err + two extra FD open
> + */
> +static int test2(const void *unused ATTRIBUTE_UNUSED) {
> + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
> + int newfd1 = dup(STDERR_FILENO);
> + int newfd2 = dup(STDERR_FILENO);
> + int newfd3 = dup(STDERR_FILENO);
> + close(newfd2);
> +
> + virCommandPreserveFD(cmd, newfd1);
> + virCommandPreserveFD(cmd, newfd3);
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virErrorPtr err = virGetLastError();
> + printf("Cannot run child %s\n", err->message);
> + return -1;
> + }
> +
> + virCommandFree(cmd);
> +
> + return checkoutput("test2");
> +}
> +
> +
> +/*
> + * Run program, no args, inherit all ENV, CWD is /
> + * Only stdin/out/err open.
> + * Daemonized
> + */
> +static int test3(const void *unused ATTRIBUTE_UNUSED) {
> + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
> + pid_t pid;
> + char *pidfile = virFilePid(abs_builddir, "commandhelper");
> +
> + virCommandSetPidFile(cmd, pidfile);
> + virCommandDaemonize(cmd);
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virErrorPtr err = virGetLastError();
> + printf("Cannot run child %s\n", err->message);
> + return -1;
> + }
> +
> + if (virFileReadPid(abs_builddir, "commandhelper", &pid) != 0) {
> + printf("cannot read pidfile\n");
> + return -1;
> + }
> + while (kill(pid, 0) != -1)
> + usleep(100*1000);
> +
> + virCommandFree(cmd);
> +
> + VIR_FREE(pidfile);
> +
> + return checkoutput("test3");
> +}
> +
> +
> +/*
> + * Run program, no args, inherit filtered ENV, keep CWD.
> + * Only stdin/out/err open
> + */
> +static int test4(const void *unused ATTRIBUTE_UNUSED) {
> + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
> +
> + virCommandAddEnvPassCommon(cmd);
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virErrorPtr err = virGetLastError();
> + printf("Cannot run child %s\n", err->message);
> + return -1;
> + }
> +
> + virCommandFree(cmd);
> +
> + return checkoutput("test4");
> +}
> +
> +
> +/*
> + * Run program, no args, inherit filtered ENV, keep CWD.
> + * Only stdin/out/err open
> + */
> +static int test5(const void *unused ATTRIBUTE_UNUSED) {
> + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
> +
> + virCommandAddEnvPass(cmd, "DISPLAY");
> + virCommandAddEnvPass(cmd, "DOESNOTEXIST");
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virErrorPtr err = virGetLastError();
> + printf("Cannot run child %s\n", err->message);
> + return -1;
> + }
> +
> + virCommandFree(cmd);
> +
> + return checkoutput("test5");
> +}
> +
> +
> +/*
> + * Run program, no args, inherit filtered ENV, keep CWD.
> + * Only stdin/out/err open
> + */
> +static int test6(const void *unused ATTRIBUTE_UNUSED) {
> + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
> +
> + virCommandAddEnvPassCommon(cmd);
> + virCommandAddEnvPass(cmd, "DISPLAY");
> + virCommandAddEnvPass(cmd, "DOESNOTEXIST");
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virErrorPtr err = virGetLastError();
> + printf("Cannot run child %s\n", err->message);
> + return -1;
> + }
> +
> + virCommandFree(cmd);
> +
> + return checkoutput("test6");
> +}
> +
> +/*
> + * Run program, no args, inherit filtered ENV, keep CWD.
> + * Only stdin/out/err open
> + */
> +static int test7(const void *unused ATTRIBUTE_UNUSED) {
> + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
> +
> + virCommandAddEnvString(cmd, "LANG=C");
> + virCommandAddEnvPair(cmd, "USER", "test");
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virErrorPtr err = virGetLastError();
> + printf("Cannot run child %s\n", err->message);
> + return -1;
> + }
> +
> + virCommandFree(cmd);
> +
> + return checkoutput("test7");
> +}
> +
> +
> +/*
> + * Run program, some args, inherit all ENV, keep CWD.
> + * Only stdin/out/err open
> + */
> +static int test8(const void *unused ATTRIBUTE_UNUSED) {
> + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
> +
> + virCommandAddArg(cmd, "-version");
> + virCommandAddArgPair(cmd, "-log", "bar.log");
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virErrorPtr err = virGetLastError();
> + printf("Cannot run child %s\n", err->message);
> + return -1;
> + }
> +
> + virCommandFree(cmd);
> +
> + return checkoutput("test8");
> +}
> +
> +
> +/*
> + * Run program, some args, inherit all ENV, keep CWD.
> + * Only stdin/out/err open
> + */
> +static int test9(const void *unused ATTRIBUTE_UNUSED) {
> + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
> + const char *const args[] = {
> + "-version", "-log=bar.log", NULL,
> + };
> +
> + virCommandAddArgSet(cmd, args);
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virErrorPtr err = virGetLastError();
> + printf("Cannot run child %s\n", err->message);
> + return -1;
> + }
> +
> + virCommandFree(cmd);
> +
> + return checkoutput("test9");
> +}
> +
> +/*
> + * Run program, some args, inherit all ENV, keep CWD.
> + * Only stdin/out/err open
> + */
> +static int test10(const void *unused ATTRIBUTE_UNUSED) {
> + const char *args[] = {
> + abs_builddir "/commandhelper",
> + "-version", "-log=bar.log", NULL,
> + };
> + virCommandPtr cmd = virCommandNewArgs(args);
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virErrorPtr err = virGetLastError();
> + printf("Cannot run child %s\n", err->message);
> + return -1;
> + }
> +
> + virCommandFree(cmd);
> +
> + return checkoutput("test10");
> +}
> +
> +/*
> + * Run program, no args, inherit all ENV, keep CWD.
> + * Only stdin/out/err open. Set stdin data
> + */
> +static int test11(const void *unused ATTRIBUTE_UNUSED) {
> + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
> +
> + virCommandSetInputBuffer(cmd, "Hello World\n");
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virErrorPtr err = virGetLastError();
> + printf("Cannot run child %s\n", err->message);
> + return -1;
> + }
> +
> + virCommandFree(cmd);
> +
> + return checkoutput("test11");
> +}
> +
> +/*
> + * Run program, no args, inherit all ENV, keep CWD.
> + * Only stdin/out/err open. Set stdin data
> + */
> +static int test12(const void *unused ATTRIBUTE_UNUSED) {
> + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
> + char *outactual = NULL;
> + const char *outexpect = "BEGIN STDOUT\n"
> + "Hello World\n"
> + "END STDOUT\n";
> + int ret = -1;
> +
> + virCommandSetInputBuffer(cmd, "Hello World\n");
> + virCommandSetOutputBuffer(cmd, &outactual);
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virErrorPtr err = virGetLastError();
> + printf("Cannot run child %s\n", err->message);
> + return -1;
> + }
> +
> + virCommandFree(cmd);
> +
> + if (!STREQ(outactual, outexpect)) {
> + virtTestDifference(stderr, outactual, outexpect);
> + goto cleanup;
> + }
> +
> + if (checkoutput("test12") < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> +cleanup:
> + VIR_FREE(outactual);
> + return ret;
> +}
> +
> +/*
> + * Run program, no args, inherit all ENV, keep CWD.
> + * Only stdin/out/err open. Set stdin data
> + */
> +static int test13(const void *unused ATTRIBUTE_UNUSED) {
> + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper");
> + char *outactual = NULL;
> + const char *outexpect = "BEGIN STDOUT\n"
> + "Hello World\n"
> + "END STDOUT\n";
> + char *erractual = NULL;
> + const char *errexpect = "BEGIN STDERR\n"
> + "Hello World\n"
> + "END STDERR\n";
> + int ret = -1;
> +
> + virCommandSetInputBuffer(cmd, "Hello World\n");
> + virCommandSetOutputBuffer(cmd, &outactual);
> + virCommandSetErrorBuffer(cmd, &erractual);
> +
> + if (virCommandRun(cmd, NULL) < 0) {
> + virErrorPtr err = virGetLastError();
> + printf("Cannot run child %s\n", err->message);
> + return -1;
> + }
> +
> + virCommandFree(cmd);
> +
> + if (!STREQ(outactual, outexpect)) {
> + virtTestDifference(stderr, outactual, outexpect);
> + goto cleanup;
> + }
> + if (!STREQ(erractual, errexpect)) {
> + virtTestDifference(stderr, erractual, errexpect);
> + goto cleanup;
> + }
> +
> + if (checkoutput("test13") < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> +cleanup:
> + VIR_FREE(outactual);
> + VIR_FREE(erractual);
> + return ret;
> +}
> +
> +
> +static int
> +mymain(int argc, char **argv)
> +{
> + int ret = 0;
> + char cwd[PATH_MAX];
> +
> + abs_srcdir = getenv("abs_srcdir");
> + if (!abs_srcdir)
> + abs_srcdir = getcwd(cwd, sizeof(cwd));
> +
> + progname = argv[0];
> +
> + if (argc > 1) {
> + fprintf(stderr, "Usage: %s\n", progname);
> + return(EXIT_FAILURE);
> + }
> +
> + if (chdir("/tmp") < 0)
> + return(EXIT_FAILURE);
Do we really want to risk spurious test failures by running directly in
/tmp, or should we be creating a secure subdirectory?
> +
> + virInitialize();
> +
> + const char *const newenv[] = {
> + "PATH=/usr/bin:/bin",
If you enable the test for Cygwin, you have to be careful about existing
PATH elements (cygwin programs will not run if the windows system files
are not somewhere in the path).
> + "HOSTNAME=test",
> + "LANG=C",
> + "HOME=/home/test",
> + "USER=test",
> + "LOGNAME=test"
> + "TMPDIR=/tmp",
> + "DISPLAY=:0.0",
> + NULL
> + };
> + environ = (char **)newenv;
> +
> +#define DO_TEST(NAME) \
> + if (virtTestRun("Command Exec " #NAME " test", \
> + 1, NAME, NULL) < 0) \
> + ret = -1
> +
> + char *actualname;
> + if (virAsprintf(&actualname, "%s/commandhelper.log", abs_builddir) < 0)
> + return EXIT_FAILURE;
> + unlink(actualname);
> + VIR_FREE(actualname);
> +
> + DO_TEST(test0);
> + DO_TEST(test1);
> + DO_TEST(test2);
> + DO_TEST(test3);
> + DO_TEST(test4);
> + DO_TEST(test5);
> + DO_TEST(test6);
> + DO_TEST(test7);
> + DO_TEST(test8);
> + DO_TEST(test9);
> + DO_TEST(test10);
> + DO_TEST(test11);
> + DO_TEST(test12);
> + DO_TEST(test13);
> +
> + return(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE);
> +}
> +
> +#endif /* __linux__ */
> +
> +VIRT_TEST_MAIN(mymain)
Good start, looking forward to v2!
--
Eric Blake eblake@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list