On Fri, May 06, 2011 at 01:26:06PM -0400, Cole Robinson wrote:

Similar to virCommandArgAddFormat. We will use this shortly.

v2: Convert virCommandEnvAddPair to use the new function

Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 1 + src/util/command.c | 23 ++++++++++++++++++----- src/util/command.h | 7 +++++++ 3 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 1b22be6..d89b191 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -99,6 +99,7 @@ virCommandAddArgList; virCommandAddArgPair; virCommandAddArgSet; virCommandAddEnvBuffer; +virCommandAddEnvFormat; virCommandAddEnvPair; virCommandAddEnvPass; virCommandAddEnvPassCommon; diff --git a/src/util/command.c b/src/util/command.c index 862a913..ff4869d 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -259,23 +259,26 @@ virCommandNonblockingFDs(virCommandPtr cmd) }

/* - * Add an environment variable to the child - * using separate name & value strings + * Add an environment variable to the child created by a printf-style format */ void -virCommandAddEnvPair(virCommandPtr cmd, const char *name, const char *value) +virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) { char *env; + va_list list;

if (!cmd || cmd->has_error) return;

- if (virAsprintf(&env, "%s=%s", name, value ? value : "") < 0) { + va_start(list, format); + if (virVasprintf(&env, format, list) < 0) { cmd->has_error = ENOMEM; + va_end(list); return; } + va_end(list);

- /* env plus trailing NULL */ + /* Arg plus trailing NULL. */ if (VIR_RESIZE_N(cmd->env, cmd->maxenv, cmd->nenv, 1 + 1) < 0) { VIR_FREE(env); cmd->has_error = ENOMEM; @@ -285,6 +288,16 @@ virCommandAddEnvPair(virCommandPtr cmd, const char *name, const char *value) cmd->env[cmd->nenv++] = env; }

+/* + * Add an environment variable to the child + * using separate name & value strings + */ +void +virCommandAddEnvPair(virCommandPtr cmd, const char *name, const char *value) +{ + virCommandAddEnvFormat(cmd, "%s=%s", name, value); +} +

/* * Add an environment variable to the child diff --git a/src/util/command.h b/src/util/command.h index ff8ccf5..69e9169 100644 --- a/src/util/command.h +++ b/src/util/command.h @@ -98,6 +98,13 @@ void virCommandDaemonize(virCommandPtr cmd); void virCommandNonblockingFDs(virCommandPtr cmd);

/* + * Add an environment variable to the child created by a printf-style format + */ +void +virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_FMT_PRINTF(2, 3); + +/* * Add an environment variable to the child * using separate name & value strings */

ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Fri, May 06, 2011 at 01:26:07PM -0400, Cole Robinson wrote:

v2: Shorten a few virCommand calls s/remain/retain/

Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/lxc/lxc_driver.c | 185 ++++++++++---------------------------------------- 1 files changed, 36 insertions(+), 149 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index b94941d..2537f5d 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1247,134 +1247,58 @@ static int lxcControllerStart(lxc_driver_t *driver, int nveths, char **veths, int appPty, - int logfd) + int logfile) { int i; - int rc; - int largc = 0, larga = 0; - const char **largv = NULL; - int lenvc = 0, lenva = 0; - const char **lenv = NULL; + int ret = -1; char *filterstr; char *outputstr; - char *tmp; - int log_level; - pid_t child; - int status; - fd_set keepfd; - char appPtyStr[30]; - const char *emulator; - - FD_ZERO(&keepfd); - -#define ADD_ARG_SPACE \ - do { \ - if (largc == larga) { \ - larga += 10; \ - if (VIR_REALLOC_N(largv, larga) < 0) \ - goto no_memory; \ - } \ - } while (0) - -#define ADD_ARG(thisarg) \ - do { \ - ADD_ARG_SPACE; \ - largv[largc++] = thisarg; \ - } while (0) - -#define ADD_ARG_LIT(thisarg) \ - do { \ - ADD_ARG_SPACE; \ - if ((largv[largc++] = strdup(thisarg)) == NULL) \ - goto no_memory; \ - } while (0) - -#define ADD_ENV_SPACE \ - do { \ - if (lenvc == lenva) { \ - lenva += 10; \ - if (VIR_REALLOC_N(lenv, lenva) < 0) \ - goto no_memory; \ - } \ - } while (0) - -#define ADD_ENV(thisarg) \ - do { \ - ADD_ENV_SPACE; \ - lenv[lenvc++] = thisarg; \ - } 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; \ - lenv[lenvc++] = envval; \ - } while (0) - -#define ADD_ENV_COPY(envname) \ - do { \ - char *val = getenv(envname); \ - if (val != NULL) { \ - ADD_ENV_PAIR(envname, val); \ - } \ - } while (0) + virCommandPtr cmd;

- /* - * The controller may call ip command, so we have to remain PATH. - */ - ADD_ENV_COPY("PATH"); + cmd = virCommandNew(vm->def->emulator); + + /* The controller may call ip command, so we have to retain PATH. */ + virCommandAddEnvPass(cmd, "PATH");

- log_level = virLogGetDefaultPriority(); - if (virAsprintf(&tmp, "LIBVIRT_DEBUG=%d", log_level) < 0) - goto no_memory; - ADD_ENV(tmp); + virCommandAddEnvFormat(cmd, "LIBVIRT_DEBUG=%d", + virLogGetDefaultPriority());

if (virLogGetNbFilters() > 0) { filterstr = virLogGetFilters(); - if (!filterstr) - goto no_memory; - ADD_ENV_PAIR("LIBVIRT_LOG_FILTERS", filterstr); + if (!filterstr) { + virReportOOMError(); + goto cleanup; + } + + virCommandAddEnvPair(cmd, "LIBVIRT_LOG_FILTERS", filterstr); VIR_FREE(filterstr); }

if (driver->log_libvirtd) { if (virLogGetNbOutputs() > 0) { outputstr = virLogGetOutputs(); - if (!outputstr) - goto no_memory; - ADD_ENV_PAIR("LIBVIRT_LOG_OUTPUTS", outputstr); + if (!outputstr) { + virReportOOMError(); + goto cleanup; + } + + virCommandAddEnvPair(cmd, "LIBVIRT_LOG_OUTPUTS", outputstr); VIR_FREE(outputstr); } } else { - if (virAsprintf(&tmp, "LIBVIRT_LOG_OUTPUTS=%d:stderr", log_level) < 0) - goto no_memory; - ADD_ENV(tmp); + virCommandAddEnvFormat(cmd, + "LIBVIRT_LOG_OUTPUTS=%d:stderr", + virLogGetDefaultPriority()); }

- ADD_ENV(NULL); - - snprintf(appPtyStr, sizeof(appPtyStr), "%d", appPty); - - emulator = vm->def->emulator; - - ADD_ARG_LIT(emulator); - ADD_ARG_LIT("--name"); - ADD_ARG_LIT(vm->def->name); - ADD_ARG_LIT("--console"); - ADD_ARG_LIT(appPtyStr); - ADD_ARG_LIT("--background"); + virCommandAddArgList(cmd, "--name", vm->def->name, "--console", NULL); + virCommandAddArgFormat(cmd, "%d", appPty); + virCommandAddArg(cmd, "--background");

for (i = 0 ; i < nveths ; i++) { - ADD_ARG_LIT("--veth"); - ADD_ARG_LIT(veths[i]); + virCommandAddArgList(cmd, "--veth", veths[i], NULL); }

- ADD_ARG(NULL); - - FD_SET(appPty, &keepfd); - /* now that we know it is about to start call the hook if present */ if (virHookPresent(VIR_HOOK_DRIVER_LXC)) { char *xml = virDomainDefFormat(vm->def, 0); @@ -1391,52 +1315,15 @@ static int lxcControllerStart(lxc_driver_t *driver, goto cleanup; }

- if (virExec(largv, lenv, &keepfd, &child, - -1, &logfd, &logfd, - VIR_EXEC_NONE) < 0) - goto cleanup; - - /* We now wait for the process to exit - the controller - * will fork() itself into the background - waiting for - * it to exit thus guarentees it has written its pidfile - */ - while ((rc = waitpid(child, &status, 0) == -1) && errno == EINTR); - if (rc == -1) { - virReportSystemError(errno, - _("Cannot wait for '%s'"), - largv[0]); - goto cleanup; - } - - if (!(WIFEXITED(status) && WEXITSTATUS(status) == 0)) { - lxcError(VIR_ERR_INTERNAL_ERROR, - _("Container '%s' unexpectedly shutdown during startup"), - largv[0]); - goto cleanup; - } + virCommandPreserveFD(cmd, appPty); + virCommandSetOutputFD(cmd, &logfile); + virCommandSetErrorFD(cmd, &logfile);

-#undef ADD_ARG -#undef ADD_ARG_LIT -#undef ADD_ARG_SPACE -#undef ADD_ENV_SPACE -#undef ADD_ENV_PAIR - - return 0; + ret = virCommandRun(cmd, NULL);

-no_memory: - virReportOOMError(); cleanup: - if (largv) { - for (i = 0 ; i < largc ; i++) - VIR_FREE(largv[i]); - VIR_FREE(largv); - } - if (lenv) { - for (i=0 ; i < lenvc ; i++) - VIR_FREE(lenv[i]); - VIR_FREE(lenv); - } - return -1; + virCommandFree(cmd); + return ret; }

ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Fri, May 06, 2011 at 01:26:08PM -0400, Cole Robinson wrote:

Log the full command line and a timestamp like we do for QEMU

Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/lxc/lxc_driver.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 2537f5d..7aed910 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -56,6 +56,8 @@

#define VIR_FROM_THIS VIR_FROM_LXC

+#define START_POSTFIX ": starting up\n" + #define LXC_NB_MEM_PARAM 3

typedef struct _lxcDomainObjPrivate lxcDomainObjPrivate; @@ -1254,6 +1256,9 @@ static int lxcControllerStart(lxc_driver_t *driver, char *filterstr; char *outputstr; virCommandPtr cmd; + off_t pos = -1; + char ebuf[1024]; + char *timestamp;

cmd = virCommandNew(vm->def->emulator);

@@ -1315,6 +1320,24 @@ static int lxcControllerStart(lxc_driver_t *driver, goto cleanup; }

+ /* Log timestamp */ + if ((timestamp = virTimestamp()) == NULL) { + virReportOOMError(); + goto cleanup; + } + if (safewrite(logfile, timestamp, strlen(timestamp)) < 0 || + safewrite(logfile, START_POSTFIX, strlen(START_POSTFIX)) < 0) { + VIR_WARN("Unable to write timestamp to logfile: %s", + virStrerror(errno, ebuf, sizeof ebuf)); + } + VIR_FREE(timestamp); + + /* Log generated command line */ + 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)); + virCommandPreserveFD(cmd, appPty); virCommandSetOutputFD(cmd, &logfile); virCommandSetErrorFD(cmd, &logfile);

ACK, No equivalent in shutdown though ? Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Fri, May 06, 2011 at 01:26:09PM -0400, Cole Robinson wrote:

Actually execs the argv/env we've generated, replacing the current process. Kind of has a limited usage, but allows us to use virCommand in LXC driver to launch the 'init' process

Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 1 + src/util/command.c | 22 ++++++++++++++++++++++ src/util/command.h | 10 ++++++++++ 3 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d89b191..6e96692 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -106,6 +106,7 @@ virCommandAddEnvPassCommon; virCommandAddEnvString; virCommandClearCaps; virCommandDaemonize; +virCommandExec; virCommandFree; virCommandNew; virCommandNewArgList; diff --git a/src/util/command.c b/src/util/command.c index ff4869d..78586e8 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -980,6 +980,28 @@ cleanup: return ret; }

+/* + * Exec the command, replacing the current process. Meant to be called + * after already forking / cloning, so does not attempt to daemonize or + * preserve any FDs. + * + * Returns -1 on any error executing the command. + * Will not return on success. + */ +int virCommandExec(virCommandPtr cmd) +{ + if (!cmd ||cmd->has_error == ENOMEM) { + virReportOOMError(); + return -1; + } + if (cmd->has_error) { + virCommandError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid use of command API")); + return -1; + } + + return execve(cmd->args[0], cmd->args, cmd->env); +}

/* * Run the command and wait for completion. diff --git a/src/util/command.h b/src/util/command.h index 69e9169..aa5136b 100644 --- a/src/util/command.h +++ b/src/util/command.h @@ -256,6 +256,16 @@ char *virCommandToString(virCommandPtr cmd) ATTRIBUTE_RETURN_CHECK; char *virCommandTranslateStatus(int exitstatus) ATTRIBUTE_RETURN_CHECK;

/* + * Exec the command, replacing the current process. Meant to be called + * after already forking / cloning, so does not attempt to daemonize or + * preserve any FDs. + * + * Returns -1 on any error executing the command. + * Will not return on success. + */ +int virCommandExec(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,

ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Fri, May 06, 2011 at 01:26:10PM -0400, Cole Robinson wrote:

Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/lxc/lxc_container.c | 33 ++++++++++----------------------- 1 files changed, 10 insertions(+), 23 deletions(-)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index af453f3..4b31479 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -54,6 +54,7 @@ #include "veth.h" #include "uuid.h" #include "files.h" +#include "command.h"

#define VIR_FROM_THIS VIR_FROM_LXC

@@ -103,33 +104,19 @@ struct __lxc_child_argv { */ static int lxcContainerExecInit(virDomainDefPtr vmDef) { - char *uuidenv, *nameenv; char uuidstr[VIR_UUID_STRING_BUFLEN]; + virCommandPtr cmd;

virUUIDFormat(vmDef->uuid, uuidstr);

- if (virAsprintf(&uuidenv, "LIBVIRT_LXC_UUID=%s", uuidstr) < 0) { - virReportOOMError(); - return -1; - } - if (virAsprintf(&nameenv, "LIBVIRT_LXC_NAME=%s", vmDef->name) < 0) { - virReportOOMError(); - return -1; - } + cmd = virCommandNew(vmDef->os.init);

- const char *const argv[] = { - vmDef->os.init, - NULL, - }; - const char *const envp[] = { - "PATH=/bin:/sbin", - "TERM=linux", - uuidenv, - nameenv, - NULL, - }; + virCommandAddEnvString(cmd, "PATH=/bin:/sbin"); + virCommandAddEnvString(cmd, "TERM=linux"); + virCommandAddEnvPair(cmd, "LIBVIRT_LXC_UUID", uuidstr); + virCommandAddEnvPair(cmd, "LIBVIRT_LXC_NAME", vmDef->name);

- return execve(argv[0], (char **)argv,(char**)envp); + return virCommandExec(cmd);

ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Fri, May 06, 2011 at 01:26:11PM -0400, Cole Robinson wrote:

That way we can log the 'init' argv for debugging.

Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/lxc/lxc_container.c | 41 +++++++++++++++++++++++++---------------- 1 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 4b31479..8b229ed 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -94,15 +94,14 @@ struct __lxc_child_argv {

/** - * lxcContainerExecInit: + * lxcContainerBuildInitCmd: * @vmDef: pointer to vm definition structure * - * Exec the container init string. The container init will replace then - * be running in the current process + * Build a virCommandPtr for launching the container 'init' process * - * Does not return + * Returns a virCommandPtr */ -static int lxcContainerExecInit(virDomainDefPtr vmDef) +static virCommandPtr lxcContainerBuildInitCmd(virDomainDefPtr vmDef) { char uuidstr[VIR_UUID_STRING_BUFLEN]; virCommandPtr cmd; @@ -116,7 +115,7 @@ static int lxcContainerExecInit(virDomainDefPtr vmDef) virCommandAddEnvPair(cmd, "LIBVIRT_LXC_UUID", uuidstr); virCommandAddEnvPair(cmd, "LIBVIRT_LXC_NAME", vmDef->name);

- return virCommandExec(cmd); + return cmd; }

/** @@ -753,28 +752,34 @@ static int lxcContainerChild( void *data ) lxc_child_argv_t *argv = data; virDomainDefPtr vmDef = argv->config; int ttyfd; + int ret = -1; char *ttyPath; virDomainFSDefPtr root; + virCommandPtr cmd = NULL;

if (NULL == vmDef) { lxcError(VIR_ERR_INTERNAL_ERROR, "%s", _("lxcChild() passed invalid vm definition")); - return -1; + goto cleanup; }

+ cmd = lxcContainerBuildInitCmd(vmDef); + virCommandWriteArgLog(cmd, 1); + root = virDomainGetRootFilesystem(vmDef);

if (root) { if (virAsprintf(&ttyPath, "%s%s", root->src, argv->ttyPath) < 0) { virReportOOMError(); - return -1; + goto cleanup; } } else { if (!(ttyPath = strdup(argv->ttyPath))) { virReportOOMError(); - return -1; + goto cleanup; } } + VIR_DEBUG("Container TTY path: %s", ttyPath);

ttyfd = open(ttyPath, O_RDWR|O_NOCTTY); if (ttyfd < 0) { @@ -782,34 +787,38 @@ static int lxcContainerChild( void *data ) _("Failed to open tty %s"), ttyPath); VIR_FREE(ttyPath); - return -1; + goto cleanup; } VIR_FREE(ttyPath);

if (lxcContainerSetStdio(argv->monitor, ttyfd) < 0) { VIR_FORCE_CLOSE(ttyfd); - return -1; + goto cleanup; } VIR_FORCE_CLOSE(ttyfd);

if (lxcContainerSetupMounts(vmDef, root) < 0) - return -1; + goto cleanup;

/* Wait for interface devices to show up */ if (lxcContainerWaitForContinue(argv->monitor) < 0) - return -1; + goto cleanup;

/* rename and enable interfaces */ if (lxcContainerRenameAndEnableInterfaces(argv->nveths, argv->veths) < 0) - return -1; + goto cleanup;

/* drop a set of root capabilities */ if (lxcContainerDropCapabilities() < 0) - return -1; + goto cleanup;

/* this function will only return if an error occured */ - return lxcContainerExecInit(vmDef); + ret = virCommandExec(cmd); + +cleanup: + virCommandFree(cmd); + return ret; }

ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|