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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 05/25/2010 07:24 AM, Daniel P. Berrange wrote:

--- docs/Makefile.am | 12 ++++++++++-- docs/internals.html.in | 9 +++++++++ docs/sitemap.html.in | 4 ++++ 3 files changed, 23 insertions(+), 2 deletions(-)

+++ b/docs/Makefile.am @@ -53,7 +53,7 @@ gif = \ architecture.gif \ node.gif

-dot_html_in = $(wildcard *.html.in) +dot_html_in = $(wildcard *.html.in) $(wildcard internals/*.html.in) dot_html = $(dot_html_in:%.html.in=%.html)

Note to myself: If I ever get time to revisit my VPATH patches, I've got a merge conflict in this area. Jim already pointed out the new file omission, but given your first message had the text-ified version, here's some spelling fixes:

Spawning processes / commands from libvirt drivers

This page describes the usage of libvirt APIs for spawning processes / commands from libvirt drivers. All code is required to use these APIs

Problems with standard POSIX APIs

The POSIX specification includes a number of APIs for spawning processes / commands, but they suffer from a number of flaws

* fork+exec: The lowest & most flexible level, but very hard to use correctly / safely. It is easy to leak file descriptors, have unexpected signal handler behaviour and not handle edge cases

Also, not portable to mingw.

* system: Convenient if you don't care about capturing command output, but has the serious downside that the command string is interpreted by the shell. This makes it very dangerous to use, because improperly validated user input can lead to exploits via shell meta characters.

* popen: Similar problems to system; also has issues affecting signal handling related to other child processes.

* posix_spawn: A half-way house between simplicity of system() and the flexibility of fork+exec. It does not allow for a couple of important features though, such as running a hook between the fork+exec stage, or closing all open file descriptors.

Due to the problems mentioned with each of these, libvirt driver code must not use any of the above APIs. Historically libvirt provided a higher level API known as virExec. This was wrapper around fork+exec, in a similar style to posix_spawn, but with a few more features.

I didn't see anything in patch 1/3 that deprecated virExec in the public headers. Is it worth adding conditional deprecation (guarded by a macro defined only in command.c), to make sure it is not called anywhere except in the command.c implementation?

This wrapper still suffered from a number of problems. Handling command cleanup via waitpid() is overly complex & error prone for most usage. Building up the argv[] + env[] string arrays is quite cumbersome and error prone, particularly wrt memory leak / OOM handling.

The libvirt command execution API

There is now a high level API that provides a safe and flexible way to spawn commands, which prevents the most common errors & is easy to code

s/&/and/

against. This code is provided in the src/util/command.h header which can be imported using #include "command.h"

Defining commands in libvirt

The first step is to declare what command is to be executed. The command name can be either a fully qualified path, or a bare command name. In the latter case it will be resolved wrt the $PATH environment variable.

virCommandPtr cmd = virCommandNew("/usr/bin/dnsmasq");

There is no need to check for allocation failure after virCommandNew. This will be detected and reported at a later time.

Adding arguments to the command

There are a number of APIs for adding arguments to a command. To add a direct string arg

virCommandAddArg(cmd, "-strict-order");

If an argument takes an attached value of the form -arg=val, then this can be done using

virCommandAddArgPair(cmd, "--conf-file", "/etc/dnsmasq.conf");

To add a entire NULL terminated array of arguments in one go

const char *const args[] = { "--strict-order", "--except-interface", "lo", "--domain", "localdomain", NULL }; virCommandAddArgSet(cmd, args);

This latter option can also be used at time of initial construction of the virCommandPtr object

const char *const args[] = { "--strict-order", "--except-interface", "lo", "--domain", "localdomain", NULL }; virCommandPtr cmd = virCommandNewArgs(cmd, args);

Setting up the environment

By default a command will inherit all environment variables from the current process. Generally this is not desirable and a customized environment will be more suitable. Any customization done via the following APIs will prevent inheritance of any existing environment variables unless explicitly allowed. The first step is usually to pass through a small number of variables from the current process.

virCommandAddEnvPassCommon(cmd);

This has now set up a clean environment for the child, passing through PATH, LD_PRELOAD, LD_LIBRARY_PATH, HOME, USER, LOGNAME and TMPDIR. Furthermore it will explicitly set LC_ALL=C to avoid unexpected localization of command output. Further variables can be passed through from parent explicitly:

virCommandAddEnvPass(cmd, "DISPLAY"); virCommandAddEnvPass(cmd, "XAUTHORITY");

To define an environment variable in the child with an separate key / value:

virCommandAddEnvPair(cmd, "TERM", "xterm");

If the key/value pair is pre-formatted in the right format, it can be set directly

virCommandAddEnvString(cmd, "TERM=xterm");

Miscellaneous other options

Normally the spawned command will retain the current process as its parent. If the current process dies, the child will then (usually) be terminated too. If this cleanup is not desired, then the command should be marked as daemonized:

virCommandDaemonize(cmd);

When daemonizing a command, the PID visible from the caller will be that of the intermediate process, not the actual damonized command. If the PID

s/damonized/daemonized/

of the real command is required then a pidfile can be requested

virCommandSetPidFile(cmd, "/var/run/dnsmasq.pid");

This PID file is guaranteed to be written before the intermediate process exits.

Reducing command privileges

Normally a command will inherit all privileges of the current process. To restrict what a command can do, it is possible to request that all its capabilities are cleared. With this done it will only be able to access resources for which it has explicit DAC permissions

virCommandClearCaps(cmd);

Managing file handles

To prevent unintended resource leaks to child processes, all open file handles will be closed in the child, and its stdin/out/err all connected to /dev/null. It is possible to allow an open file handle to be passed into the child:

virCommandPreserveFD(cmd, 5);

With this file descriptor 5 in the current process remains open as file descriptor 5 in the child. For stdin/out/err it is usually neccessary to

s/neccessary/necessary/

map a file handle. To attach file descriptor 7 in the current process to stdin in the child:

virCommandSetInputFD(cmd, 7);

Equivalently to redirect stdout or stderr in the child, pass in a pointer to the desired handle

int outfd = open("out.log", "w+"); int errfd = open("err.log", "w+"); virCommandSetOutputFD(cmd, &outfd); virCommandSetErrorFD(cmd, &errfd);

Alternatively it is possible to request that a pipe be created to fetch stdout/err in the parent, by initializing the FD to -1.

int outfd = -1; int errfd = -1 virCommandSetOutputFD(cmd, &outfd); virCommandSetErrorFD(cmd, &errfd);

Once the command is running, outfd and errfd will be initialized with valid file handles that can be read from.

Feeding & capturing strings to/from the child

Often dealing with file handles for stdin/out/err is unneccessarily

s/unneccessarily/unnecessarily/

complex. It is possible to specify a string buffer to act as the data source for the child's stdin

const char *input = "Hello World\n"; virCommandSetInputBuffer(cmd, input);

Similarly it is possible to request that the child's stdout/err be redirected into a string buffer

char *output = NULL, *errors = NULL; virCommandSetOutputBuffer(cmd, &output); virCommandSetErrorBuffer(cmd, &errors);

Once the command has finished executing, these buffers will contain the output. It is the callers responsibility to free these buffers.

Running commands synchronously

For most commands, the desired behaviour is to spawn the command, wait for

/me decides not to even bother with the UK vs. US issue.

it to complete & exit and then check that its exit status is zero.

if (virCommandRun(cmd, NULL) < 0) return -1;

Note: if the command has been daemonized this will only block & wait for the intermediate process, not the real command. virCommandRun will report on any errors that have occured upon this point with all previous API

s/occured/occurred/

calls. If the command fails to run, or exits with non-zero status an error will be reported via normal libvirt error infrastructure. If a non-zero exit status can represent a success condition, it is possible to request the exit status and perform that check manually instead of letting virCommandRun raise the error

int status; if (virCommandRun(cmd, &status) < 0) return -1;

if (WEXITSTATUS(status) ...) { ...do stuff... }

Running commands asynchronously

In certain complex scenarios, particularly special I/O handling is required for the child's stdin/err/out it will be neccessary to run the

s/neccessary/necessary/

command asynchronously and wait for completion separately.

pid_t pid; if (virCommandRunAsync(cmd, &pid) < 0) return -1;

... do something while pid is running ...

int status; if (virCommandWait(cmd, &status) < 0) return -1;

if (WEXITSTATUS(status)...) { ..do stuff.. }

As with virCommandRun, the status arg for virCommandWait can be omitted, in which case it will validate that exit status is zero and raise an error if not.

Releasing resources

Once the command has been executed, or if execution has been abandoned, it is neccessary to release resources associated with the virCommandPtr object. This is done with:

virCommandFree(cmd);

There is no need to check if cmd is NULL before calling virCommandFree. This scenario is handled automatically. If the command is still running, it will be forcably killed and cleaned up (via waitpid).

s/forcably/forcibly/

Complete examples

This shows a complete example usage of the APIs roughly using the libvirt source src/util/hooks.c

int runhook(const char *drvstr, const char *id, const char *opstr, const char *subopstr, const char *extra) { int ret; char *path; virCommandPtr cmd;

ret = virBuildPath(&path, LIBVIRT_HOOK_DIR, drvstr); if ((ret < 0) || (path == NULL)) { virHookReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to build path for %s hook"), drvstr); return -1; }

cmd = virCommandNew(path); VIR_FREE(path);

virCommandAddEnvPassCommon(cmd);

virCommandAddArg(cmd, id); virCommandAddArg(cmd, opstr); virCommandAddArg(cmd, subopstr); virCommandAddArg(cmd, extra);

virCommandSetInputBuffer(cmd, input);

ret = virCommandRun(cmd, NULL);

virCommandFree(cmd);

return ret;

In this example, the command is being run synchronously. A pre-formatted string is being fed to the command as its stdin. The command takes four arguments, and has a minimal set of environment variables passed down. In this example, the code does not require any error checking. All errors are reported by the virCommandRun method, and the exit status from this is returned to the caller to handle as desired.

Also, see my comments to 0/3 regarding semantic issues (this review only did spelling/grammar issues). -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org