* src/util/util.c (saferead): Fix return type. (safewrite): Fix indentation. --- v2: new patch. Not really essential to the series, so much as a trivial cleanup I noticed while in the area. src/util/util.c | 64 ++++++++++++++++++++++++++++-------------------------- src/util/util.h | 2 +- 2 files changed, 34 insertions(+), 32 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 3a27c23..f6050de 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -88,42 +88,44 @@ verify(sizeof(gid_t) <= sizeof (unsigned int) && __FUNCTION__, __LINE__, __VA_ARGS__) /* Like read(), but restarts after EINTR */ -int saferead(int fd, void *buf, size_t count) -{ - size_t nread = 0; - while (count > 0) { - ssize_t r = read(fd, buf, count); - if (r < 0 && errno == EINTR) - continue; - if (r < 0) - return r; - if (r == 0) - return nread; - buf = (char *)buf + r; - count -= r; - nread += r; - } - return nread; +ssize_t +saferead(int fd, void *buf, size_t count) +{ + size_t nread = 0; + while (count > 0) { + ssize_t r = read(fd, buf, count); + if (r < 0 && errno == EINTR) + continue; + if (r < 0) + return r; + if (r == 0) + return nread; + buf = (char *)buf + r; + count -= r; + nread += r; + } + return nread; } /* Like write(), but restarts after EINTR */ -ssize_t safewrite(int fd, const void *buf, size_t count) -{ - size_t nwritten = 0; - while (count > 0) { - ssize_t r = write(fd, buf, count); - - if (r < 0 && errno == EINTR) - continue; - if (r < 0) - return r; - if (r == 0) - return nwritten; - buf = (const char *)buf + r; - count -= r; - nwritten += r; - } - return nwritten; +ssize_t +safewrite(int fd, const void *buf, size_t count) +{ + size_t nwritten = 0; + while (count > 0) { + ssize_t r = write(fd, buf, count); + + if (r < 0 && errno == EINTR) + continue; + if (r < 0) + return r; + if (r == 0) + return nwritten; + buf = (const char *)buf + r; + count -= r; + nwritten += r; + } + return nwritten; } #ifdef HAVE_POSIX_FALLOCATE diff --git a/src/util/util.h b/src/util/util.h index edbf01e..deaf8bb 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -37,7 +37,7 @@ # define MIN(a, b) ((a) < (b) ? (a) : (b)) # endif -int saferead(int fd, void *buf, size_t count) ATTRIBUTE_RETURN_CHECK; +ssize_t saferead(int fd, void *buf, size_t count) ATTRIBUTE_RETURN_CHECK; ssize_t safewrite(int fd, const void *buf, size_t count) ATTRIBUTE_RETURN_CHECK; int safezero(int fd, int flags, off_t offset, off_t len) -- 1.7.3.2

From: Daniel P. Berrange <berrange@redhat.com> This introduces a new set of APIs in src/util/command.h to use for invoking commands. This is intended to replace all current usage of virRun and virExec variants, with a more flexible and less error prone API. * src/util/command.c: New file. * src/util/command.h: New header. * src/Makefile.am (UTIL_SOURCES): Build it. * src/libvirt_private.syms: Export symbols internally. * tests/commandtest.c: New test. * tests/Makefile.am (check_PROGRAMS): Run it. * tests/commandhelper.c: Auxiliary program. * tests/commanddata/test2.log - test15.log: New expected outputs. * cfg.mk (useless_free_options): Add virCommandFree. * po/POTFILES.in: New translation. * .x-sc_avoid_write: Add exemption. * tests/.gitignore: Ignore new built file. --- v2: add virCommandTransferFD, virCommandToString, virCommandAddArgFormat, virCommandNewArgList, virCommandWriteArgLog, virCommandNonblockingFDs. Fix virCommandRunAsync and virCommandFree to free transfered FDs. Add a bit more test exposure. .x-sc_avoid_write | 1 + cfg.mk | 1 + po/POTFILES.in | 1 + src/Makefile.am | 1 + src/libvirt_private.syms | 35 ++ src/util/command.c | 1138 ++++++++++++++++++++++++++++++++++++++++++ src/util/command.h | 260 ++++++++++ tests/.gitignore | 4 + tests/Makefile.am | 18 +- tests/commanddata/test10.log | 14 + tests/commanddata/test11.log | 14 + tests/commanddata/test12.log | 12 + tests/commanddata/test13.log | 12 + tests/commanddata/test14.log | 12 + tests/commanddata/test15.log | 12 + tests/commanddata/test16.log | 1 + tests/commanddata/test2.log | 12 + tests/commanddata/test3.log | 14 + tests/commanddata/test4.log | 12 + tests/commanddata/test5.log | 10 + tests/commanddata/test6.log | 6 + tests/commanddata/test7.log | 11 + tests/commanddata/test8.log | 7 + tests/commanddata/test9.log | 18 + tests/commandhelper.c | 137 +++++ tests/commandtest.c | 630 +++++++++++++++++++++++ 26 files changed, 2392 insertions(+), 1 deletions(-) create mode 100644 src/util/command.c create mode 100644 src/util/command.h create mode 100644 tests/commanddata/test10.log create mode 100644 tests/commanddata/test11.log create mode 100644 tests/commanddata/test12.log create mode 100644 tests/commanddata/test13.log create mode 100644 tests/commanddata/test14.log create mode 100644 tests/commanddata/test15.log create mode 100644 tests/commanddata/test16.log create mode 100644 tests/commanddata/test2.log create mode 100644 tests/commanddata/test3.log create mode 100644 tests/commanddata/test4.log create mode 100644 tests/commanddata/test5.log create mode 100644 tests/commanddata/test6.log create mode 100644 tests/commanddata/test7.log create mode 100644 tests/commanddata/test8.log create mode 100644 tests/commanddata/test9.log create mode 100644 tests/commandhelper.c create mode 100644 tests/commandtest.c diff --git a/.x-sc_avoid_write b/.x-sc_avoid_write index 232504f..f6fc1b2 100644 --- a/.x-sc_avoid_write +++ b/.x-sc_avoid_write @@ -1,6 +1,7 @@ ^src/libvirt\.c$ ^src/fdstream\.c$ ^src/qemu/qemu_monitor\.c$ +^src/util/command\.c$ ^src/util/util\.c$ ^src/xen/xend_internal\.c$ ^daemon/libvirtd.c$ diff --git a/cfg.mk b/cfg.mk index dea8301..6312632 100644 --- a/cfg.mk +++ b/cfg.mk @@ -77,6 +77,7 @@ useless_free_options = \ --name=virCapabilitiesFreeHostNUMACell \ --name=virCapabilitiesFreeMachines \ --name=virCgroupFree \ + --name=virCommandFree \ --name=virConfFreeList \ --name=virConfFreeValue \ --name=virDomainChrDefFree \ diff --git a/po/POTFILES.in b/po/POTFILES.in index 2820ac1..e7be0d3 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -78,6 +78,7 @@ src/uml/uml_driver.c src/util/authhelper.c src/util/bridge.c src/util/cgroup.c +src/util/command.c src/util/conf.c src/util/dnsmasq.c src/util/hooks.c diff --git a/src/Makefile.am b/src/Makefile.am index a9a1986..0923d60 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -48,6 +48,7 @@ UTIL_SOURCES = \ util/bitmap.c util/bitmap.h \ util/bridge.c util/bridge.h \ util/buf.c util/buf.h \ + util/command.c util/command.h \ util/conf.c util/conf.h \ util/cgroup.c util/cgroup.h \ util/event.c util/event.h \ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d9f70a7..f97a315 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -83,6 +83,41 @@ virCgroupSetMemorySoftLimit; virCgroupSetSwapHardLimit; +# command.h +virCommandAddArg; +virCommandAddArgFormat; +virCommandAddArgList; +virCommandAddArgPair; +virCommandAddArgSet; +virCommandAddEnvPair; +virCommandAddEnvPass; +virCommandAddEnvPassCommon; +virCommandAddEnvString; +virCommandClearCaps; +virCommandDaemonize; +virCommandFree; +virCommandNew; +virCommandNewArgList; +virCommandNewArgs; +virCommandNonblockingFDs; +virCommandPreserveFD; +virCommandRun; +virCommandRunAsync; +virCommandSetErrorBuffer; +virCommandSetErrorFD; +virCommandSetInputBuffer; +virCommandSetInputFD; +virCommandSetOutputBuffer; +virCommandSetOutputFD; +virCommandSetPidFile; +virCommandSetPreExecHook; +virCommandSetWorkingDirectory; +virCommandToString; +virCommandTransferFD; +virCommandWait; +virCommandWriteArgLog; + + # conf.h virConfFree; virConfFreeValue; diff --git a/src/util/command.c b/src/util/command.c new file mode 100644 index 0000000..948a957 --- /dev/null +++ b/src/util/command.c @@ -0,0 +1,1138 @@ +/* + * command.c: 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 + * + */ + +#include <config.h> + +#include <poll.h> +#include <stdarg.h> +#include <stdbool.h> +#include <stdlib.h> +#include <sys/wait.h> + +#include "command.h" +#include "memory.h" +#include "virterror_internal.h" +#include "util.h" +#include "logging.h" +#include "files.h" +#include "buf.h" + +#define VIR_FROM_THIS VIR_FROM_NONE + +#define virCommandError(code, ...) \ + virReportErrorHelper(NULL, VIR_FROM_NONE, code, __FILE__, \ + __FUNCTION__, __LINE__, __VA_ARGS__) + +struct _virCommand { + int has_error; /* ENOMEM on allocation failure, -1 for anything else. */ + + char **args; + size_t nargs; + size_t maxargs; + + char **env; + size_t nenv; + size_t maxenv; + + char *pwd; + + /* XXX Use int[] if we ever need to support more than FD_SETSIZE fd's. */ + fd_set preserve; /* FDs to pass to child. */ + fd_set transfer; /* FDs to close in parent. */ + + unsigned int flags; + + char *inbuf; + char **outbuf; + char **errbuf; + + int infd; + int inpipe; + int outfd; + int errfd; + int *outfdptr; + int *errfdptr; + + virExecHook hook; + void *opaque; + + pid_t pid; + char *pidfile; +}; + +/* + * 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; + + FD_ZERO(&cmd->preserve); + FD_ZERO(&cmd->transfer); + cmd->infd = cmd->outfd = cmd->errfd = -1; + cmd->inpipe = -1; + cmd->pid = -1; + + virCommandAddArgSet(cmd, args); + + if (cmd->has_error) { + virCommandFree(cmd); + return NULL; + } + + return cmd; +} + +/* + * Create a new command with a NULL terminated + * list of args, starting with the binary to run + */ +virCommandPtr +virCommandNewArgList(const char *binary, ...) +{ + virCommandPtr cmd = virCommandNew(binary); + va_list list; + const char *arg; + + if (!cmd || cmd->has_error) + return NULL; + + va_start(list, binary); + while ((arg = va_arg(list, const char *)) != NULL) + virCommandAddArg(cmd, arg); + va_end(list); + return cmd; +} + + +/* + * Preserve the specified file descriptor in the child, instead of + * closing it. FD must not be one of the three standard streams. If + * transfer is true, then fd will be closed in the parent after a call + * to Run/RunAsync/Free, otherwise caller is still responsible for fd. + */ +static void +virCommandKeepFD(virCommandPtr cmd, int fd, bool transfer) +{ + if (!cmd) + return; + + if (fd <= STDERR_FILENO || FD_SETSIZE <= fd) { + if (!cmd->has_error) + cmd->has_error = -1; + VIR_DEBUG("cannot preserve %d", fd); + return; + } + + FD_SET(fd, &cmd->preserve); + if (transfer) + FD_SET(fd, &cmd->transfer); +} + +/* + * Preserve the specified file descriptor + * in the child, instead of closing it. + * The parent is still responsible for managing fd. + */ +void +virCommandPreserveFD(virCommandPtr cmd, int fd) +{ + return virCommandKeepFD(cmd, fd, false); +} + +/* + * Transfer the specified file descriptor + * to the child, instead of closing it. + * Close the fd in the parent during Run/RunAsync/Free. + */ +void +virCommandTransferFD(virCommandPtr cmd, int fd) +{ + return virCommandKeepFD(cmd, fd, true); +} + + +/* + * 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 = ENOMEM; + } +} + + +/* + * Remove all capabilities from the child + */ +void +virCommandClearCaps(virCommandPtr cmd) +{ + if (!cmd || cmd->has_error) + return; + + cmd->flags |= VIR_EXEC_CLEAR_CAPS; +} + +#if 0 /* XXX Enable if we have a need for capability management. */ + +/* + * Re-allow a specific capability + */ +void +virCommandAllowCap(virCommandPtr cmd, + int capability ATTRIBUTE_UNUSED) +{ + if (!cmd || cmd->has_error) + return; + + /* XXX ? */ +} + +#endif /* 0 */ + + +/* + * Daemonize the child process + */ +void +virCommandDaemonize(virCommandPtr cmd) +{ + if (!cmd || cmd->has_error) + return; + + cmd->flags |= VIR_EXEC_DAEMON; +} + +/* + * Set FDs created by virCommandSetOutputFD and virCommandSetErrorFD + * as non-blocking in the parent. + */ +void +virCommandNonblockingFDs(virCommandPtr cmd) +{ + if (!cmd || cmd->has_error) + return; + + cmd->flags |= VIR_EXEC_NONBLOCK; +} + +/* + * 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 = ENOMEM; + return; + } + + /* env plus trailing NULL */ + if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1) < 0) { + VIR_FREE(env); + cmd->has_error = ENOMEM; + return; + } + + cmd->env[cmd->nenv++] = env; +} + + +/* + * Add an environment variable to the child + * using a preformatted env string FOO=BAR + */ +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 = ENOMEM; + return; + } + + /* env plus trailing NULL */ + if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1) < 0) { + VIR_FREE(env); + cmd->has_error = ENOMEM; + return; + } + + cmd->env[cmd->nenv++] = env; +} + + +/* + * 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); +} + + +/* + * Set LC_ALL to C, and propagate other essential environment + * variables from the parent process. + */ +void +virCommandAddEnvPassCommon(virCommandPtr cmd) +{ + /* Attempt to Pre-allocate; allocation failure will be detected + * later during virCommandAdd*. */ + ignore_value(VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 9)); + + 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 = ENOMEM; + return; + } + + /* Arg plus trailing NULL. */ + if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1) < 0) { + VIR_FREE(arg); + cmd->has_error = ENOMEM; + return; + } + + cmd->args[cmd->nargs++] = arg; +} + + +/* + * Add a command line argument created by a printf-style format + */ +void +virCommandAddArgFormat(virCommandPtr cmd, const char *format, ...) +{ + char *arg; + va_list list; + + if (!cmd || cmd->has_error) + return; + + va_start(list, format); + if (virVasprintf(&arg, format, list) < 0) { + cmd->has_error = ENOMEM; + va_end(list); + return; + } + va_end(list); + + /* Arg plus trailing NULL. */ + if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, 1 + 1) < 0) { + VIR_FREE(arg); + cmd->has_error = ENOMEM; + return; + } + + cmd->args[cmd->nargs++] = arg; +} + +/* + * Add "NAME=VAL" as a single command line argument to the child + */ +void +virCommandAddArgPair(virCommandPtr cmd, const char *name, const char *val) +{ + virCommandAddArgFormat(cmd, "%s=%s", name, val); +} + +/* + * Add a NULL terminated list of args + */ +void +virCommandAddArgSet(virCommandPtr cmd, const char *const*vals) +{ + int narg = 0; + + if (!cmd || cmd->has_error) + return; + + while (vals[narg] != NULL) + narg++; + + /* narg plus trailing NULL. */ + if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, narg + 1) < 0) { + cmd->has_error = ENOMEM; + return; + } + + narg = 0; + while (vals[narg] != NULL) { + char *arg = strdup(vals[narg++]); + if (!arg) { + cmd->has_error = ENOMEM; + return; + } + cmd->args[cmd->nargs++] = arg; + } +} + +/* + * Add a NULL terminated list of args + */ +void +virCommandAddArgList(virCommandPtr cmd, ...) +{ + va_list list; + int narg = 0; + + if (!cmd || cmd->has_error) + return; + + va_start(list, cmd); + while (va_arg(list, const char *) != NULL) + narg++; + va_end(list); + + /* narg plus trailing NULL. */ + if (VIR_RESIZE_N(cmd->args, cmd->maxargs, cmd->nargs, narg + 1) < 0) { + cmd->has_error = ENOMEM; + return; + } + + va_start(list, cmd); + while (1) { + char *arg = va_arg(list, char *); + if (!arg) + break; + arg = strdup(arg); + if (!arg) { + cmd->has_error = ENOMEM; + va_end(list); + return; + } + cmd->args[cmd->nargs++] = arg; + } + va_end(list); +} + +/* + * Set the working directory of a non-daemon child process, rather + * than the parent's working directory. Daemons automatically get / + * without using this call. + */ +void +virCommandSetWorkingDirectory(virCommandPtr cmd, const char *pwd) +{ + if (!cmd || cmd->has_error) + return; + + if (cmd->pwd) { + cmd->has_error = -1; + VIR_DEBUG0("cannot set directory twice"); + } else { + cmd->pwd = strdup(pwd); + if (!cmd->pwd) + cmd->has_error = ENOMEM; + } +} + + +/* + * Feed the child's stdin from a string buffer + */ +void +virCommandSetInputBuffer(virCommandPtr cmd, const char *inbuf) +{ + if (!cmd || cmd->has_error) + return; + + if (cmd->infd != -1 || cmd->inbuf) { + cmd->has_error = -1; + VIR_DEBUG0("cannot specify input twice"); + return; + } + + cmd->inbuf = strdup(inbuf); + if (!cmd->inbuf) + cmd->has_error = ENOMEM; +} + + +/* + * Capture the child's stdout to a string buffer + */ +void +virCommandSetOutputBuffer(virCommandPtr cmd, char **outbuf) +{ + if (!cmd || cmd->has_error) + return; + + if (cmd->outfd != -1) { + cmd->has_error = -1; + VIR_DEBUG0("cannot specify output twice"); + return; + } + + cmd->outbuf = outbuf; + cmd->outfdptr = &cmd->outfd; +} + + +/* + * Capture the child's stderr to a string buffer + */ +void +virCommandSetErrorBuffer(virCommandPtr cmd, char **errbuf) +{ + if (!cmd || cmd->has_error) + return; + + if (cmd->errfd != -1) { + cmd->has_error = -1; + VIR_DEBUG0("cannot specify stderr twice"); + return; + } + + cmd->errbuf = errbuf; + cmd->errfdptr = &cmd->errfd; +} + + +/* + * Attach a file descriptor to the child's stdin + */ +void +virCommandSetInputFD(virCommandPtr cmd, int infd) +{ + if (!cmd || cmd->has_error) + return; + + if (infd < 0 || cmd->inbuf) { + cmd->has_error = -1; + VIR_DEBUG0("cannot specify input twice"); + return; + } + + cmd->infd = infd; +} + + +/* + * Attach a file descriptor to the child's stdout + */ +void +virCommandSetOutputFD(virCommandPtr cmd, int *outfd) +{ + if (!cmd || cmd->has_error) + return; + + if (!outfd || cmd->outfd != -1) { + cmd->has_error = -1; + VIR_DEBUG0("cannot specify output twice"); + return; + } + + cmd->outfdptr = outfd; +} + + +/* + * Attach a file descriptor to the child's stderr + */ +void +virCommandSetErrorFD(virCommandPtr cmd, int *errfd) +{ + if (!cmd || cmd->has_error) + return; + + if (!errfd || cmd->errfd != -1) { + cmd->has_error = -1; + VIR_DEBUG0("cannot specify stderr twice"); + return; + } + + cmd->errfdptr = errfd; +} + + +/* + * Run HOOK(OPAQUE) in the child as the last thing before changing + * directories, dropping capabilities, and executing the new process. + * Force the child to fail if HOOK does not return zero. + */ +void +virCommandSetPreExecHook(virCommandPtr cmd, virExecHook hook, void *opaque) +{ + if (!cmd || cmd->has_error) + return; + + cmd->hook = hook; + cmd->opaque = opaque; +} + + +/* + * Call after adding all arguments and environment settings, but before + * Run/RunAsync, to immediately output the environment and arguments of + * cmd to logfd. If virCommandRun cannot succeed (because of an + * out-of-memory condition while building cmd), nothing will be logged. + */ +void +virCommandWriteArgLog(virCommandPtr cmd, int logfd) +{ + int ioError = 0; + size_t i; + + /* Any errors will be reported later by virCommandRun, which means + * no command will be run, so there is nothing to log. */ + if (!cmd || cmd->has_error) + return; + + for (i = 0 ; i < cmd->nenv ; i++) { + if (safewrite(logfd, cmd->env[i], strlen(cmd->env[i])) < 0) + ioError = errno; + if (safewrite(logfd, " ", 1) < 0) + ioError = errno; + } + for (i = 0 ; i < cmd->nargs ; i++) { + if (safewrite(logfd, cmd->args[i], strlen(cmd->args[i])) < 0) + ioError = errno; + if (safewrite(logfd, i == cmd->nargs - 1 ? "\n" : " ", 1) < 0) + ioError = errno; + } + + if (ioError) { + char ebuf[1024]; + VIR_WARN("Unable to write command %s args to logfile: %s", + cmd->args[0], virStrerror(ioError, ebuf, sizeof ebuf)); + } +} + + +/* + * Call after adding all arguments and environment settings, but before + * Run/RunAsync, to return a string representation of the environment and + * arguments of cmd. If virCommandRun cannot succeed (because of an + * out-of-memory condition while building cmd), NULL will be returned. + * Caller is responsible for freeing the resulting string. + */ +char * +virCommandToString(virCommandPtr cmd) +{ + size_t i; + virBuffer buf = VIR_BUFFER_INITIALIZER; + + /* Cannot assume virCommandRun will be called; so report the error + * now. If virCommandRun is called, it will report the same error. */ + if (!cmd || cmd->has_error == -1) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); + return NULL; + } + if (cmd->has_error == ENOMEM) { + virReportOOMError(); + return NULL; + } + + for (i = 0; i < cmd->nenv; i++) { + virBufferAdd(&buf, cmd->env[i], strlen(cmd->env[i])); + virBufferAddChar(&buf, ' '); + } + virBufferAdd(&buf, cmd->args[0], strlen(cmd->args[0])); + for (i = 1; i < cmd->nargs; i++) { + virBufferAddChar(&buf, ' '); + virBufferAdd(&buf, cmd->args[i], strlen(cmd->args[i])); + } + + if (virBufferError(&buf)) { + virBufferFreeAndReset(&buf); + virReportOOMError(); + return NULL; + } + + return virBufferContentAndReset(&buf); +} + + +/* + * Manage input and output to the child process. + */ +static int +virCommandProcessIO(virCommandPtr cmd) +{ + int infd = -1, outfd = -1, errfd = -1; + size_t inlen = 0, outlen = 0, errlen = 0; + size_t inoff = 0; + + /* With an input buffer, feed data to child + * via pipe */ + if (cmd->inbuf) { + inlen = strlen(cmd->inbuf); + infd = cmd->inpipe; + } + + /* With out/err buffer, the outfd/errfd + * have been filled with an FD for us */ + if (cmd->outbuf) + outfd = cmd->outfd; + if (cmd->errbuf) + errfd = cmd->errfd; + + 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++; + } + + 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; + } + memcpy(*buf + *len, data, done); + *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) { + int tmpfd = infd; + if (VIR_CLOSE(infd) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + } + } + } + + } + } + + 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 == -1) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); + return -1; + } + if (cmd->has_error == ENOMEM) { + virReportOOMError(); + return -1; + } + + /* If we have an input buffer, we need + * a pipe to feed the data to the child */ + if (cmd->inbuf) { + if (pipe(infd) < 0) { + virReportSystemError(errno, "%s", + _("unable to open pipe")); + return -1; + } + cmd->infd = infd[0]; + cmd->inpipe = infd[1]; + } + + if (virCommandRunAsync(cmd, NULL) < 0) { + if (cmd->inbuf) { + int tmpfd = infd[0]; + if (VIR_CLOSE(infd[0]) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + tmpfd = infd[1]; + if (VIR_CLOSE(infd[1]) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + } + return -1; + } + + /* If caller hasn't requested capture of + * stdout/err, then capture it ourselves + * so we can log it + */ + if (!cmd->outbuf && + !cmd->outfdptr) { + cmd->outfd = -1; + cmd->outfdptr = &cmd->outfd; + cmd->outbuf = &outbuf; + } + if (!cmd->errbuf && + !cmd->errfdptr) { + cmd->errfd = -1; + cmd->errfdptr = &cmd->errfd; + cmd->errbuf = &errbuf; + } + + if (cmd->inbuf || cmd->outbuf || cmd->errbuf) + ret = virCommandProcessIO(cmd); + + 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) { + int tmpfd = infd[0]; + if (VIR_CLOSE(infd[0]) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + tmpfd = infd[1]; + if (VIR_CLOSE(infd[1]) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + } + if (cmd->outbuf == &outbuf) { + int tmpfd = cmd->outfd; + if (VIR_CLOSE(cmd->outfd) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + cmd->outfdptr = NULL; + cmd->outbuf = NULL; + } + if (cmd->errbuf == &errbuf) { + int tmpfd = cmd->errfd; + if (VIR_CLOSE(cmd->errfd) < 0) + VIR_DEBUG("ignoring failed close on fd %d", tmpfd); + cmd->errfdptr = NULL; + cmd->errbuf = NULL; + } + + return ret; +} + + +/* + * Perform all virCommand-specific actions, along with the user hook. + */ +static int +virCommandHook(void *data) +{ + virCommandPtr cmd = data; + int res = 0; + + if (cmd->hook) + res = cmd->hook(cmd->opaque); + if (res == 0 && cmd->pwd) { + VIR_DEBUG("Running child in %s", cmd->pwd); + res = chdir(cmd->pwd); + } + return res; +} + + +/* + * 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; + int i; + + if (!cmd || cmd->has_error == -1) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); + return -1; + } + if (cmd->has_error == ENOMEM) { + virReportOOMError(); + return -1; + } + + if (cmd->pid != -1) { + virCommandError(VIR_ERR_INTERNAL_ERROR, + _("command is already running as pid %d"), + cmd->pid); + return -1; + } + + if (cmd->pwd && (cmd->flags & VIR_EXEC_DAEMON)) { + virCommandError(VIR_ERR_INTERNAL_ERROR, + _("daemonized command cannot set working directory %s"), + cmd->pwd); + 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, + virCommandHook, + cmd, + cmd->pidfile); + + VIR_DEBUG("Command result %d, with PID %d", + ret, (int)cmd->pid); + + for (i = STDERR_FILENO + 1; i < FD_SETSIZE; i++) { + if (FD_ISSET(i, &cmd->transfer)) { + int tmpfd = i; + VIR_FORCE_CLOSE(tmpfd); + FD_CLR(i, &cmd->transfer); + } + } + + 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 == -1) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); + return -1; + } + if (cmd->has_error == ENOMEM) { + 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)); + return -1; + } + } else { + *exitstatus = status; + } + + return 0; +} + + +/* + * Release all resources + */ +void +virCommandFree(virCommandPtr cmd) +{ + int i; + if (!cmd) + return; + + for (i = STDERR_FILENO + 1; i < FD_SETSIZE; i++) { + if (FD_ISSET(i, &cmd->transfer)) { + int tmpfd = i; + VIR_FORCE_CLOSE(tmpfd); + } + } + + VIR_FORCE_CLOSE(cmd->outfd); + VIR_FORCE_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->pwd); + + 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..9b04e68 --- /dev/null +++ b/src/util/command.h @@ -0,0 +1,260 @@ +/* + * 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); + +/* + * Create a new command with a NULL terminated + * list of args, starting with the binary to run + */ +virCommandPtr virCommandNewArgList(const char *binary, ...) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_SENTINEL; + +/* All error report from these setup APIs is + * delayed until the Run/RunAsync methods + */ + +/* + * Preserve the specified file descriptor + * in the child, instead of closing it. + * The parent is still responsible for managing fd. + */ +void virCommandPreserveFD(virCommandPtr cmd, + int fd); + +/* + * Transfer the specified file descriptor + * to the child, instead of closing it. + * Close the fd in the parent during Run/RunAsync/Free. + */ +void virCommandTransferFD(virCommandPtr cmd, + int fd); + +/* + * 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); + +# if 0 +/* + * Re-allow a specific capability + */ +void virCommandAllowCap(virCommandPtr cmd, + int capability); +# endif + +/* + * Daemonize the child process + */ +void virCommandDaemonize(virCommandPtr cmd); + +/* + * Set FDs created by virCommandSetOutputFD and virCommandSetErrorFD + * as non-blocking in the parent. + */ +void virCommandNonblockingFDs(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); + +/* + * Add an environemnt variable to the child + * using a preformated env string FOO=BAR + */ +void virCommandAddEnvString(virCommandPtr cmd, + const char *str) ATTRIBUTE_NONNULL(2); +/* + * Pass an environment variable to the child + * using current process' value + */ +void virCommandAddEnvPass(virCommandPtr cmd, + const char *name) ATTRIBUTE_NONNULL(2); +/* + * 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) ATTRIBUTE_NONNULL(2); + +/* + * Add a command line argument created by a printf-style format + */ +void virCommandAddArgFormat(virCommandPtr cmd, + const char *format, ...) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 3); + +/* + * Add a command line argument to the child + */ +void virCommandAddArgPair(virCommandPtr cmd, + const char *name, + const char *val) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); +/* + * Add a NULL terminated array of args + */ +void virCommandAddArgSet(virCommandPtr cmd, + const char *const*vals) ATTRIBUTE_NONNULL(2); +/* + * Add a NULL terminated list of args + */ +void virCommandAddArgList(virCommandPtr cmd, + ... /* const char *arg, ..., NULL */) + ATTRIBUTE_SENTINEL; + +/* + * Set the working directory of a non-daemon child process, rather + * than the parent's working directory. Daemons automatically get / + * without using this call. + */ +void virCommandSetWorkingDirectory(virCommandPtr cmd, + const char *pwd) ATTRIBUTE_NONNULL(2); + +/* + * Feed the child's stdin from a string buffer. + * + * NB: Only works with virCommandRun() + */ +void virCommandSetInputBuffer(virCommandPtr cmd, + const char *inbuf) ATTRIBUTE_NONNULL(2); +/* + * Capture the child's stdout to a string buffer + * + * NB: Only works with virCommandRun() + */ +void virCommandSetOutputBuffer(virCommandPtr cmd, + char **outbuf) ATTRIBUTE_NONNULL(2); +/* + * Capture the child's stderr to a string buffer + * + * NB: Only works with virCommandRun() + */ +void virCommandSetErrorBuffer(virCommandPtr cmd, + char **errbuf) ATTRIBUTE_NONNULL(2); + +/* + * Set a file descriptor as the child's stdin + */ +void virCommandSetInputFD(virCommandPtr cmd, + int infd); +/* + * Set a file descriptor as the child's stdout + */ +void virCommandSetOutputFD(virCommandPtr cmd, + int *outfd) ATTRIBUTE_NONNULL(2); +/* + * Set a file descriptor as the child's stderr + */ +void virCommandSetErrorFD(virCommandPtr cmd, + int *errfd) ATTRIBUTE_NONNULL(2); + +/* + * A hook function to run between fork + exec + */ +void virCommandSetPreExecHook(virCommandPtr cmd, + virExecHook hook, + void *opaque) ATTRIBUTE_NONNULL(2); + +/* + * Call after adding all arguments and environment settings, but before + * Run/RunAsync, to immediately output the environment and arguments of + * cmd to logfd. If virCommandRun cannot succeed (because of an + * out-of-memory condition while building cmd), nothing will be logged. + */ +void virCommandWriteArgLog(virCommandPtr cmd, + int logfd); + +/* + * Call after adding all arguments and environment settings, but before + * Run/RunAsync, to return a string representation of the environment and + * arguments of cmd. If virCommandRun cannot succeed (because of an + * out-of-memory condition while building cmd), NULL will be returned. + * Caller is responsible for freeing the resulting string. + */ +char *virCommandToString(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; + +/* + * 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; + +/* + * 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; + +/* + * 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; + +/* + * Release all resources + */ +void virCommandFree(virCommandPtr cmd); + + +#endif /* __VIR_COMMAND_H__ */ diff --git a/tests/.gitignore b/tests/.gitignore index 8ad3e98..e3906f0 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -1,6 +1,10 @@ *.exe .deps .libs +commandhelper +commandhelper.log +commandhelper.pid +commandtest conftest esxutilstest eventtest diff --git a/tests/Makefile.am b/tests/Makefile.am index 77b6fb9..ff3f135 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -72,7 +72,8 @@ EXTRA_DIST = \ qemuhelpdata check_PROGRAMS = virshtest conftest sockettest \ - nodeinfotest qparamtest virbuftest + nodeinfotest qparamtest virbuftest \ + commandtest commandhelper if WITH_XEN check_PROGRAMS += xml2sexprtest sexpr2xmltest \ @@ -154,6 +155,7 @@ TESTS = virshtest \ qparamtest \ virbuftest \ sockettest \ + commandtest \ $(test_scripts) if WITH_XEN @@ -339,6 +341,20 @@ 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) + if WITH_SECDRIVER_SELINUX seclabeltest_SOURCES = \ seclabeltest.c 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..e1d6092 --- /dev/null +++ b/tests/commanddata/test11.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/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/test14.log b/tests/commanddata/test14.log new file mode 100644 index 0000000..1b59206 --- /dev/null +++ b/tests/commanddata/test14.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/test15.log b/tests/commanddata/test15.log new file mode 100644 index 0000000..f439a85 --- /dev/null +++ b/tests/commanddata/test15.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:.../commanddata diff --git a/tests/commanddata/test16.log b/tests/commanddata/test16.log new file mode 100644 index 0000000..2433a4d --- /dev/null +++ b/tests/commanddata/test16.log @@ -0,0 +1 @@ +A=B /bin/true C diff --git a/tests/commanddata/test2.log b/tests/commanddata/test2.log new file mode 100644 index 0000000..1b59206 --- /dev/null +++ b/tests/commanddata/test2.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/test3.log b/tests/commanddata/test3.log new file mode 100644 index 0000000..6bd7974 --- /dev/null +++ b/tests/commanddata/test3.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/test4.log b/tests/commanddata/test4.log new file mode 100644 index 0000000..1876685 --- /dev/null +++ b/tests/commanddata/test4.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/test5.log b/tests/commanddata/test5.log new file mode 100644 index 0000000..f745c3f --- /dev/null +++ b/tests/commanddata/test5.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/test6.log b/tests/commanddata/test6.log new file mode 100644 index 0000000..5394428 --- /dev/null +++ b/tests/commanddata/test6.log @@ -0,0 +1,6 @@ +ENV:DISPLAY=:0.0 +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..cdfe445 --- /dev/null +++ b/tests/commanddata/test7.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/test8.log b/tests/commanddata/test8.log new file mode 100644 index 0000000..87874fd --- /dev/null +++ b/tests/commanddata/test8.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/test9.log b/tests/commanddata/test9.log new file mode 100644 index 0000000..2607530 --- /dev/null +++ b/tests/commanddata/test9.log @@ -0,0 +1,18 @@ +ARG:-version +ARG:-log=bar.log +ARG:arg1 +ARG:arg2 +ARG:arg3 +ARG:arg4 +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..2ee9153 --- /dev/null +++ b/tests/commandhelper.c @@ -0,0 +1,137 @@ +/* + * commandhelper.c: Auxiliary program for commandtest + * + * 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 + */ + +#include <config.h> + +#include <stdio.h> +#include <unistd.h> +#include <stdlib.h> +#include <fcntl.h> +#include <string.h> + +#include "internal.h" +#include "util.h" +#include "memory.h" +#include "files.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++; + 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++) { + int f; + int closed; + if (i == fileno(log)) + continue; + closed = fcntl(i, F_GETFD, &f) == -1 && + errno == EBADF; + if (!closed) + fprintf(log, "FD:%d\n", i); + } + + fprintf(log, "DAEMON:%s\n", getppid() == 1 ? "yes" : "no"); + char cwd[1024]; + getcwd(cwd, sizeof(cwd)); + if (strlen(cwd) > strlen("/commanddata") && + STREQ(cwd + strlen(cwd) - strlen("/commanddata"), "/commanddata")) + strcpy(cwd, ".../commanddata"); + fprintf(log, "CWD:%s\n", cwd); + + VIR_FORCE_FCLOSE(log); + + 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..46d6ae5 --- /dev/null +++ b/tests/commandtest.c @@ -0,0 +1,630 @@ +/* + * commandtest.c: Test the libCommand API + * + * 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 + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <unistd.h> +#include <signal.h> +#include <sys/stat.h> +#include <fcntl.h> + +#include "testutils.h" +#include "internal.h" +#include "nodeinfo.h" +#include "util.h" +#include "memory.h" +#include "command.h" +#include "files.h" + +#ifndef __linux__ + +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 + * No slot for return status must log error. + */ +static int test0(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd; + char *log; + int ret = -1; + + free(virtTestLogContentAndReset()); + cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); + if (virCommandRun(cmd, NULL) == 0) + goto cleanup; + if ((log = virtTestLogContentAndReset()) == NULL) + goto cleanup; + if (strstr(log, ": error :") == NULL) + goto cleanup; + virResetLastError(); + ret = 0; + +cleanup: + virCommandFree(cmd); + return ret; +} + +/* + * Run program, no args, inherit all ENV, keep CWD. + * Only stdin/out/err open + * Capturing return status must not log error. + */ +static int test1(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd; + int ret = -1; + int status; + + cmd = virCommandNew(abs_builddir "/commandhelper-doesnotexist"); + if (virCommandRun(cmd, &status) < 0) + goto cleanup; + if (status == 0) + goto cleanup; + ret = 0; + +cleanup: + virCommandFree(cmd); + return ret; +} + +/* + * Run program, no args, inherit all ENV, keep CWD. + * Only stdin/out/err open + */ +static int test2(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("test2"); +} + +/* + * Run program, no args, inherit all ENV, keep CWD. + * stdin/out/err + two extra FD open + */ +static int test3(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); + + virCommandPreserveFD(cmd, newfd1); + virCommandTransferFD(cmd, newfd3); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + if (fcntl(newfd1, F_GETFL) < 0 || + fcntl(newfd2, F_GETFL) < 0 || + fcntl(newfd3, F_GETFL) >= 0) { + puts("fds in wrong state"); + return -1; + } + + virCommandFree(cmd); + VIR_FORCE_CLOSE(newfd1); + VIR_FORCE_CLOSE(newfd2); + + return checkoutput("test3"); +} + + +/* + * Run program, no args, inherit all ENV, CWD is / + * Only stdin/out/err open. + * Daemonized + */ +static int test4(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("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"); + + virCommandAddEnvPassCommon(cmd); + + 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"); + + 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"); + + 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("test7"); +} + +/* + * Run program, no args, inherit filtered ENV, keep CWD. + * Only stdin/out/err open + */ +static int test8(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("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[] = { "arg1", "arg2", NULL }; + + virCommandAddArg(cmd, "-version"); + virCommandAddArgPair(cmd, "-log", "bar.log"); + virCommandAddArgSet(cmd, args); + virCommandAddArgList(cmd, "arg3", "arg4", NULL); + + 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) { + 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("test10"); +} + +/* + * Run program, some args, inherit all ENV, keep CWD. + * Only stdin/out/err open + */ +static int test11(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("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"); + + 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("test12"); +} + +/* + * 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"; + 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("test13") < 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 test14(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("test14") < 0) + goto cleanup; + + ret = 0; + +cleanup: + VIR_FREE(outactual); + VIR_FREE(erractual); + return ret; +} + + +/* + * Run program, no args, inherit all ENV, change CWD. + * Only stdin/out/err open + */ +static int test15(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); + + virCommandSetWorkingDirectory(cmd, abs_builddir "/commanddata"); + + if (virCommandRun(cmd, NULL) < 0) { + virErrorPtr err = virGetLastError(); + printf("Cannot run child %s\n", err->message); + return -1; + } + + virCommandFree(cmd); + + return checkoutput("test15"); +} + +/* + * Don't run program; rather, log what would be run. + */ +static int test16(const void *unused ATTRIBUTE_UNUSED) { + virCommandPtr cmd = virCommandNew("/bin/true"); + char *outactual = NULL; + const char *outexpect = "A=B /bin/true C"; + int ret = -1; + int fd = -1; + + virCommandAddEnvPair(cmd, "A", "B"); + virCommandAddArg(cmd, "C"); + + if ((outactual = virCommandToString(cmd)) == NULL) { + virErrorPtr err = virGetLastError(); + printf("Cannot convert to string: %s\n", err->message); + return -1; + } + if ((fd = open(abs_builddir "/commandhelper.log", + O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0) { + printf("Cannot open log file: %s\n", strerror (errno)); + goto cleanup; + } + virCommandWriteArgLog(cmd, fd); + if (VIR_CLOSE(fd) < 0) { + printf("Cannot close log file: %s\n", strerror (errno)); + goto cleanup; + } + + virCommandFree(cmd); + + if (checkoutput("test16") < 0) + goto cleanup; + + if (!STREQ(outactual, outexpect)) { + virtTestDifference(stderr, outactual, outexpect); + goto cleanup; + } + ret = 0; + +cleanup: + VIR_FORCE_CLOSE(fd); + VIR_FREE(outactual); + 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); + + virInitialize(); + + const char *const newenv[] = { + "PATH=/usr/bin:/bin", + "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); + DO_TEST(test14); + DO_TEST(test15); + DO_TEST(test16); + + return(ret==0 ? EXIT_SUCCESS : EXIT_FAILURE); +} + +#endif /* __linux__ */ + +VIRT_TEST_MAIN(mymain) -- 1.7.3.2

From: Daniel P. Berrange <berrange@redhat.com> * docs/internals/command.html.in: New file. * docs/Makefile.am: Build new docs. * docs/subsite.xsl: New glue file. * docs/internals.html.in, docs/sitemap.html.in: Update glue. --- v2: document commands added in v2. docs/Makefile.am | 11 +- docs/internals.html.in | 9 + docs/internals/command.html.in | 550 ++++++++++++++++++++++++++++++++++++++++ docs/sitemap.html.in | 4 + docs/subsite.xsl | 25 ++ 5 files changed, 598 insertions(+), 1 deletions(-) create mode 100644 docs/internals/command.html.in create mode 100644 docs/subsite.xsl diff --git a/docs/Makefile.am b/docs/Makefile.am index b28e04e..ce0b391 100644 --- a/docs/Makefile.am +++ b/docs/Makefile.am @@ -60,7 +60,8 @@ gif = \ architecture.gif \ node.gif -dot_html_in = $(notdir $(wildcard $(srcdir)/*.html.in)) todo.html.in +dot_html_in = $(notdir $(wildcard $(srcdir)/*.html.in)) todo.html.in \ + $(patsubst $(srcdir)/%,%,$(wildcard $(srcdir)/internals/*.html.in)) dot_html = $(dot_html_in:%.html.in=%.html) patches = $(wildcard api_extension/*.patch) @@ -113,6 +114,14 @@ todo: %.png: %.fig convert -rotate 90 $< $@ +internals/%.html.tmp: internals/%.html.in subsite.xsl page.xsl sitemap.html.in + @if [ -x $(XSLTPROC) ] ; then \ + echo "Generating $@"; \ + name=`echo $@ | sed -e 's/.tmp//'`; \ + $(XSLTPROC) --stringparam pagename $$name --nonet --html \ + $(top_srcdir)/docs/subsite.xsl $< > $@ \ + || { rm $@ && exit 1; }; fi + %.html.tmp: %.html.in site.xsl page.xsl sitemap.html.in @if [ -x $(XSLTPROC) ] ; then \ echo "Generating $@"; \ diff --git a/docs/internals.html.in b/docs/internals.html.in index d39098e..dc88eab 100644 --- a/docs/internals.html.in +++ b/docs/internals.html.in @@ -7,5 +7,14 @@ internals, adding new public APIs, new hypervisor drivers or extending the libvirtd daemon code. </p> + + <ul> + <li>Introduction to basic rules and guidelines for <a href="hacking.html">hacking<a> + on libvirt code</li> + <li>Guide to adding <a href="api_extension.html">public APIs<a></li> + <li>Approach for <a href="internals/command.html">spawning commands</a> from + libvirt driver code</li> + </ul> + </body> </html> diff --git a/docs/internals/command.html.in b/docs/internals/command.html.in new file mode 100644 index 0000000..c4fa800 --- /dev/null +++ b/docs/internals/command.html.in @@ -0,0 +1,550 @@ +<html> + <body> + <h1>Spawning processes / commands from libvirt drivers</h1> + + <ul id="toc"></ul> + + <p> + This page describes the usage of libvirt APIs for + spawning processes / commands from libvirt drivers. + All code is required to use these APIs + </p> + + <h2><a name="posix">Problems with standard POSIX APIs</a></h2> + + <p> + The POSIX specification includes a number of APIs for + spawning processes / commands, but they suffer from + a number of flaws + </p> + + <ul> + <li><code>fork+exec</code>: 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. + Furthermore, it is not portable to mingw. + </li> + <li><code>system</code>: 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. + </li> + <li><code>popen</code>: Inherits the flaws of + <code>system</code>, and has no option for bi-directional + communication. + </li> + <li><code>posix_spawn</code>: 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.</li> + </ul> + + <p> + Due to the problems mentioned with each of these, + libvirt driver code <strong>must not use</strong> 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. + </p> + + <p> + 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. + </p> + + <h2><a name="api">The libvirt command execution API</a></h2> + + <p> + 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 against. This + code is provided in the <code>src/util/command.h</code> + header which can be imported using <code>#include "command.h"</code> + </p> + + <h3><a name="initial">Defining commands in libvirt</a></h3> + + <p> + 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 <code>$PATH</code> + environment variable. + </p> + +<pre> + virCommandPtr cmd = virCommandNew("/usr/bin/dnsmasq"); +</pre> + + <p> + There is no need to check for allocation failure after + <code>virCommandNew</code>. This will be detected and + reported at a later time. + </p> + + <h3><a name="args">Adding arguments to the command</a></h3> + + <p> + There are a number of APIs for adding arguments to a + command. To add a direct string arg + </p> + +<pre> + virCommandAddArg(cmd, "-strict-order"); +</pre> + + <p> + If an argument takes an attached value of the form + <code>-arg=val</code>, then this can be done using + </p> + +<pre> + virCommandAddArgPair(cmd, "--conf-file", "/etc/dnsmasq.conf"); +</pre> + + <p> + If an argument needs to be formatted as if by + <code>printf</code>: + </p> + +<pre> + virCommandAddArgFormat(cmd, "%d", count); +</pre> + + <p> + To add a entire NULL terminated array of arguments in one go, + there are two options. + </p> + +<pre> + const char *const args[] = { + "--strict-order", "--except-interface", "lo", NULL + }; + virCommandAddArgSet(cmd, args); + virCommandAddArgList(cmd, "--domain", "localdomain", NULL); +</pre> + + <p> + This can also be done at the time of initial construction of + the <code>virCommandPtr</code> object: + </p> + +<pre> + const char *const args[] = { + "/usr/bin/dnsmasq", + "--strict-order", "--except-interface", + "lo", "--domain", "localdomain", NULL + }; + virCommandPtr cmd1 = virCommandNewArgs(cmd, args); + virCommandPtr cmd2 = virCommandNewArgList("/usr/bin/dnsmasq", + "--domain", "localdomain", NULL); +</pre> + + <h3><a name="env">Setting up the environment</a></h3> + + <p> + 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. + </p> + +<pre> + virCommandAddEnvPassCommon(cmd); +</pre> + + <p> + This has now set up a clean environment for the child, passing + through <code>PATH</code>, <code>LD_PRELOAD</code>, + <code>LD_LIBRARY_PATH</code>, <code>HOME</code>, + <code>USER</code>, <code>LOGNAME</code> and <code>TMPDIR</code>. + Furthermore it will explicitly set <code>LC_ALL=C</code> to + avoid unexpected localization of command output. Further + variables can be passed through from parent explicitly: + </p> + +<pre> + virCommandAddEnvPass(cmd, "DISPLAY"); + virCommandAddEnvPass(cmd, "XAUTHORITY"); +</pre> + + <p> + To define an environment variable in the child with an + separate key / value: + </p> + +<pre> + virCommandAddEnvPair(cmd, "TERM", "xterm"); +</pre> + + <p> + If the key/value pair is pre-formatted in the right + format, it can be set directly + </p> + +<pre> + virCommandAddEnvString(cmd, "TERM=xterm"); +</pre> + + <h3><a name="misc">Miscellaneous other options</a></h3> + + <p> + Normally the spawned command will retain the current + process and process group 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: + </p> + +<pre> + virCommandDaemonize(cmd); +</pre> + + <p> + 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 of the real + command is required then a pidfile can be requested + </p> + +<pre> + virCommandSetPidFile(cmd, "/var/run/dnsmasq.pid"); +</pre> + + <p> + This PID file is guaranteed to be written before + the intermediate process exits. + </p> + + <h3><a name="privs">Reducing command privileges</a></h3> + + <p> + 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 + </p> + +<pre> + virCommandClearCaps(cmd); +</pre> + + <h3><a name="fds">Managing file handles</a></h3> + + <p> + To prevent unintended resource leaks to child processes, the + child defaults to closing all open file handles, and setting + stdin/out/err to <code>/dev/null</code>. It is possible to + allow an open file handle to be passed into the child, while + controlling whether that handle remains open in the parent or + guaranteeing that the handle will be closed in the parent after + either virCommandRun or virCommandFree. + </p> + +<pre> + int sharedfd = open("cmd.log", "w+"); + int childfd = open("conf.txt", "r"); + virCommandPreserveFD(cmd, sharedfd); + virCommandTransferFD(cmd, childfd); + if (VIR_CLOSE(sharedfd) < 0) + goto cleanup; +</pre> + + <p> + With this, both file descriptors sharedfd and childfd in the + current process remain open as the same file descriptors in the + child. Meanwhile, after the child is spawned, sharedfd remains + open in the parent, while childfd is closed. For stdin/out/err + it is usually necessary to map a file handle. To attach file + descriptor 7 in the current process to stdin in the child: + </p> + +<pre> + virCommandSetInputFD(cmd, 7); +</pre> + + <p> + Equivalently to redirect stdout or stderr in the child, + pass in a pointer to the desired handle + </p> + +<pre> + int outfd = open("out.log", "w+"); + int errfd = open("err.log", "w+"); + virCommandSetOutputFD(cmd, &outfd); + virCommandSetErrorFD(cmd, &errfd); +</pre> + + <p> + Alternatively it is possible to request that a pipe be + created to fetch stdout/err in the parent, by initializing + the FD to -1. + </p> + +<pre> + int outfd = -1; + int errfd = -1 + virCommandSetOutputFD(cmd, &outfd); + virCommandSetErrorFD(cmd, &errfd); +</pre> + + <p> + Once the command is running, <code>outfd</code> + and <code>errfd</code> will be initialized with + valid file handles that can be read from. It is + permissible to pass the same pointer for both outfd + and errfd, in which case both standard streams in + the child will share the same fd in the parent. + </p> + + <p> + Normally, file descriptors opened to collect output from a child + process perform blocking I/O, but the parent process can request + non-blocking mode: + </p> + +<pre> + virCommandNonblockingFDs(cmd); +</pre> + + <h3><a name="buffers">Feeding & capturing strings to/from the child</a></h3> + + <p> + Often dealing with file handles for stdin/out/err + is unnecessarily complex. It is possible to specify + a string buffer to act as the data source for the + child's stdin, if there are no embedded NUL bytes, + and if the command will be run with virCommandRun: + </p> + +<pre> + const char *input = "Hello World\n"; + virCommandSetInputBuffer(cmd, input); +</pre> + + <p> + Similarly it is possible to request that the child's + stdout/err be redirected into a string buffer, if the + output is not expected to contain NUL bytes, and if + the command will be run with virCommandRun: + </p> + +<pre> + char *output = NULL, *errors = NULL; + virCommandSetOutputBuffer(cmd, &output); + virCommandSetErrorBuffer(cmd, &errors); +</pre> + + <p> + Once the command has finished executing, these buffers + will contain the output. It is the callers responsibility + to free these buffers. + </p> + + <h3><a name="directory">Setting working directory</a></h3> + + <p> + Daemonized commands are always run with "/" as the current + working directory. All other commands default to running in the + same working directory as the parent process, but an alternate + directory can be specified: + </p> + +<pre> + virCommandSetWorkingDirectory(cmd, LOCALSTATEDIR); +</pre> + + <h3><a name="hooks">Any additional hooks</a></h3> + + <p> + If anything else is needed, it is possible to request a hook + function that is called in the child after the fork, as the + last thing before changing directories, dropping capabilities, + and executing the new process. If hook(opaque) returns + non-zero, then the child process will not be run. + </p> + +<pre> + virCommandSetPreExecHook(cmd, hook, opaque); +</pre> + + <h3><a name="logging">Logging commands</a></h3> + + <p> + Sometimes, it is desirable to log what command will be run, or + even to use virCommand solely for creation of a single + consolidated string without running anything. + </p> + +<pre> + int logfd = ...; + char *timestamp = virTimestamp(); + char *string = NULL; + + dprintf(logfd, "%s: ", timestamp); + VIR_FREE(timestamp); + virCommandWriteArgLog(cmd, logfd); + + string = virCommandToString(cmd); + if (string) + VIR_DEBUG("about to run %s", string); + VIR_FREE(string); + if (virCommandRun(cmd) < 0) + return -1; +</pre> + + <h3><a name="sync">Running commands synchronously</a></h3> + + <p> + For most commands, the desired behaviour is to spawn + the command, wait for it to complete & exit and then + check that its exit status is zero + </p> + +<pre> + if (virCommandRun(cmd, NULL) < 0) + return -1; +</pre> + + <p> + <strong>Note:</strong> if the command has been daemonized + this will only block & wait for the intermediate process, + not the real command. <code>virCommandRun</code> will + report on any errors that have occured upon this point + with all previous API 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 <code>virCommandRun</code> + raise the error + </p> + +<pre> + int status; + if (virCommandRun(cmd, &status) < 0) + return -1; + + if (WEXITSTATUS(status) ...) { + ...do stuff... + } +</pre> + + <h3><a name="async">Running commands asynchronously</a></h3> + + <p> + In certain complex scenarios, particularly special + I/O handling is required for the child's stdin/err/out + it will be necessary to run the command asynchronously + and wait for completion separately. + </p> + +<pre> + 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.. + } +</pre> + + <p> + As with <code>virCommandRun</code>, the <code>status</code> + arg for <code>virCommandWait</code> can be omitted, in which + case it will validate that exit status is zero and raise an + error if not. + </p> + + + <h3><a name="release">Releasing resources</a></h3> + + <p> + Once the command has been executed, or if execution + has been abandoned, it is necessary to release + resources associated with the <code>virCommandPtr</code> + object. This is done with: + </p> + +<pre> + virCommandFree(cmd); +</pre> + + <p> + There is no need to check if <code>cmd</code> is NULL + before calling <code>virCommandFree</code>. This scenario + is handled automatically. If the command is still running, + it will be forcably killed and cleaned up (via waitpid). + </p> + + <h2><a name="example">Complete examples</a></h2> + + <p> + This shows a complete example usage of the APIs roughly + using the libvirt source src/util/hooks.c + </p> + +<pre> +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); + + virCommandAddArgList(cmd, id, opstr, subopstr, extra, NULL); + + virCommandSetInputBuffer(cmd, input); + + ret = virCommandRun(cmd, NULL); + + virCommandFree(cmd); + + return ret; +} +</pre> + + <p> + 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 <code>virCommandRun</code> + method, and the exit status from this is returned to + the caller to handle as desired. + </p> + + </body> +</html> diff --git a/docs/sitemap.html.in b/docs/sitemap.html.in index 692da29..7db59a1 100644 --- a/docs/sitemap.html.in +++ b/docs/sitemap.html.in @@ -260,6 +260,10 @@ <a href="api_extension.html">API extensions</a> <span>Adding new public libvirt APIs</span> </li> + <li> + <a href="internals/command.html">Spawning commands</a> + <span>Spawning commands from libvirt driver code</span> + </li> </ul> </li> <li> diff --git a/docs/subsite.xsl b/docs/subsite.xsl new file mode 100644 index 0000000..108d0d8 --- /dev/null +++ b/docs/subsite.xsl @@ -0,0 +1,25 @@ +<?xml version="1.0"?> +<xsl:stylesheet + xmlns:xsl="http://www.w3.org/1999/XSL/Transform" + xmlns:exsl="http://exslt.org/common" + exclude-result-prefixes="xsl exsl" + version="1.0"> + + <xsl:import href="page.xsl"/> + + <xsl:output + method="xml" + encoding="UTF-8" + indent="yes" + doctype-public="-//W3C//DTD XHTML 1.0 Strict//EN" + doctype-system="http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"/> + + <xsl:variable name="href_base" select="'../'"/> + + <xsl:template match="/"> + <xsl:apply-templates select="." mode="page"> + <xsl:with-param name="pagename" select="$pagename"/> + </xsl:apply-templates> + </xsl:template> + +</xsl:stylesheet> -- 1.7.3.2

From: Daniel P. Berrange <berrange@redhat.com> This proof of concept shows how two existing uses of virExec and virRun can be ported to the new virCommand APIs, and how much simpler the code becomes --- v2: not much change src/util/hooks.c | 216 +++------------------------------------------------ src/util/iptables.c | 73 +++-------------- 2 files changed, 23 insertions(+), 266 deletions(-) diff --git a/src/util/hooks.c b/src/util/hooks.c index 8e24564..04ca6fa 100644 --- a/src/util/hooks.c +++ b/src/util/hooks.c @@ -38,6 +38,7 @@ #include "memory.h" #include "files.h" #include "configmake.h" +#include "command.h" #define VIR_FROM_THIS VIR_FROM_HOOK @@ -190,36 +191,15 @@ virHookPresent(int driver) { * Returns: 0 if the execution succeeded, 1 if the script was not found or * invalid parameters, and -1 if script returned an error */ -#ifdef WIN32 -int -virHookCall(int driver ATTRIBUTE_UNUSED, - const char *id ATTRIBUTE_UNUSED, - int op ATTRIBUTE_UNUSED, - int sub_op ATTRIBUTE_UNUSED, - const char *extra ATTRIBUTE_UNUSED, - const char *input ATTRIBUTE_UNUSED) { - virReportSystemError(ENOSYS, "%s", - _("spawning hooks not supported on this platform")); - return -1; -} -#else int virHookCall(int driver, const char *id, int op, int sub_op, const char *extra, const char *input) { - int ret, waitret, exitstatus, i; + int ret; char *path; - int argc = 0, arga = 0; - const char **argv = NULL; - int envc = 0, enva = 0; - const char **env = NULL; + virCommandPtr cmd; const char *drvstr; const char *opstr; const char *subopstr; - pid_t pid; - int outfd = -1, errfd = -1; - int pipefd[2] = { -1, -1}; - char *outbuf = NULL; - char *errbuf = NULL; if ((driver < VIR_HOOK_DRIVER_DAEMON) || (driver >= VIR_HOOK_DRIVER_LAST)) @@ -271,192 +251,18 @@ virHookCall(int driver, const char *id, int op, int sub_op, const char *extra, return(-1); } - /* - * Convenience macros borrowed from qemudBuildCommandLine() - */ -# define ADD_ARG_SPACE \ - do { \ - if (argc == arga) { \ - arga += 10; \ - if (VIR_REALLOC_N(argv, arga) < 0) \ - goto no_memory; \ - } \ - } while (0) - -# define ADD_ARG(thisarg) \ - do { \ - ADD_ARG_SPACE; \ - argv[argc++] = thisarg; \ - } while (0) - -# define ADD_ARG_LIT(thisarg) \ - do { \ - ADD_ARG_SPACE; \ - if ((argv[argc++] = strdup(thisarg)) == NULL) \ - goto no_memory; \ - } while (0) - -# define ADD_ENV_SPACE \ - do { \ - if (envc == enva) { \ - enva += 10; \ - if (VIR_REALLOC_N(env, enva) < 0) \ - goto no_memory; \ - } \ - } while (0) - -# define ADD_ENV(thisarg) \ - do { \ - ADD_ENV_SPACE; \ - env[envc++] = thisarg; \ - } while (0) - -# define ADD_ENV_LIT(thisarg) \ - do { \ - ADD_ENV_SPACE; \ - if ((env[envc++] = strdup(thisarg)) == NULL) \ - goto no_memory; \ - } while (0) - -# define ADD_ENV_PAIR(envname, val) \ - do { \ - char *envval; \ - ADD_ENV_SPACE; \ - if (virAsprintf(&envval, "%s=%s", envname, val) < 0) \ - goto no_memory; \ - env[envc++] = envval; \ - } while (0) - -# define ADD_ENV_COPY(envname) \ - do { \ - char *val = getenv(envname); \ - if (val != NULL) { \ - ADD_ENV_PAIR(envname, val); \ - } \ - } while (0) - - ADD_ENV_LIT("LC_ALL=C"); - - ADD_ENV_COPY("LD_PRELOAD"); - ADD_ENV_COPY("LD_LIBRARY_PATH"); - ADD_ENV_COPY("PATH"); - ADD_ENV_COPY("HOME"); - ADD_ENV_COPY("USER"); - ADD_ENV_COPY("LOGNAME"); - ADD_ENV_COPY("TMPDIR"); - ADD_ENV(NULL); - - ADD_ARG_LIT(path); - ADD_ARG_LIT(id); - ADD_ARG_LIT(opstr); - ADD_ARG_LIT(subopstr); - - ADD_ARG_LIT(extra); - ADD_ARG(NULL); - - /* pass any optional input on the script stdin */ - if (input != NULL) { - if (pipe(pipefd) < -1) { - virReportSystemError(errno, "%s", - _("unable to create pipe for hook input")); - ret = 1; - goto cleanup; - } - if (safewrite(pipefd[1], input, strlen(input)) < 0) { - virReportSystemError(errno, "%s", - _("unable to write to pipe for hook input")); - ret = 1; - goto cleanup; - } - ret = virExec(argv, env, NULL, &pid, pipefd[0], &outfd, &errfd, - VIR_EXEC_NONE | VIR_EXEC_NONBLOCK); - if (VIR_CLOSE(pipefd[1]) < 0) { - virReportSystemError(errno, "%s", - _("unable to close pipe for hook input")); - } - } else { - ret = virExec(argv, env, NULL, &pid, -1, &outfd, &errfd, - VIR_EXEC_NONE | VIR_EXEC_NONBLOCK); - } - if (ret < 0) { - virHookReportError(VIR_ERR_HOOK_SCRIPT_FAILED, - _("Failed to execute %s hook script"), - path); - ret = 1; - goto cleanup; - } - - /* - * we are interested in the error log if any and make sure the - * script doesn't block on stdout/stderr descriptors being full - * stdout can be useful for debug too. - */ - if (virPipeReadUntilEOF(outfd, errfd, &outbuf, &errbuf) < 0) { - virReportSystemError(errno, _("cannot wait for '%s'"), path); - while (waitpid(pid, &exitstatus, 0) == -1 && errno == EINTR) - ; - ret = 1; - goto cleanup; - } + cmd = virCommandNewArgList(path, id, opstr, subopstr, extra, NULL); - if (outbuf) - VIR_DEBUG("Command stdout: %s", outbuf); - if (errbuf) - VIR_DEBUG("Command stderr: %s", errbuf); - - while ((waitret = waitpid(pid, &exitstatus, 0) == -1) && - (errno == EINTR)); - if (waitret == -1) { - virReportSystemError(errno, _("Failed to wait for '%s'"), path); - ret = 1; - goto cleanup; - } - if (exitstatus != 0) { - virHookReportError(VIR_ERR_HOOK_SCRIPT_FAILED, - _("Hook script %s %s failed with error code %d:%s"), - path, drvstr, exitstatus, errbuf); - ret = -1; - } + virCommandAddEnvPassCommon(cmd); -cleanup: - if (VIR_CLOSE(pipefd[0]) < 0) { - virReportSystemError(errno, "%s", - _("unable to close pipe for hook input")); - ret = 1; - } - if (VIR_CLOSE(pipefd[1]) < 0) { - virReportSystemError(errno, "%s", - _("unable to close pipe for hook input")); - ret = 1; - } - if (argv) { - for (i = 0 ; i < argc ; i++) - VIR_FREE((argv)[i]); - VIR_FREE(argv); - } - if (env) { - for (i = 0 ; i < envc ; i++) - VIR_FREE((env)[i]); - VIR_FREE(env); - } - VIR_FREE(outbuf); - VIR_FREE(errbuf); - VIR_FREE(path); + if (input) + virCommandSetInputBuffer(cmd, input); - return(ret); + ret = virCommandRun(cmd, NULL); -no_memory: - virReportOOMError(); + virCommandFree(cmd); - goto cleanup; + VIR_FREE(path); -# undef ADD_ARG -# undef ADD_ARG_LIT -# undef ADD_ARG_SPACE -# undef ADD_USBDISK -# undef ADD_ENV -# undef ADD_ENV_COPY -# undef ADD_ENV_LIT -# undef ADD_ENV_SPACE + return ret; } -#endif diff --git a/src/util/iptables.c b/src/util/iptables.c index fe78d1c..effd81b 100644 --- a/src/util/iptables.c +++ b/src/util/iptables.c @@ -39,7 +39,7 @@ #include "internal.h" #include "iptables.h" -#include "util.h" +#include "command.h" #include "memory.h" #include "virterror_internal.h" #include "logging.h" @@ -102,72 +102,23 @@ static int ATTRIBUTE_SENTINEL iptablesAddRemoveRule(iptRules *rules, int action, const char *arg, ...) { va_list args; - int retval = ENOMEM; - const char **argv; + int ret; + virCommandPtr cmd; const char *s; - int n; - - n = 1 + /* /sbin/iptables */ - 2 + /* --table foo */ - 2 + /* --insert bar */ - 1; /* arg */ - - va_start(args, arg); - while (va_arg(args, const char *)) - n++; - - va_end(args); - - if (VIR_ALLOC_N(argv, n + 1) < 0) - goto error; - - n = 0; - - if (!(argv[n++] = strdup(IPTABLES_PATH))) - goto error; - - if (!(argv[n++] = strdup("--table"))) - goto error; - - if (!(argv[n++] = strdup(rules->table))) - goto error; - - if (!(argv[n++] = strdup(action == ADD ? "--insert" : "--delete"))) - goto error; - - if (!(argv[n++] = strdup(rules->chain))) - goto error; - if (!(argv[n++] = strdup(arg))) - goto error; + cmd = virCommandNew(IPTABLES_PATH); + virCommandAddArgList(cmd, "--table", rules->table, + action == ADD ? "--insert" : "--delete", + rules->chain, arg, NULL); va_start(args, arg); - - while ((s = va_arg(args, const char *))) { - if (!(argv[n++] = strdup(s))) { - va_end(args); - goto error; - } - } - + while ((s = va_arg(args, const char *))) + virCommandAddArg(cmd, s); va_end(args); - if (virRun(argv, NULL) < 0) { - retval = errno; - goto error; - } - - retval = 0; - - error: - if (argv) { - n = 0; - while (argv[n]) - VIR_FREE(argv[n++]); - VIR_FREE(argv); - } - - return retval; + ret = virCommandRun(cmd, NULL); + virCommandFree(cmd); + return ret; } /** -- 1.7.3.2

* src/qemu/qemu_conf.c (qemudExtractVersionInfo): Check for file before executing it here, rather than in callers. (qemudBuildCommandLine): Rewrite with virCommand. * src/qemu/qemu_conf.h (qemudBuildCommandLine): Update signature. * src/qemu/qemu_driver.c (qemuAssignPCIAddresses) (qemudStartVMDaemon, qemuDomainXMLToNative): Adjust callers. --- v2: new patch, taking most of the ideas from https://www.redhat.com/archives/libvir-list/2010-November/msg00979.html but making it better by using virCommand improvements src/qemu/qemu_conf.c | 710 ++++++++++++++++++---------------------------- src/qemu/qemu_conf.h | 10 +- src/qemu/qemu_driver.c | 190 +++---------- tests/qemuxml2argvtest.c | 68 +---- 4 files changed, 338 insertions(+), 640 deletions(-) diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 35caccc..7c0db15 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -1530,6 +1530,15 @@ int qemudExtractVersionInfo(const char *qemu, if (retversion) *retversion = 0; + /* Make sure the binary we are about to try exec'ing exists. + * Technically we could catch the exec() failure, but that's + * in a sub-process so it's hard to feed back a useful error. + */ + if (access(qemu, X_OK) < 0) { + virReportSystemError(errno, _("Cannot find QEMU binary %s"), qemu); + return -1; + } + if (virExec(qemuarg, qemuenv, NULL, &child, -1, &newstdout, NULL, VIR_EXEC_CLEAR_CAPS) < 0) return -1; @@ -3942,43 +3951,35 @@ qemuBuildSmpArgStr(const virDomainDefPtr def, * XXX 'conn' is only required to resolve network -> bridge name * figure out how to remove this requirement some day */ -int qemudBuildCommandLine(virConnectPtr conn, - struct qemud_driver *driver, - virDomainDefPtr def, - virDomainChrDefPtr monitor_chr, - int monitor_json, - unsigned long long qemuCmdFlags, - const char ***retargv, - const char ***retenv, - int **vmfds, - int *nvmfds, - const char *migrateFrom, - virDomainSnapshotObjPtr current_snapshot) +virCommandPtr +qemudBuildCommandLine(virConnectPtr conn, + struct qemud_driver *driver, + virDomainDefPtr def, + virDomainChrDefPtr monitor_chr, + bool monitor_json, + unsigned long long qemuCmdFlags, + const char *migrateFrom, + virDomainSnapshotObjPtr current_snapshot) { int i; - char memory[50]; char boot[VIR_DOMAIN_BOOT_LAST+1]; struct utsname ut; int disableKQEMU = 0; int enableKQEMU = 0; int disableKVM = 0; int enableKVM = 0; - int qargc = 0, qarga = 0; - const char **qargv = NULL; - int qenvc = 0, qenva = 0; - const char **qenv = NULL; const char *emulator; char uuid[VIR_UUID_STRING_BUFLEN]; - char domid[50]; char *cpu; char *smp; int last_good_net = -1; bool hasHwVirt = false; + virCommandPtr cmd; uname_normalize(&ut); if (qemuAssignDeviceAliases(def, qemuCmdFlags) < 0) - return -1; + return NULL; virUUIDFormat(def->uuid, uuid); @@ -3990,7 +3991,7 @@ int qemudBuildCommandLine(virConnectPtr conn, if (!(qemuCmdFlags & QEMUD_CMD_FLAG_MIGRATE_QEMU_TCP)) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("TCP migration is not supported with this QEMU binary")); - return -1; + return NULL; } } else if (STREQ(migrateFrom, "stdio")) { if (qemuCmdFlags & QEMUD_CMD_FLAG_MIGRATE_QEMU_EXEC) { @@ -3998,13 +3999,13 @@ int qemudBuildCommandLine(virConnectPtr conn, } else if (!(qemuCmdFlags & QEMUD_CMD_FLAG_MIGRATE_KVM_STDIO)) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("STDIO migration is not supported with this QEMU binary")); - return -1; + return NULL; } } else if (STRPREFIX(migrateFrom, "exec")) { if (!(qemuCmdFlags & QEMUD_CMD_FLAG_MIGRATE_QEMU_EXEC)) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("STDIO migration is not supported with this QEMU binary")); - return -1; + return NULL; } } } @@ -4065,134 +4066,45 @@ int qemudBuildCommandLine(virConnectPtr conn, break; } -#define ADD_ARG_SPACE \ - do { \ - if (qargc == qarga) { \ - qarga += 10; \ - if (VIR_REALLOC_N(qargv, qarga) < 0) \ - goto no_memory; \ - } \ - } while (0) - -#define ADD_ARG(thisarg) \ - do { \ - ADD_ARG_SPACE; \ - qargv[qargc++] = thisarg; \ - } while (0) - -#define ADD_ARG_LIT(thisarg) \ - do { \ - ADD_ARG_SPACE; \ - if ((qargv[qargc++] = strdup(thisarg)) == NULL) \ - goto no_memory; \ - } while (0) - -#define ADD_USBDISK(thisarg) \ - do { \ - ADD_ARG_LIT("-usbdevice"); \ - ADD_ARG_SPACE; \ - if ((virAsprintf((char **)&(qargv[qargc++]), \ - "disk:%s", thisarg)) == -1) { \ - goto no_memory; \ - } \ - } while (0) - -#define ADD_ENV_SPACE \ - do { \ - if (qenvc == qenva) { \ - qenva += 10; \ - if (VIR_REALLOC_N(qenv, qenva) < 0) \ - goto no_memory; \ - } \ - } while (0) - -#define ADD_ENV(thisarg) \ - do { \ - ADD_ENV_SPACE; \ - qenv[qenvc++] = thisarg; \ - } while (0) - -#define ADD_ENV_LIT(thisarg) \ - do { \ - ADD_ENV_SPACE; \ - if ((qenv[qenvc++] = strdup(thisarg)) == NULL) \ - goto no_memory; \ - } while (0) - -#define ADD_ENV_PAIR(envname, val) \ - do { \ - char *envval; \ - ADD_ENV_SPACE; \ - if (virAsprintf(&envval, "%s=%s", envname, val) < 0) \ - goto no_memory; \ - qenv[qenvc++] = envval; \ - } while (0) - - /* Make sure to unset or set all envvars in qemuxml2argvtest.c that - * are copied here using this macro, otherwise the test may fail */ -#define ADD_ENV_COPY(envname) \ - do { \ - char *val = getenv(envname); \ - if (val != NULL) { \ - ADD_ENV_PAIR(envname, val); \ - } \ - } while (0) + cmd = virCommandNewArgList(emulator, "-S", NULL); - /* Set '-m MB' based on maxmem, because the lower 'memory' limit - * is set post-startup using the balloon driver. If balloon driver - * is not supported, then they're out of luck anyway - */ - snprintf(memory, sizeof(memory), "%lu", def->mem.max_balloon/1024); - snprintf(domid, sizeof(domid), "%d", def->id); - - ADD_ENV_LIT("LC_ALL=C"); - - ADD_ENV_COPY("LD_PRELOAD"); - ADD_ENV_COPY("LD_LIBRARY_PATH"); - ADD_ENV_COPY("PATH"); - ADD_ENV_COPY("HOME"); - ADD_ENV_COPY("USER"); - ADD_ENV_COPY("LOGNAME"); - ADD_ENV_COPY("TMPDIR"); - - ADD_ARG_LIT(emulator); - ADD_ARG_LIT("-S"); + virCommandAddEnvPassCommon(cmd); /* This should *never* be NULL, since we always provide * a machine in the capabilities data for QEMU. So this * check is just here as a safety in case the unexpected * happens */ - if (def->os.machine) { - ADD_ARG_LIT("-M"); - ADD_ARG_LIT(def->os.machine); - } + if (def->os.machine) + virCommandAddArgList(cmd, "-M", def->os.machine, NULL); if (qemuBuildCpuArgStr(driver, def, emulator, qemuCmdFlags, &ut, &cpu, &hasHwVirt) < 0) goto error; if (cpu) { - ADD_ARG_LIT("-cpu"); - ADD_ARG_LIT(cpu); + virCommandAddArgList(cmd, "-cpu", cpu, NULL); VIR_FREE(cpu); if ((qemuCmdFlags & QEMUD_CMD_FLAG_NESTING) && hasHwVirt) - ADD_ARG_LIT("-enable-nesting"); + virCommandAddArg(cmd, "-enable-nesting"); } if (disableKQEMU) - ADD_ARG_LIT("-no-kqemu"); - else if (enableKQEMU) { - ADD_ARG_LIT("-enable-kqemu"); - ADD_ARG_LIT("-kernel-kqemu"); - } + virCommandAddArg(cmd, "-no-kqemu"); + else if (enableKQEMU) + virCommandAddArgList(cmd, "-enable-kqemu", "-kernel-kqemu", NULL); if (disableKVM) - ADD_ARG_LIT("-no-kvm"); + virCommandAddArg(cmd, "-no-kvm"); if (enableKVM) - ADD_ARG_LIT("-enable-kvm"); - ADD_ARG_LIT("-m"); - ADD_ARG_LIT(memory); + virCommandAddArg(cmd, "-enable-kvm"); + + /* Set '-m MB' based on maxmem, because the lower 'memory' limit + * is set post-startup using the balloon driver. If balloon driver + * is not supported, then they're out of luck anyway + */ + virCommandAddArg(cmd, "-m"); + virCommandAddArgFormat(cmd, "%lu", def->mem.max_balloon / 1024); if (def->mem.hugepage_backed) { if (!driver->hugetlbfs_mount) { qemuReportError(VIR_ERR_INTERNAL_ERROR, @@ -4210,43 +4122,38 @@ int qemudBuildCommandLine(virConnectPtr conn, def->emulator); goto error; } - ADD_ARG_LIT("-mem-prealloc"); - ADD_ARG_LIT("-mem-path"); - ADD_ARG_LIT(driver->hugepage_path); + virCommandAddArgList(cmd, "-mem-prealloc", "-mem-path", + driver->hugepage_path, NULL); } - ADD_ARG_LIT("-smp"); + virCommandAddArg(cmd, "-smp"); if (!(smp = qemuBuildSmpArgStr(def, qemuCmdFlags))) goto error; - ADD_ARG(smp); + virCommandAddArg(cmd, smp); + VIR_FREE(smp); if (qemuCmdFlags & QEMUD_CMD_FLAG_NAME) { - ADD_ARG_LIT("-name"); + virCommandAddArg(cmd, "-name"); if (driver->setProcessName && (qemuCmdFlags & QEMUD_CMD_FLAG_NAME_PROCESS)) { - char *name; - if (virAsprintf(&name, "%s,process=qemu:%s", - def->name, def->name) < 0) - goto no_memory; - ADD_ARG_LIT(name); + virCommandAddArgFormat(cmd, "%s,process=qemu:%s", + def->name, def->name); } else { - ADD_ARG_LIT(def->name); + virCommandAddArg(cmd, def->name); } } - if (qemuCmdFlags & QEMUD_CMD_FLAG_UUID) { - ADD_ARG_LIT("-uuid"); - ADD_ARG_LIT(uuid); - } + if (qemuCmdFlags & QEMUD_CMD_FLAG_UUID) + virCommandAddArgList(cmd, "-uuid", uuid, NULL); if (def->virtType == VIR_DOMAIN_VIRT_XEN || STREQ(def->os.type, "xen") || STREQ(def->os.type, "linux")) { if (qemuCmdFlags & QEMUD_CMD_FLAG_DOMID) { - ADD_ARG_LIT("-domid"); - ADD_ARG_LIT(domid); + virCommandAddArg(cmd, "-domid"); + virCommandAddArgFormat(cmd, "%d", def->id); } else if (qemuCmdFlags & QEMUD_CMD_FLAG_XEN_DOMID) { - ADD_ARG_LIT("-xen-attach"); - ADD_ARG_LIT("-xen-domid"); - ADD_ARG_LIT(domid); + virCommandAddArg(cmd, "-xen-attach"); + virCommandAddArg(cmd, "-xen-domid"); + virCommandAddArgFormat(cmd, "%d", def->id); } else { qemuReportError(VIR_ERR_INTERNAL_ERROR, _("qemu emulator '%s' does not support xen"), @@ -4288,13 +4195,13 @@ int qemudBuildCommandLine(virConnectPtr conn, smbioscmd = qemuBuildSmbiosBiosStr(source); if (smbioscmd != NULL) { - ADD_ARG_LIT("-smbios"); - ADD_ARG(smbioscmd); + virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL); + VIR_FREE(smbioscmd); } smbioscmd = qemuBuildSmbiosSystemStr(source); if (smbioscmd != NULL) { - ADD_ARG_LIT("-smbios"); - ADD_ARG(smbioscmd); + virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL); + VIR_FREE(smbioscmd); } } } @@ -4307,12 +4214,14 @@ int qemudBuildCommandLine(virConnectPtr conn, * these defaults ourselves... */ if (!def->graphics) - ADD_ARG_LIT("-nographic"); + virCommandAddArg(cmd, "-nographic"); if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { if (qemuCmdFlags & QEMUD_CMD_FLAG_NODEFCONFIG) - ADD_ARG_LIT("-nodefconfig"); /* Disabling global config files */ - ADD_ARG_LIT("-nodefaults"); /* Disabling default guest devices */ + virCommandAddArg(cmd, + "-nodefconfig"); /* Disable global config files */ + virCommandAddArg(cmd, + "-nodefaults"); /* Disable default guest devices */ } if (monitor_chr) { @@ -4320,39 +4229,40 @@ int qemudBuildCommandLine(virConnectPtr conn, /* Use -chardev if it's available */ if (qemuCmdFlags & QEMUD_CMD_FLAG_CHARDEV) { - ADD_ARG_LIT("-chardev"); + virCommandAddArg(cmd, "-chardev"); if (!(chrdev = qemuBuildChrChardevStr(monitor_chr))) goto error; - ADD_ARG(chrdev); + virCommandAddArg(cmd, chrdev); + VIR_FREE(chrdev); - ADD_ARG_LIT("-mon"); - if (monitor_json) - ADD_ARG_LIT("chardev=monitor,mode=control"); - else - ADD_ARG_LIT("chardev=monitor,mode=readline"); + virCommandAddArg(cmd, "-mon"); + virCommandAddArgFormat(cmd, "chardev=monitor,mode=%s", + monitor_json ? "control" : "readline"); } else { const char *prefix = NULL; if (monitor_json) prefix = "control,"; - ADD_ARG_LIT("-monitor"); + virCommandAddArg(cmd, "-monitor"); if (!(chrdev = qemuBuildChrArgStr(monitor_chr, prefix))) goto error; - ADD_ARG(chrdev); + virCommandAddArg(cmd, chrdev); + VIR_FREE(chrdev); } } if (qemuCmdFlags & QEMUD_CMD_FLAG_RTC) { const char *rtcopt; - ADD_ARG_LIT("-rtc"); + virCommandAddArg(cmd, "-rtc"); if (!(rtcopt = qemuBuildClockArgStr(&def->clock))) goto error; - ADD_ARG(rtcopt); + virCommandAddArg(cmd, rtcopt); + VIR_FREE(rtcopt); } else { switch (def->clock.offset) { case VIR_DOMAIN_CLOCK_OFFSET_LOCALTIME: case VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE: - ADD_ARG_LIT("-localtime"); + virCommandAddArg(cmd, "-localtime"); break; case VIR_DOMAIN_CLOCK_OFFSET_UTC: @@ -4368,7 +4278,7 @@ int qemudBuildCommandLine(virConnectPtr conn, } if (def->clock.offset == VIR_DOMAIN_CLOCK_OFFSET_TIMEZONE && def->clock.data.timezone) { - ADD_ENV_PAIR("TZ", def->clock.data.timezone); + virCommandAddEnvPair(cmd, "TZ", def->clock.data.timezone); } for (i = 0; i < def->clock.ntimers; i++) { @@ -4392,7 +4302,7 @@ int qemudBuildCommandLine(virConnectPtr conn, /* the default - do nothing */ break; case VIR_DOMAIN_TIMER_TICKPOLICY_CATCHUP: - ADD_ARG_LIT("-rtc-td-hack"); + virCommandAddArg(cmd, "-rtc-td-hack"); break; case VIR_DOMAIN_TIMER_TICKPOLICY_MERGE: case VIR_DOMAIN_TIMER_TICKPOLICY_DISCARD: @@ -4421,14 +4331,14 @@ int qemudBuildCommandLine(virConnectPtr conn, /* delay is the default if we don't have kernel (-no-kvm-pit), otherwise, the default is catchup. */ if (qemuCmdFlags & QEMUD_CMD_FLAG_NO_KVM_PIT) - ADD_ARG_LIT("-no-kvm-pit-reinjection"); + virCommandAddArg(cmd, "-no-kvm-pit-reinjection"); break; case VIR_DOMAIN_TIMER_TICKPOLICY_CATCHUP: if (qemuCmdFlags & QEMUD_CMD_FLAG_NO_KVM_PIT) { /* do nothing - this is default for kvm-pit */ } else if (qemuCmdFlags & QEMUD_CMD_FLAG_TDF) { /* -tdf switches to 'catchup' with userspace pit. */ - ADD_ARG_LIT("-tdf"); + virCommandAddArg(cmd, "-tdf"); } else { /* can't catchup if we have neither pit mode */ qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -4457,7 +4367,7 @@ int qemudBuildCommandLine(virConnectPtr conn, if (qemuCmdFlags & QEMUD_CMD_FLAG_NO_HPET) { if (def->clock.timers[i]->present == 0) - ADD_ARG_LIT("-no-hpet"); + virCommandAddArg(cmd, "-no-hpet"); } else { /* no hpet timer available. The only possible action is to raise an error if present="yes" */ @@ -4472,10 +4382,10 @@ int qemudBuildCommandLine(virConnectPtr conn, if ((qemuCmdFlags & QEMUD_CMD_FLAG_NO_REBOOT) && def->onReboot != VIR_DOMAIN_LIFECYCLE_RESTART) - ADD_ARG_LIT("-no-reboot"); + virCommandAddArg(cmd, "-no-reboot"); if (!(def->features & (1 << VIR_DOMAIN_FEATURE_ACPI))) - ADD_ARG_LIT("-no-acpi"); + virCommandAddArg(cmd, "-no-acpi"); if (!def->os.bootloader) { for (i = 0 ; i < def->os.nBootDevs ; i++) { @@ -4499,7 +4409,8 @@ int qemudBuildCommandLine(virConnectPtr conn, } if (def->os.nBootDevs) { virBuffer boot_buf = VIR_BUFFER_INITIALIZER; - ADD_ARG_LIT("-boot"); + char *bootstr; + virCommandAddArg(cmd, "-boot"); boot[def->os.nBootDevs] = '\0'; @@ -4518,24 +4429,19 @@ int qemudBuildCommandLine(virConnectPtr conn, goto error; } - ADD_ARG(virBufferContentAndReset(&boot_buf)); + bootstr = virBufferContentAndReset(&boot_buf); + virCommandAddArg(cmd, bootstr); + VIR_FREE(bootstr); } - if (def->os.kernel) { - ADD_ARG_LIT("-kernel"); - ADD_ARG_LIT(def->os.kernel); - } - if (def->os.initrd) { - ADD_ARG_LIT("-initrd"); - ADD_ARG_LIT(def->os.initrd); - } - if (def->os.cmdline) { - ADD_ARG_LIT("-append"); - ADD_ARG_LIT(def->os.cmdline); - } + if (def->os.kernel) + virCommandAddArgList(cmd, "-kernel", def->os.kernel, NULL); + if (def->os.initrd) + virCommandAddArgList(cmd, "-initrd", def->os.initrd, NULL); + if (def->os.cmdline) + virCommandAddArgList(cmd, "-append", def->os.cmdline, NULL); } else { - ADD_ARG_LIT("-bootloader"); - ADD_ARG_LIT(def->os.bootloader); + virCommandAddArgList(cmd, "-bootloader", def->os.bootloader, NULL); } for (i = 0 ; i < def->ndisks ; i++) { @@ -4568,13 +4474,14 @@ int qemudBuildCommandLine(virConnectPtr conn, goto error; } - ADD_ARG_LIT("-device"); + virCommandAddArg(cmd, "-device"); char *devstr; if (!(devstr = qemuBuildControllerDevStr(def->controllers[i]))) goto no_memory; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); } } @@ -4610,10 +4517,12 @@ int qemudBuildCommandLine(virConnectPtr conn, if ((disk->bus == VIR_DOMAIN_DISK_BUS_USB) && !(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - ADD_USBDISK(disk->src); + virCommandAddArg(cmd, "-usbdevice"); + virCommandAddArgFormat(cmd, "disk:%s", disk->src); } else { qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported usb disk type for '%s'"), disk->src); + _("unsupported usb disk type for '%s'"), + disk->src); goto error; } continue; @@ -4634,10 +4543,10 @@ int qemudBuildCommandLine(virConnectPtr conn, break; } - ADD_ARG_LIT("-drive"); + virCommandAddArg(cmd, "-drive"); - /* Unfortunately it is nt possible to use - -device for floppys, or Xen paravirt + /* Unfortunately it is not possible to use + -device for floppies, or Xen paravirt devices. Fortunately, those don't need static PCI addresses, so we don't really care that we can't use -device */ @@ -4648,24 +4557,23 @@ int qemudBuildCommandLine(virConnectPtr conn, (withDeviceArg ? qemuCmdFlags : (qemuCmdFlags & ~QEMUD_CMD_FLAG_DEVICE))))) goto error; - ADD_ARG(optstr); + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); if (withDeviceArg) { if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { - char *fdc; - ADD_ARG_LIT("-global"); - - if (virAsprintf(&fdc, "isa-fdc.drive%c=drive-%s", - disk->info.addr.drive.unit ? 'B' : 'A', - disk->info.alias) < 0) - goto no_memory; - ADD_ARG(fdc); + virCommandAddArg(cmd, "-global"); + virCommandAddArgFormat(cmd, "isa-fdc.drive%c=drive-%s", + disk->info.addr.drive.unit + ? 'B' : 'A', + disk->info.alias); } else { - ADD_ARG_LIT("-device"); + virCommandAddArg(cmd, "-device"); if (!(optstr = qemuBuildDriveDevStr(disk))) goto error; - ADD_ARG(optstr); + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); } } } @@ -4677,10 +4585,12 @@ int qemudBuildCommandLine(virConnectPtr conn, if (disk->bus == VIR_DOMAIN_DISK_BUS_USB) { if (disk->device == VIR_DOMAIN_DISK_DEVICE_DISK) { - ADD_USBDISK(disk->src); + virCommandAddArg(cmd, "-usbdevice"); + virCommandAddArgFormat(cmd, "disk:%s", disk->src); } else { qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("unsupported usb disk type for '%s'"), disk->src); + _("unsupported usb disk type for '%s'"), + disk->src); goto error; } continue; @@ -4726,8 +4636,7 @@ int qemudBuildCommandLine(virConnectPtr conn, snprintf(file, PATH_MAX, "%s", disk->src); } - ADD_ARG_LIT(dev); - ADD_ARG_LIT(file); + virCommandAddArgList(cmd, dev, file, NULL); } } @@ -4736,15 +4645,17 @@ int qemudBuildCommandLine(virConnectPtr conn, char *optstr; virDomainFSDefPtr fs = def->fss[i]; - ADD_ARG_LIT("-fsdev"); + virCommandAddArg(cmd, "-fsdev"); if (!(optstr = qemuBuildFSStr(fs, qemuCmdFlags))) goto error; - ADD_ARG(optstr); + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); - ADD_ARG_LIT("-device"); + virCommandAddArg(cmd, "-device"); if (!(optstr = qemuBuildFSDevStr(fs))) goto error; - ADD_ARG(optstr); + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); } } else { if (def->nfss) { @@ -4756,10 +4667,8 @@ int qemudBuildCommandLine(virConnectPtr conn, if (!def->nnets) { /* If we have -device, then we set -nodefault already */ - if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { - ADD_ARG_LIT("-net"); - ADD_ARG_LIT("none"); - } + if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) + virCommandAddArgList(cmd, "-net", "none", NULL); } else { for (i = 0 ; i < def->nnets ; i++) { virDomainNetDefPtr net = def->nets[i]; @@ -4777,21 +4686,16 @@ int qemudBuildCommandLine(virConnectPtr conn, if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) { - int tapfd = qemudNetworkIfaceConnect(conn, driver, net, qemuCmdFlags); + int tapfd = qemudNetworkIfaceConnect(conn, driver, net, + qemuCmdFlags); if (tapfd < 0) goto error; - if (VIR_REALLOC_N(*vmfds, (*nvmfds)+1) < 0) { - virDomainConfNWFilterTeardown(net); - VIR_FORCE_CLOSE(tapfd); - goto no_memory; - } - last_good_net = i; + virCommandTransferFD(cmd, tapfd); - (*vmfds)[(*nvmfds)++] = tapfd; - - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", tapfd) >= sizeof(tapfd_name)) + if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", + tapfd) >= sizeof(tapfd_name)) goto no_memory; } else if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) { int tapfd = qemudPhysIfaceConnect(conn, driver, net, @@ -4800,17 +4704,11 @@ int qemudBuildCommandLine(virConnectPtr conn, if (tapfd < 0) goto error; - if (VIR_REALLOC_N(*vmfds, (*nvmfds)+1) < 0) { - virDomainConfNWFilterTeardown(net); - VIR_FORCE_CLOSE(tapfd); - goto no_memory; - } - last_good_net = i; + virCommandTransferFD(cmd, tapfd); - (*vmfds)[(*nvmfds)++] = tapfd; - - if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", tapfd) >= sizeof(tapfd_name)) + if (snprintf(tapfd_name, sizeof(tapfd_name), "%d", + tapfd) >= sizeof(tapfd_name)) goto no_memory; } @@ -4821,14 +4719,10 @@ int qemudBuildCommandLine(virConnectPtr conn, network device */ int vhostfd = qemudOpenVhostNet(net, qemuCmdFlags); if (vhostfd >= 0) { - if (VIR_REALLOC_N(*vmfds, (*nvmfds)+1) < 0) { - VIR_FORCE_CLOSE(vhostfd); - goto no_memory; - } + virCommandTransferFD(cmd, vhostfd); - (*vmfds)[(*nvmfds)++] = vhostfd; - if (snprintf(vhostfd_name, sizeof(vhostfd_name), "%d", vhostfd) - >= sizeof(vhostfd_name)) + if (snprintf(vhostfd_name, sizeof(vhostfd_name), "%d", + vhostfd) >= sizeof(vhostfd_name)) goto no_memory; } } @@ -4842,40 +4736,42 @@ int qemudBuildCommandLine(virConnectPtr conn, */ if ((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { - ADD_ARG_LIT("-netdev"); + virCommandAddArg(cmd, "-netdev"); if (!(host = qemuBuildHostNetStr(net, ',', vlan, tapfd_name, vhostfd_name))) goto error; - ADD_ARG(host); + virCommandAddArg(cmd, host); + VIR_FREE(host); } if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { - ADD_ARG_LIT("-device"); + virCommandAddArg(cmd, "-device"); if (!(nic = qemuBuildNicDevStr(net, vlan))) goto error; - ADD_ARG(nic); + virCommandAddArg(cmd, nic); + VIR_FREE(nic); } else { - ADD_ARG_LIT("-net"); + virCommandAddArg(cmd, "-net"); if (!(nic = qemuBuildNicStr(net, "nic,", vlan))) goto error; - ADD_ARG(nic); + virCommandAddArg(cmd, nic); + VIR_FREE(nic); } if (!((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE))) { - ADD_ARG_LIT("-net"); + virCommandAddArg(cmd, "-net"); if (!(host = qemuBuildHostNetStr(net, ',', vlan, tapfd_name, vhostfd_name))) goto error; - ADD_ARG(host); + virCommandAddArg(cmd, host); + VIR_FREE(host); } } } if (!def->nserials) { /* If we have -device, then we set -nodefault already */ - if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { - ADD_ARG_LIT("-serial"); - ADD_ARG_LIT("none"); - } + if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) + virCommandAddArgList(cmd, "-serial", "none", NULL); } else { for (i = 0 ; i < def->nserials ; i++) { virDomainChrDefPtr serial = def->serials[i]; @@ -4884,30 +4780,29 @@ int qemudBuildCommandLine(virConnectPtr conn, /* Use -chardev with -device if they are available */ if ((qemuCmdFlags & QEMUD_CMD_FLAG_CHARDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { - ADD_ARG_LIT("-chardev"); + virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(serial))) goto error; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); - ADD_ARG_LIT("-device"); - if (virAsprintf(&devstr, "isa-serial,chardev=%s", serial->info.alias) < 0) - goto no_memory; - ADD_ARG(devstr); + virCommandAddArg(cmd, "-device"); + virCommandAddArgFormat(cmd, "isa-serial,chardev=%s", + serial->info.alias); } else { - ADD_ARG_LIT("-serial"); + virCommandAddArg(cmd, "-serial"); if (!(devstr = qemuBuildChrArgStr(serial, NULL))) goto error; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); } } } if (!def->nparallels) { /* If we have -device, then we set -nodefault already */ - if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { - ADD_ARG_LIT("-parallel"); - ADD_ARG_LIT("none"); - } + if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) + virCommandAddArgList(cmd, "-parallel", "none", NULL); } else { for (i = 0 ; i < def->nparallels ; i++) { virDomainChrDefPtr parallel = def->parallels[i]; @@ -4916,20 +4811,21 @@ int qemudBuildCommandLine(virConnectPtr conn, /* Use -chardev with -device if they are available */ if ((qemuCmdFlags & QEMUD_CMD_FLAG_CHARDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { - ADD_ARG_LIT("-chardev"); + virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(parallel))) goto error; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); - ADD_ARG_LIT("-device"); - if (virAsprintf(&devstr, "isa-parallel,chardev=%s", parallel->info.alias) < 0) - goto no_memory; - ADD_ARG(devstr); + virCommandAddArg(cmd, "-device"); + virCommandAddArgFormat(cmd, "isa-parallel,chardev=%s", + parallel->info.alias); } else { - ADD_ARG_LIT("-parallel"); + virCommandAddArg(cmd, "-parallel"); if (!(devstr = qemuBuildChrArgStr(parallel, NULL))) goto error; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); } } } @@ -4947,24 +4843,23 @@ int qemudBuildCommandLine(virConnectPtr conn, goto error; } - ADD_ARG_LIT("-chardev"); + virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(channel))) goto error; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); char *addr = virSocketFormatAddr(channel->target.addr); if (!addr) goto error; int port = virSocketGetPort(channel->target.addr); - ADD_ARG_LIT("-netdev"); - if (virAsprintf(&devstr, "user,guestfwd=tcp:%s:%i,chardev=%s,id=user-%s", - addr, port, channel->info.alias, channel->info.alias) < 0) { - VIR_FREE(addr); - goto no_memory; - } + virCommandAddArg(cmd, "-netdev"); + virCommandAddArgFormat(cmd, + "user,guestfwd=tcp:%s:%i,chardev=%s,id=user-%s", + addr, port, channel->info.alias, + channel->info.alias); VIR_FREE(addr); - ADD_ARG(devstr); break; case VIR_DOMAIN_CHR_CHANNEL_TARGET_TYPE_VIRTIO: @@ -4974,15 +4869,17 @@ int qemudBuildCommandLine(virConnectPtr conn, goto error; } - ADD_ARG_LIT("-chardev"); + virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(channel))) goto error; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); - ADD_ARG_LIT("-device"); + virCommandAddArg(cmd, "-device"); if (!(devstr = qemuBuildVirtioSerialPortDevStr(channel))) goto error; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); break; } } @@ -5000,15 +4897,17 @@ int qemudBuildCommandLine(virConnectPtr conn, goto error; } - ADD_ARG_LIT("-chardev"); + virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(console))) goto error; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); - ADD_ARG_LIT("-device"); + virCommandAddArg(cmd, "-device"); if (!(devstr = qemuBuildVirtioSerialPortDevStr(console))) goto error; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); break; case VIR_DOMAIN_CHR_CONSOLE_TARGET_TYPE_SERIAL: @@ -5022,20 +4921,22 @@ int qemudBuildCommandLine(virConnectPtr conn, } } - ADD_ARG_LIT("-usb"); + virCommandAddArg(cmd, "-usb"); for (i = 0 ; i < def->ninputs ; i++) { virDomainInputDefPtr input = def->inputs[i]; if (input->bus == VIR_DOMAIN_INPUT_BUS_USB) { if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { char *optstr; - ADD_ARG_LIT("-device"); + virCommandAddArg(cmd, "-device"); if (!(optstr = qemuBuildUSBInputDevStr(input))) goto error; - ADD_ARG(optstr); + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); } else { - ADD_ARG_LIT("-usbdevice"); - ADD_ARG_LIT(input->type == VIR_DOMAIN_INPUT_TYPE_MOUSE ? "mouse" : "tablet"); + virCommandAddArgList(cmd, "-usbdevice", + input->type == VIR_DOMAIN_INPUT_TYPE_MOUSE + ? "mouse" : "tablet", NULL); } } } @@ -5079,7 +4980,8 @@ int qemudBuildCommandLine(virConnectPtr conn, virBufferAddLit(&opt, ",sasl"); if (driver->vncSASLdir) - ADD_ENV_PAIR("SASL_CONF_DIR", driver->vncSASLdir); + virCommandAddEnvPair(cmd, "SASL_CONF_DIR", + driver->vncSASLdir); /* TODO: Support ACLs later */ } @@ -5094,11 +4996,11 @@ int qemudBuildCommandLine(virConnectPtr conn, optstr = virBufferContentAndReset(&opt); - ADD_ARG_LIT("-vnc"); - ADD_ARG(optstr); + virCommandAddArgList(cmd, "-vnc", optstr, NULL); + VIR_FREE(optstr); if (def->graphics[0]->data.vnc.keymap) { - ADD_ARG_LIT("-k"); - ADD_ARG_LIT(def->graphics[0]->data.vnc.keymap); + virCommandAddArgList(cmd, "-k", def->graphics[0]->data.vnc.keymap, + NULL); } /* Unless user requested it, set the audio backend to none, to @@ -5106,45 +5008,33 @@ int qemudBuildCommandLine(virConnectPtr conn, * security issues and might not work when using VNC. */ if (driver->vncAllowHostAudio) { - ADD_ENV_COPY("QEMU_AUDIO_DRV"); + virCommandAddEnvPass(cmd, "QEMU_AUDIO_DRV"); } else { - ADD_ENV_LIT("QEMU_AUDIO_DRV=none"); + virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none"); } } else if ((def->ngraphics == 1) && def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL) { - char *xauth = NULL; - char *display = NULL; - - if (def->graphics[0]->data.sdl.xauth && - virAsprintf(&xauth, "XAUTHORITY=%s", - def->graphics[0]->data.sdl.xauth) < 0) - goto no_memory; - if (def->graphics[0]->data.sdl.display && - virAsprintf(&display, "DISPLAY=%s", - def->graphics[0]->data.sdl.display) < 0) { - VIR_FREE(xauth); - goto no_memory; - } - - if (xauth) - ADD_ENV(xauth); - if (display) - ADD_ENV(display); + if (def->graphics[0]->data.sdl.xauth) + virCommandAddEnvPair(cmd, "XAUTHORITY", + def->graphics[0]->data.sdl.xauth); + if (def->graphics[0]->data.sdl.display) + virCommandAddEnvPair(cmd, "DISPLAY", + def->graphics[0]->data.sdl.display); if (def->graphics[0]->data.sdl.fullscreen) - ADD_ARG_LIT("-full-screen"); + virCommandAddArg(cmd, "-full-screen"); /* If using SDL for video, then we should just let it * use QEMU's host audio drivers, possibly SDL too * User can set these two before starting libvirtd */ - ADD_ENV_COPY("QEMU_AUDIO_DRV"); - ADD_ENV_COPY("SDL_AUDIODRIVER"); + virCommandAddEnvPass(cmd, "QEMU_AUDIO_DRV"); + virCommandAddEnvPass(cmd, "SDL_AUDIODRIVER"); /* New QEMU has this flag to let us explicitly ask for * SDL graphics. This is better than relying on the * default, since the default changes :-( */ if (qemuCmdFlags & QEMUD_CMD_FLAG_SDL) - ADD_ARG_LIT("-sdl"); + virCommandAddArg(cmd, "-sdl"); } else if ((def->ngraphics == 1) && def->graphics[0]->type == VIR_DOMAIN_GRAPHICS_TYPE_SPICE) { @@ -5197,16 +5087,15 @@ int qemudBuildCommandLine(virConnectPtr conn, optstr = virBufferContentAndReset(&opt); - ADD_ARG_LIT("-spice"); - ADD_ARG(optstr); - if (def->graphics[0]->data.spice.keymap) { - ADD_ARG_LIT("-k"); - ADD_ARG_LIT(def->graphics[0]->data.spice.keymap); - } + virCommandAddArgList(cmd, "-spice", optstr, NULL); + VIR_FREE(optstr); + if (def->graphics[0]->data.spice.keymap) + virCommandAddArgList(cmd, "-k", + def->graphics[0]->data.spice.keymap, NULL); /* SPICE includes native support for tunnelling audio, so we * set the audio backend to point at SPICE's own driver */ - ADD_ENV_LIT("QEMU_AUDIO_DRV=spice"); + virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=spice"); } else if ((def->ngraphics == 1)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, @@ -5235,18 +5124,17 @@ int qemudBuildCommandLine(virConnectPtr conn, goto error; } - ADD_ARG_LIT("-vga"); - ADD_ARG_LIT(vgastr); + virCommandAddArgList(cmd, "-vga", vgastr, NULL); } } else { switch (def->videos[0]->type) { case VIR_DOMAIN_VIDEO_TYPE_VGA: - ADD_ARG_LIT("-std-vga"); + virCommandAddArg(cmd, "-std-vga"); break; case VIR_DOMAIN_VIDEO_TYPE_VMVGA: - ADD_ARG_LIT("-vmwarevga"); + virCommandAddArg(cmd, "-vmwarevga"); break; case VIR_DOMAIN_VIDEO_TYPE_XEN: @@ -5273,12 +5161,13 @@ int qemudBuildCommandLine(virConnectPtr conn, goto error; } - ADD_ARG_LIT("-device"); + virCommandAddArg(cmd, "-device"); if (!(str = qemuBuildVideoDevStr(def->videos[i]))) goto error; - ADD_ARG(str); + virCommandAddArg(cmd, str); + VIR_FREE(str); } } else { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -5290,10 +5179,8 @@ int qemudBuildCommandLine(virConnectPtr conn, } else { /* If we have -device, then we set -nodefault already */ if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) && - (qemuCmdFlags & QEMUD_CMD_FLAG_VGA)) { - ADD_ARG_LIT("-vga"); - ADD_ARG_LIT("none"); - } + (qemuCmdFlags & QEMUD_CMD_FLAG_VGA)) + virCommandAddArgList(cmd, "-vga", "none", NULL); } /* Add sound hardware */ @@ -5307,15 +5194,15 @@ int qemudBuildCommandLine(virConnectPtr conn, * we don't need to set any PCI address on it, so we don't * mind too much */ if (sound->model == VIR_DOMAIN_SOUND_MODEL_PCSPK) { - ADD_ARG_LIT("-soundhw"); - ADD_ARG_LIT("pcspk"); + virCommandAddArgList(cmd, "-soundhw", "pcspk", NULL); } else { - ADD_ARG_LIT("-device"); + virCommandAddArg(cmd, "-device"); if (!(str = qemuBuildSoundDevStr(sound))) goto error; - ADD_ARG(str); + virCommandAddArg(cmd, str); + VIR_FREE(str); } } } else { @@ -5338,8 +5225,8 @@ int qemudBuildCommandLine(virConnectPtr conn, if (i < (def->nsounds - 1)) strncat(modstr, ",", size--); } - ADD_ARG_LIT("-soundhw"); - ADD_ARG(modstr); + virCommandAddArgList(cmd, "-soundhw", modstr, NULL); + VIR_FREE(modstr); } } @@ -5349,13 +5236,13 @@ int qemudBuildCommandLine(virConnectPtr conn, char *optstr; if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { - ADD_ARG_LIT("-device"); + virCommandAddArg(cmd, "-device"); optstr = qemuBuildWatchdogDevStr(watchdog); if (!optstr) goto error; } else { - ADD_ARG_LIT("-watchdog"); + virCommandAddArg(cmd, "-watchdog"); const char *model = virDomainWatchdogModelTypeToString(watchdog->model); if (!model) { @@ -5367,7 +5254,8 @@ int qemudBuildCommandLine(virConnectPtr conn, if (!(optstr = strdup(model))) goto no_memory; } - ADD_ARG(optstr); + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); const char *action = virDomainWatchdogActionTypeToString(watchdog->action); if (!action) { @@ -5375,8 +5263,7 @@ int qemudBuildCommandLine(virConnectPtr conn, "%s", _("invalid watchdog action")); goto error; } - ADD_ARG_LIT("-watchdog-action"); - ADD_ARG_LIT(action); + virCommandAddArgList(cmd, "-watchdog-action", action, NULL); } /* Add host passthrough hardware */ @@ -5389,15 +5276,17 @@ int qemudBuildCommandLine(virConnectPtr conn, hostdev->source.subsys.type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB) { if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { - ADD_ARG_LIT("-device"); + virCommandAddArg(cmd, "-device"); if (!(devstr = qemuBuildUSBHostdevDevStr(hostdev))) goto error; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); } else { - ADD_ARG_LIT("-usbdevice"); + virCommandAddArg(cmd, "-usbdevice"); if (!(devstr = qemuBuildUSBHostdevUsbDevStr(hostdev))) goto error; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); } } @@ -5416,26 +5305,22 @@ int qemudBuildCommandLine(virConnectPtr conn, goto no_memory; } - if (VIR_REALLOC_N(*vmfds, (*nvmfds)+1) < 0) { - VIR_FREE(configfd_name); - VIR_FORCE_CLOSE(configfd); - goto no_memory; - } - - (*vmfds)[(*nvmfds)++] = configfd; + virCommandTransferFD(cmd, configfd); } } - ADD_ARG_LIT("-device"); + virCommandAddArg(cmd, "-device"); devstr = qemuBuildPCIHostdevDevStr(hostdev, configfd_name); VIR_FREE(configfd_name); if (!devstr) goto error; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); } else if (qemuCmdFlags & QEMUD_CMD_FLAG_PCIDEVICE) { - ADD_ARG_LIT("-pcidevice"); + virCommandAddArg(cmd, "-pcidevice"); if (!(devstr = qemuBuildPCIHostdevPCIDevStr(hostdev))) goto error; - ADD_ARG(devstr); + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); } else { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("PCI device assignment is not supported by this version of qemu")); @@ -5444,10 +5329,8 @@ int qemudBuildCommandLine(virConnectPtr conn, } } - if (migrateFrom) { - ADD_ARG_LIT("-incoming"); - ADD_ARG_LIT(migrateFrom); - } + if (migrateFrom) + virCommandAddArgList(cmd, "-incoming", migrateFrom, NULL); /* QEMU changed its default behavior to not include the virtio balloon * device. Explicitly request it to ensure it will be present. @@ -5465,76 +5348,43 @@ int qemudBuildCommandLine(virConnectPtr conn, } if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { char *optstr; - ADD_ARG_LIT("-device"); + virCommandAddArg(cmd, "-device"); optstr = qemuBuildMemballoonDevStr(def->memballoon); if (!optstr) goto error; - ADD_ARG(optstr); + virCommandAddArg(cmd, optstr); + VIR_FREE(optstr); } else if (qemuCmdFlags & QEMUD_CMD_FLAG_BALLOON) { - ADD_ARG_LIT("-balloon"); - ADD_ARG_LIT("virtio"); + virCommandAddArgList(cmd, "-balloon", "virtio", NULL); } } - if (current_snapshot && current_snapshot->def->active) { - ADD_ARG_LIT("-loadvm"); - ADD_ARG_LIT(current_snapshot->def->name); - } + if (current_snapshot && current_snapshot->def->active) + virCommandAddArgList(cmd, "-loadvm", current_snapshot->def->name, + NULL); if (def->namespaceData) { - qemuDomainCmdlineDefPtr cmd; - - cmd = def->namespaceData; - for (i = 0; i < cmd->num_args; i++) - ADD_ARG_LIT(cmd->args[i]); - for (i = 0; i < cmd->num_env; i++) { - if (cmd->env_value[i]) - ADD_ENV_PAIR(cmd->env_name[i], cmd->env_value[i]); - else - ADD_ENV_PAIR(cmd->env_name[i], ""); - } - } + qemuDomainCmdlineDefPtr qemucmd; - ADD_ARG(NULL); - ADD_ENV(NULL); + qemucmd = def->namespaceData; + for (i = 0; i < qemucmd->num_args; i++) + virCommandAddArg(cmd, qemucmd->args[i]); + for (i = 0; i < qemucmd->num_env; i++) + virCommandAddEnvPair(cmd, qemucmd->env_name[i], + qemucmd->env_value[i] + ? qemucmd->env_value[i] : ""); + } - *retargv = qargv; - *retenv = qenv; - return 0; + return cmd; no_memory: virReportOOMError(); error: for (i = 0; i <= last_good_net; i++) virDomainConfNWFilterTeardown(def->nets[i]); - if (vmfds && - *vmfds) { - for (i = 0; i < *nvmfds; i++) - VIR_FORCE_CLOSE((*vmfds)[i]); - VIR_FREE(*vmfds); - *nvmfds = 0; - } - if (qargv) { - for (i = 0 ; i < qargc ; i++) - VIR_FREE((qargv)[i]); - VIR_FREE(qargv); - } - if (qenv) { - for (i = 0 ; i < qenvc ; i++) - VIR_FREE((qenv)[i]); - VIR_FREE(qenv); - } - return -1; - -#undef ADD_ARG -#undef ADD_ARG_LIT -#undef ADD_ARG_SPACE -#undef ADD_USBDISK -#undef ADD_ENV -#undef ADD_ENV_COPY -#undef ADD_ENV_LIT -#undef ADD_ENV_SPACE + virCommandFree(cmd); + return NULL; } diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 790ce98..24df871 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -25,6 +25,7 @@ # define __QEMUD_CONF_H # include <config.h> +# include <stdbool.h> # include "ebtables.h" # include "internal.h" @@ -40,6 +41,7 @@ # include "cpu_conf.h" # include "driver.h" # include "bitmap.h" +# include "command.h" # define qemudDebug(fmt, ...) do {} while(0) @@ -227,16 +229,12 @@ int qemudParseHelpStr (const char *qemu, unsigned int *is_kvm, unsigned int *kvm_version); -int qemudBuildCommandLine (virConnectPtr conn, +virCommandPtr qemudBuildCommandLine (virConnectPtr conn, struct qemud_driver *driver, virDomainDefPtr def, virDomainChrDefPtr monitor_chr, - int monitor_json, + bool monitor_json, unsigned long long qemuCmdFlags, - const char ***retargv, - const char ***retenv, - int **vmfds, - int *nvmfds, const char *migrateFrom, virDomainSnapshotObjPtr current_snapshot) ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f00d8a3..38d7bd2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3018,14 +3018,6 @@ qemuAssignPCIAddresses(virDomainDefPtr def) int ret = -1; unsigned long long qemuCmdFlags = 0; qemuDomainPCIAddressSetPtr addrs = NULL; - struct stat sb; - - if (stat(def->emulator, &sb) < 0) { - virReportSystemError(errno, - _("Cannot find QEMU binary %s"), - def->emulator); - goto cleanup; - } if (qemudExtractVersionInfo(def->emulator, NULL, @@ -3865,30 +3857,21 @@ static int qemudStartVMDaemon(virConnectPtr conn, bool start_paused, int stdin_fd, const char *stdin_path) { - const char **argv = NULL, **tmp; - const char **progenv = NULL; - int i, ret, runflags; - struct stat sb; - int *vmfds = NULL; - int nvmfds = 0; + int ret; unsigned long long qemuCmdFlags; - fd_set keepfd; - const char *emulator; - pid_t child; int pos = -1; char ebuf[1024]; char *pidfile = NULL; int logfile = -1; char *timestamp; qemuDomainObjPrivatePtr priv = vm->privateData; + virCommandPtr cmd = NULL; struct qemudHookData hookData; hookData.conn = conn; hookData.vm = vm; hookData.driver = driver; - FD_ZERO(&keepfd); - DEBUG0("Beginning VM startup process"); if (virDomainObjIsActive(vm)) { @@ -3979,21 +3962,8 @@ static int qemudStartVMDaemon(virConnectPtr conn, if ((logfile = qemudLogFD(driver, vm->def->name, false)) < 0) goto cleanup; - emulator = vm->def->emulator; - - /* Make sure the binary we are about to try exec'ing exists. - * Technically we could catch the exec() failure, but that's - * in a sub-process so its hard to feed back a useful error - */ - if (stat(emulator, &sb) < 0) { - virReportSystemError(errno, - _("Cannot find QEMU binary %s"), - emulator); - goto cleanup; - } - - DEBUG0("Determing emulator version"); - if (qemudExtractVersionInfo(emulator, + DEBUG0("Determining emulator version"); + if (qemudExtractVersionInfo(vm->def->emulator, NULL, &qemuCmdFlags) < 0) goto cleanup; @@ -4062,10 +4032,10 @@ static int qemudStartVMDaemon(virConnectPtr conn, DEBUG0("Building emulator command line"); vm->def->id = driver->nextvmid++; - if (qemudBuildCommandLine(conn, driver, vm->def, priv->monConfig, - priv->monJSON, qemuCmdFlags, &argv, &progenv, - &vmfds, &nvmfds, migrateFrom, - vm->current_snapshot) < 0) + if (!(cmd = qemudBuildCommandLine(conn, driver, vm->def, priv->monConfig, + priv->monJSON != 0, qemuCmdFlags, + migrateFrom, + vm->current_snapshot))) goto cleanup; if (qemuDomainSnapshotSetInactive(vm, driver->snapshotDir) < 0) @@ -4100,50 +4070,28 @@ static int qemudStartVMDaemon(virConnectPtr conn, VIR_FREE(timestamp); } - tmp = progenv; - - while (*tmp) { - if (safewrite(logfile, *tmp, strlen(*tmp)) < 0) - VIR_WARN("Unable to write envv to logfile: %s", - virStrerror(errno, ebuf, sizeof ebuf)); - if (safewrite(logfile, " ", 1) < 0) - VIR_WARN("Unable to write envv to logfile: %s", - virStrerror(errno, ebuf, sizeof ebuf)); - tmp++; - } - tmp = argv; - while (*tmp) { - if (safewrite(logfile, *tmp, strlen(*tmp)) < 0) - VIR_WARN("Unable to write argv to logfile: %s", - virStrerror(errno, ebuf, sizeof ebuf)); - if (safewrite(logfile, " ", 1) < 0) - VIR_WARN("Unable to write argv to logfile: %s", - virStrerror(errno, ebuf, sizeof ebuf)); - tmp++; - } - if (safewrite(logfile, "\n", 1) < 0) - VIR_WARN("Unable to write argv to logfile: %s", - virStrerror(errno, ebuf, sizeof ebuf)); + virCommandWriteArgLog(cmd, logfile); if ((pos = lseek(logfile, 0, SEEK_END)) < 0) VIR_WARN("Unable to seek to end of logfile: %s", virStrerror(errno, ebuf, sizeof ebuf)); - for (i = 0 ; i < nvmfds ; i++) - FD_SET(vmfds[i], &keepfd); - VIR_DEBUG("Clear emulator capabilities: %d", driver->clearEmulatorCapabilities); - runflags = VIR_EXEC_NONBLOCK; - if (driver->clearEmulatorCapabilities) { - runflags |= VIR_EXEC_CLEAR_CAPS; - } - - ret = virExecDaemonize(argv, progenv, &keepfd, &child, - stdin_fd, &logfile, &logfile, - runflags, - qemudSecurityHook, &hookData, - pidfile); + if (driver->clearEmulatorCapabilities) + virCommandClearCaps(cmd); + + VIR_WARN("Executing %s", vm->def->emulator); + virCommandSetPreExecHook(cmd, qemudSecurityHook, &hookData); + virCommandSetInputFD(cmd, stdin_fd); + virCommandSetOutputFD(cmd, &logfile); + virCommandSetErrorFD(cmd, &logfile); + virCommandNonblockingFDs(cmd); + virCommandSetPidFile(cmd, pidfile); + virCommandDaemonize(cmd); + + ret = virCommandRun(cmd, NULL); + VIR_WARN("Executing done %s", vm->def->emulator); VIR_FREE(pidfile); /* wait for qemu process to to show up */ @@ -4153,7 +4101,13 @@ static int qemudStartVMDaemon(virConnectPtr conn, _("Domain %s didn't show up\n"), vm->def->name); ret = -1; } +#if 0 } else if (ret == -2) { + /* + * XXX this is bogus. It isn't safe to set vm->pid = child + * because the child no longer exists. + */ + /* The virExec process that launches the daemon failed. Pending on * when it failed (we can't determine for sure), there may be * extra info in the domain log (if the hook failed for example). @@ -4163,30 +4117,16 @@ static int qemudStartVMDaemon(virConnectPtr conn, */ vm->pid = child; ret = 0; +#endif } if (migrateFrom) start_paused = true; vm->state = start_paused ? VIR_DOMAIN_PAUSED : VIR_DOMAIN_RUNNING; - for (i = 0 ; argv[i] ; i++) - VIR_FREE(argv[i]); - VIR_FREE(argv); - - for (i = 0 ; progenv[i] ; i++) - VIR_FREE(progenv[i]); - VIR_FREE(progenv); - if (ret == -1) /* The VM failed to start; tear filters before taps */ virDomainConfVMNWFilterTeardown(vm); - if (vmfds) { - for (i = 0 ; i < nvmfds ; i++) { - VIR_FORCE_CLOSE(vmfds[i]); - } - VIR_FREE(vmfds); - } - if (ret == -1) /* The VM failed to start */ goto cleanup; @@ -4240,6 +4180,7 @@ static int qemudStartVMDaemon(virConnectPtr conn, if (virDomainObjSetDefTransient(driver->caps, vm) < 0) goto cleanup; + virCommandFree(cmd); VIR_FORCE_CLOSE(logfile); return 0; @@ -4248,9 +4189,9 @@ cleanup: /* We jump here if we failed to start the VM for any reason, or * if we failed to initialize the now running VM. kill it off and * pretend we never started it */ - qemudShutdownVMDaemon(driver, vm, 0); - + virCommandFree(cmd); VIR_FORCE_CLOSE(logfile); + qemudShutdownVMDaemon(driver, vm, 0); return -1; } @@ -7312,13 +7253,8 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, struct qemud_driver *driver = conn->privateData; virDomainDefPtr def = NULL; virDomainChrDef monConfig; - const char *emulator; unsigned long long qemuCmdFlags; - struct stat sb; - const char **retargv = NULL; - const char **retenv = NULL; - const char **tmp; - virBuffer buf = VIR_BUFFER_INITIALIZER; + virCommandPtr cmd = NULL; char *ret = NULL; int i; @@ -7368,70 +7304,26 @@ static char *qemuDomainXMLToNative(virConnectPtr conn, def->graphics[i]->data.vnc.autoport) def->graphics[i]->data.vnc.port = 5900; } - emulator = def->emulator; - - /* Make sure the binary we are about to try exec'ing exists. - * Technically we could catch the exec() failure, but that's - * in a sub-process so its hard to feed back a useful error - */ - if (stat(emulator, &sb) < 0) { - virReportSystemError(errno, - _("Cannot find QEMU binary %s"), - emulator); - goto cleanup; - } - if (qemudExtractVersionInfo(emulator, + if (qemudExtractVersionInfo(def->emulator, NULL, - &qemuCmdFlags) < 0) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("Cannot determine QEMU argv syntax %s"), - emulator); + &qemuCmdFlags) < 0) goto cleanup; - } if (qemuPrepareMonitorChr(driver, &monConfig, def->name) < 0) goto cleanup; - if (qemudBuildCommandLine(conn, driver, def, - &monConfig, 0, qemuCmdFlags, - &retargv, &retenv, - NULL, NULL, /* Don't want it to create TAP devices */ - NULL, NULL) < 0) { - goto cleanup; - } - - tmp = retenv; - while (*tmp) { - virBufferAdd(&buf, *tmp, strlen(*tmp)); - virBufferAddLit(&buf, " "); - tmp++; - } - tmp = retargv; - while (*tmp) { - virBufferAdd(&buf, *tmp, strlen(*tmp)); - virBufferAddLit(&buf, " "); - tmp++; - } - - if (virBufferError(&buf)) { - virBufferFreeAndReset(&buf); - virReportOOMError(); + if (!(cmd = qemudBuildCommandLine(conn, driver, def, + &monConfig, false, qemuCmdFlags, + NULL, NULL))) goto cleanup; - } - ret = virBufferContentAndReset(&buf); + ret = virCommandToString(cmd); cleanup: qemuDriverUnlock(driver); - for (tmp = retargv ; tmp && *tmp ; tmp++) - VIR_FREE(*tmp); - VIR_FREE(retargv); - - for (tmp = retenv ; tmp && *tmp ; tmp++) - VIR_FREE(*tmp); - VIR_FREE(retenv); + virCommandFree(cmd); virDomainDefFree(def); return ret; } diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index b149ef4..07dde54 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -25,28 +25,30 @@ static struct qemud_driver driver; # define MAX_FILE 4096 static int testCompareXMLToArgvFiles(const char *xml, - const char *cmd, + const char *cmdline, unsigned long long extraFlags, const char *migrateFrom, bool expectError) { char argvData[MAX_FILE]; char *expectargv = &(argvData[0]); + int len; char *actualargv = NULL; - const char **argv = NULL; - const char **qenv = NULL; - const char **tmp = NULL; - int ret = -1, len; + int ret = -1; unsigned long long flags; virDomainDefPtr vmdef = NULL; virDomainChrDef monitor_chr; virConnectPtr conn; char *log = NULL; + virCommandPtr cmd = NULL; if (!(conn = virGetConnect())) goto fail; - if (virtTestLoadFile(cmd, &expectargv, MAX_FILE) < 0) + len = virtTestLoadFile(cmdline, &expectargv, MAX_FILE); + if (len < 0) goto fail; + if (len && expectargv[len - 1] == '\n') + expectargv[len - 1] = '\0'; if (!(vmdef = virDomainDefParseFile(driver.caps, xml, VIR_DOMAIN_XML_INACTIVE))) @@ -85,10 +87,9 @@ static int testCompareXMLToArgvFiles(const char *xml, free(virtTestLogContentAndReset()); - if (qemudBuildCommandLine(conn, &driver, - vmdef, &monitor_chr, 0, flags, - &argv, &qenv, - NULL, NULL, migrateFrom, NULL) < 0) + if (!(cmd = qemudBuildCommandLine(conn, &driver, + vmdef, &monitor_chr, false, flags, + migrateFrom, NULL))) goto fail; if ((log = virtTestLogContentAndReset()) == NULL) @@ -105,36 +106,8 @@ static int testCompareXMLToArgvFiles(const char *xml, virResetLastError(); } - len = 1; /* for trailing newline */ - tmp = qenv; - while (*tmp) { - len += strlen(*tmp) + 1; - tmp++; - } - - tmp = argv; - while (*tmp) { - len += strlen(*tmp) + 1; - tmp++; - } - if ((actualargv = malloc(sizeof(*actualargv)*len)) == NULL) + if (!(actualargv = virCommandToString(cmd))) goto fail; - actualargv[0] = '\0'; - tmp = qenv; - while (*tmp) { - if (actualargv[0]) - strcat(actualargv, " "); - strcat(actualargv, *tmp); - tmp++; - } - tmp = argv; - while (*tmp) { - if (actualargv[0]) - strcat(actualargv, " "); - strcat(actualargv, *tmp); - tmp++; - } - strcat(actualargv, "\n"); if (STRNEQ(expectargv, actualargv)) { virtTestDifference(stderr, expectargv, actualargv); @@ -146,22 +119,7 @@ static int testCompareXMLToArgvFiles(const char *xml, fail: free(log); free(actualargv); - if (argv) { - tmp = argv; - while (*tmp) { - free(*(char**)tmp); - tmp++; - } - free(argv); - } - if (qenv) { - tmp = qenv; - while (*tmp) { - free(*(char**)tmp); - tmp++; - } - free(qenv); - } + virCommandFree(cmd); virDomainDefFree(vmdef); virUnrefConnect(conn); return ret; -- 1.7.3.2

From: Daniel P. Berrange <berrange@redhat.com> * src/uml/uml_conf.c (umlBuildCommandLineChr) (umlBuildCommandLine): Rewrite with virCommand. * src/uml/uml_conf.h (umlBuildCommandLine): Update signature. * src/uml/uml_driver.c (umlStartVMDaemon): Adjust caller. --- v2: new patch (well, Dan started it in May, but this is the first time I've cleaned it up enough to be worth posting) src/uml/uml_conf.c | 163 ++++++++++---------------------------------------- src/uml/uml_conf.h | 10 +-- src/uml/uml_driver.c | 68 ++++----------------- 3 files changed, 48 insertions(+), 193 deletions(-) diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index 0921c81..55b3ef6 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -48,6 +48,7 @@ #include "logging.h" #include "domain_nwfilter.h" #include "files.h" +#include "command.h" #define VIR_FROM_THIS VIR_FROM_UML @@ -307,7 +308,7 @@ error: static char * umlBuildCommandLineChr(virDomainChrDefPtr def, const char *dev, - fd_set *keepfd) + virCommandPtr cmd) { char *ret = NULL; @@ -371,7 +372,7 @@ umlBuildCommandLineChr(virDomainChrDefPtr def, VIR_FORCE_CLOSE(fd_out); return NULL; } - FD_SET(fd_out, keepfd); + virCommandTransferFD(cmd, fd_out); } break; case VIR_DOMAIN_CHR_TYPE_PIPE: @@ -419,109 +420,27 @@ static char *umlNextArg(char *args) * Constructs a argv suitable for launching uml with config defined * for a given virtual machine. */ -int umlBuildCommandLine(virConnectPtr conn, - struct uml_driver *driver, - virDomainObjPtr vm, - fd_set *keepfd, - const char ***retargv, - const char ***retenv) +virCommandPtr umlBuildCommandLine(virConnectPtr conn, + struct uml_driver *driver, + virDomainObjPtr vm) { int i, j; - char memory[50]; struct utsname ut; - int qargc = 0, qarga = 0; - const char **qargv = NULL; - int qenvc = 0, qenva = 0; - const char **qenv = NULL; - char *cmdline = NULL; + virCommandPtr cmd; uname(&ut); -#define ADD_ARG_SPACE \ - do { \ - if (qargc == qarga) { \ - qarga += 10; \ - if (VIR_REALLOC_N(qargv, qarga) < 0) \ - goto no_memory; \ - } \ - } while (0) - -#define ADD_ARG(thisarg) \ - do { \ - ADD_ARG_SPACE; \ - qargv[qargc++] = thisarg; \ - } while (0) - -#define ADD_ARG_LIT(thisarg) \ - do { \ - ADD_ARG_SPACE; \ - if ((qargv[qargc++] = strdup(thisarg)) == NULL) \ - goto no_memory; \ - } while (0) - -#define ADD_ARG_PAIR(key,val) \ - do { \ - char *arg; \ - ADD_ARG_SPACE; \ - if (virAsprintf(&arg, "%s=%s", key, val) < 0) \ - goto no_memory; \ - qargv[qargc++] = arg; \ - } while (0) - - -#define ADD_ENV_SPACE \ - do { \ - if (qenvc == qenva) { \ - qenva += 10; \ - if (VIR_REALLOC_N(qenv, qenva) < 0) \ - goto no_memory; \ - } \ - } while (0) - -#define ADD_ENV(thisarg) \ - do { \ - ADD_ENV_SPACE; \ - qenv[qenvc++] = thisarg; \ - } while (0) - -#define ADD_ENV_LIT(thisarg) \ - do { \ - ADD_ENV_SPACE; \ - if ((qenv[qenvc++] = strdup(thisarg)) == NULL) \ - goto no_memory; \ - } while (0) - -#define ADD_ENV_COPY(envname) \ - do { \ - char *val = getenv(envname); \ - char *envval; \ - ADD_ENV_SPACE; \ - if (val != NULL) { \ - if (virAsprintf(&envval, "%s=%s", envname, val) < 0) \ - goto no_memory; \ - qenv[qenvc++] = envval; \ - } \ - } while (0) - - snprintf(memory, sizeof(memory), "%luK", vm->def->mem.cur_balloon); - - ADD_ENV_LIT("LC_ALL=C"); - - ADD_ENV_COPY("LD_PRELOAD"); - ADD_ENV_COPY("LD_LIBRARY_PATH"); - ADD_ENV_COPY("PATH"); - ADD_ENV_COPY("USER"); - ADD_ENV_COPY("LOGNAME"); - ADD_ENV_COPY("TMPDIR"); - - ADD_ARG_LIT(vm->def->os.kernel); - //ADD_ARG_PAIR("con0", "fd:0,fd:1"); - ADD_ARG_PAIR("mem", memory); - ADD_ARG_PAIR("umid", vm->def->name); - ADD_ARG_PAIR("uml_dir", driver->monitorDir); + cmd = virCommandNew(vm->def->os.kernel); + + virCommandAddEnvPassCommon(cmd); + + //virCommandAddArgPair(cmd, "con0", "fd:0,fd:1"); + virCommandAddArgFormat(cmd, "mem=%luK", vm->def->mem.cur_balloon); + virCommandAddArgPair(cmd, "umid", vm->def->name); + virCommandAddArgPair(cmd, "uml_dir", driver->monitorDir); if (vm->def->os.root) - ADD_ARG_PAIR("root", vm->def->os.root); + virCommandAddArgPair(cmd, "root", vm->def->os.root); for (i = 0 ; i < vm->def->ndisks ; i++) { virDomainDiskDefPtr disk = vm->def->disks[i]; @@ -532,24 +451,26 @@ int umlBuildCommandLine(virConnectPtr conn, goto error; } - ADD_ARG_PAIR(disk->dst, disk->src); + virCommandAddArgPair(cmd, disk->dst, disk->src); } for (i = 0 ; i < vm->def->nnets ; i++) { char *ret = umlBuildCommandLineNet(conn, vm->def->nets[i], i); if (!ret) goto error; - ADD_ARG(ret); + virCommandAddArg(cmd, ret); + VIR_FREE(ret); } for (i = 0 ; i < UML_MAX_CHAR_DEVICE ; i++) { char *ret = NULL; if (i == 0 && vm->def->console) - ret = umlBuildCommandLineChr(vm->def->console, "con", keepfd); + ret = umlBuildCommandLineChr(vm->def->console, "con", cmd); if (!ret) if (virAsprintf(&ret, "con%d=none", i) < 0) goto no_memory; - ADD_ARG(ret); + virCommandAddArg(cmd, ret); + VIR_FREE(ret); } for (i = 0 ; i < UML_MAX_CHAR_DEVICE ; i++) { @@ -559,15 +480,18 @@ int umlBuildCommandLine(virConnectPtr conn, if (vm->def->serials[j]->target.port == i) chr = vm->def->serials[j]; if (chr) - ret = umlBuildCommandLineChr(chr, "ssl", keepfd); + ret = umlBuildCommandLineChr(chr, "ssl", cmd); if (!ret) if (virAsprintf(&ret, "ssl%d=none", i) < 0) goto no_memory; - ADD_ARG(ret); + + virCommandAddArg(cmd, ret); + VIR_FREE(ret); } if (vm->def->os.cmdline) { char *args, *next_arg; + char *cmdline; if ((cmdline = strdup(vm->def->os.cmdline)) == NULL) goto no_memory; @@ -577,41 +501,18 @@ int umlBuildCommandLine(virConnectPtr conn, while (*args) { next_arg = umlNextArg(args); - ADD_ARG_LIT(args); + virCommandAddArg(cmd, args); args = next_arg; } + VIR_FREE(cmdline); } - ADD_ARG(NULL); - ADD_ENV(NULL); - - *retargv = qargv; - *retenv = qenv; - return 0; + return cmd; no_memory: virReportOOMError(); error: - if (qargv) { - for (i = 0 ; i < qargc ; i++) - VIR_FREE((qargv)[i]); - VIR_FREE(qargv); - } - if (qenv) { - for (i = 0 ; i < qenvc ; i++) - VIR_FREE((qenv)[i]); - VIR_FREE(qenv); - } - VIR_FREE(cmdline); - return -1; - -#undef ADD_ARG -#undef ADD_ARG_LIT -#undef ADD_ARG_SPACE -#undef ADD_USBDISK -#undef ADD_ENV -#undef ADD_ENV_COPY -#undef ADD_ENV_LIT -#undef ADD_ENV_SPACE + virCommandFree(cmd); + return NULL; } diff --git a/src/uml/uml_conf.h b/src/uml/uml_conf.h index d8b2349..64df5f7 100644 --- a/src/uml/uml_conf.h +++ b/src/uml/uml_conf.h @@ -31,6 +31,7 @@ # include "domain_conf.h" # include "virterror_internal.h" # include "threads.h" +# include "command.h" # define umlDebug(fmt, ...) do {} while(0) @@ -68,11 +69,8 @@ struct uml_driver { virCapsPtr umlCapsInit (void); -int umlBuildCommandLine (virConnectPtr conn, - struct uml_driver *driver, - virDomainObjPtr dom, - fd_set *keepfd, - const char ***retargv, - const char ***retenv); +virCommandPtr umlBuildCommandLine(virConnectPtr conn, + struct uml_driver *driver, + virDomainObjPtr dom); #endif /* __UML_CONF_H */ diff --git a/src/uml/uml_driver.c b/src/uml/uml_driver.c index 6c28c76..546e960 100644 --- a/src/uml/uml_driver.c +++ b/src/uml/uml_driver.c @@ -812,18 +812,11 @@ static int umlCleanupTapDevices(virConnectPtr conn ATTRIBUTE_UNUSED, static int umlStartVMDaemon(virConnectPtr conn, struct uml_driver *driver, virDomainObjPtr vm) { - const char **argv = NULL, **tmp; - const char **progenv = NULL; - int i, ret; - pid_t pid; + int ret; char *logfile; int logfd = -1; - struct stat sb; - fd_set keepfd; - char ebuf[1024]; umlDomainObjPrivatePtr priv = vm->privateData; - - FD_ZERO(&keepfd); + virCommandPtr cmd = NULL; if (virDomainObjIsActive(vm)) { umlReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -840,7 +833,7 @@ static int umlStartVMDaemon(virConnectPtr conn, * Technically we could catch the exec() failure, but that's * in a sub-process so its hard to feed back a useful error */ - if (stat(vm->def->os.kernel, &sb) < 0) { + if (access(vm->def->os.kernel, X_OK) < 0) { virReportSystemError(errno, _("Cannot find UML kernel %s"), vm->def->os.kernel); @@ -877,67 +870,30 @@ static int umlStartVMDaemon(virConnectPtr conn, return -1; } - if (umlBuildCommandLine(conn, driver, vm, &keepfd, - &argv, &progenv) < 0) { + if (!(cmd = umlBuildCommandLine(conn, driver, vm))) { VIR_FORCE_CLOSE(logfd); virDomainConfVMNWFilterTeardown(vm); umlCleanupTapDevices(conn, vm); return -1; } - tmp = progenv; - while (*tmp) { - if (safewrite(logfd, *tmp, strlen(*tmp)) < 0) - VIR_WARN("Unable to write envv to logfile: %s", - virStrerror(errno, ebuf, sizeof ebuf)); - if (safewrite(logfd, " ", 1) < 0) - VIR_WARN("Unable to write envv to logfile: %s", - virStrerror(errno, ebuf, sizeof ebuf)); - tmp++; - } - tmp = argv; - while (*tmp) { - if (safewrite(logfd, *tmp, strlen(*tmp)) < 0) - VIR_WARN("Unable to write argv to logfile: %s", - virStrerror(errno, ebuf, sizeof ebuf)); - if (safewrite(logfd, " ", 1) < 0) - VIR_WARN("Unable to write argv to logfile: %s", - virStrerror(errno, ebuf, sizeof ebuf)); - tmp++; - } - if (safewrite(logfd, "\n", 1) < 0) - VIR_WARN("Unable to write argv to logfile: %s", - virStrerror(errno, ebuf, sizeof ebuf)); + virCommandWriteArgLog(cmd, logfd); priv->monitor = -1; - ret = virExecDaemonize(argv, progenv, &keepfd, &pid, - -1, &logfd, &logfd, - VIR_EXEC_CLEAR_CAPS, - NULL, NULL, NULL); + virCommandClearCaps(cmd); + virCommandSetOutputFD(cmd, &logfd); + virCommandSetErrorFD(cmd, &logfd); + virCommandDaemonize(cmd); + + ret = virCommandRun(cmd, NULL); VIR_FORCE_CLOSE(logfd); if (ret < 0) goto cleanup; ret = virDomainObjSetDefTransient(driver->caps, vm); cleanup: - /* - * At the moment, the only thing that populates keepfd is - * umlBuildCommandLineChr. We want to close every fd it opens. - */ - for (i = 0; i < FD_SETSIZE; i++) - if (FD_ISSET(i, &keepfd)) { - int tmpfd = i; - VIR_FORCE_CLOSE(tmpfd); - } - - for (i = 0 ; argv[i] ; i++) - VIR_FREE(argv[i]); - VIR_FREE(argv); - - for (i = 0 ; progenv[i] ; i++) - VIR_FREE(progenv[i]); - VIR_FREE(progenv); + virCommandFree(cmd); if (ret < 0) { virDomainConfVMNWFilterTeardown(vm); -- 1.7.3.2

From: Daniel P. Berrange <berrange@redhat.com> --- v2: rearrange to later in the series; no other change. Passes for me with macvtap compilation enabled, so I'm not sure if it still suffers from the same problem as the v1 complaint: https://www.redhat.com/archives/libvir-list/2010-November/msg00834.html src/conf/domain_conf.c | 1 - src/util/hooks.c | 1 - 2 files changed, 0 insertions(+), 2 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3f14cee..9516427 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -42,7 +42,6 @@ #include "c-ctype.h" #include "logging.h" #include "network.h" -#include "macvtap.h" #include "nwfilter_conf.h" #include "ignore-value.h" #include "storage_file.h" diff --git a/src/util/hooks.c b/src/util/hooks.c index 04ca6fa..5ba2036 100644 --- a/src/util/hooks.c +++ b/src/util/hooks.c @@ -33,7 +33,6 @@ #include "virterror_internal.h" #include "hooks.h" #include "util.h" -#include "conf/domain_conf.h" #include "logging.h" #include "memory.h" #include "files.h" -- 1.7.3.2