[libvirt] [PATCH 1/3] command: Add virCommandEnvAddFormat
Similar to virCommandArgAddFormat. We will use this shortly. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 1 + src/util/command.c | 30 ++++++++++++++++++++++++++++++ src/util/command.h | 7 +++++++ 3 files changed, 38 insertions(+), 0 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..f382cc5 100644 --- a/src/util/command.c +++ b/src/util/command.c @@ -259,6 +259,36 @@ virCommandNonblockingFDs(virCommandPtr cmd) } /* + * Add an environment variable to the child created by a printf-style format + */ +void +virCommandAddEnvFormat(virCommandPtr cmd, const char *format, ...) +{ + char *env; + va_list list; + + if (!cmd || cmd->has_error) + return; + + va_start(list, format); + if (virVasprintf(&env, format, list) < 0) { + cmd->has_error = ENOMEM; + va_end(list); + return; + } + va_end(list); + + /* Arg 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 separate name & value strings */ 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 */ -- 1.7.4.4
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/lxc/lxc_driver.c | 186 ++++++++++---------------------------------------- 1 files changed, 37 insertions(+), 149 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index b94941d..930e445 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1247,134 +1247,59 @@ 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 = virCommandNewArgList(vm->def->emulator, NULL); + + /* The controller may call ip command, so we have to remain 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, NULL); + virCommandAddArg(cmd, "--console"); + 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 +1316,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; } -- 1.7.4.4
On 05/05/2011 02:44 PM, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/lxc/lxc_driver.c | 186 ++++++++++---------------------------------------- 1 files changed, 37 insertions(+), 149 deletions(-)
I love diffstats like this. Isn't it fun how much cruft virCommand removes?
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index b94941d..930e445 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1247,134 +1247,59 @@ static int lxcControllerStart(lxc_driver_t *driver, int nveths, char **veths, int appPty, - int logfd) + int logfile)
Why the rename here? But not a show-stopper.
- /* - * The controller may call ip command, so we have to remain PATH. - */ - ADD_ENV_COPY("PATH"); + cmd = virCommandNewArgList(vm->def->emulator, NULL);
Why not just: cmd = virCommandNew(vm->def->emulator);
+ + /* The controller may call ip command, so we have to remain PATH. */
Pre-existing typo, but s/remain/retain/
+ 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());
Aha - the new API from the last patch now in use.
- - 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, NULL); + virCommandAddArg(cmd, "--console");
Why not merge these two lines: virCommandAddArgList(cmd, "--name", vm->def->name, "--console", NULL); ACK with those nits fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/05/2011 05:29 PM, Eric Blake wrote:
On 05/05/2011 02:44 PM, Cole Robinson wrote:
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/lxc/lxc_driver.c | 186 ++++++++++---------------------------------------- 1 files changed, 37 insertions(+), 149 deletions(-)
I love diffstats like this. Isn't it fun how much cruft virCommand removes?
diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index b94941d..930e445 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1247,134 +1247,59 @@ static int lxcControllerStart(lxc_driver_t *driver, int nveths, char **veths, int appPty, - int logfd) + int logfile)
Why the rename here? But not a show-stopper.
I renamed it so I could just copy the logging code from the qemu driver directly. I'll resend with the other comments addressed. Thanks, Cole
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 930e445..5e164b6 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 = virCommandNewArgList(vm->def->emulator, NULL); @@ -1316,6 +1321,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); -- 1.7.4.4
On 05/05/2011 02:44 PM, 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(-)
+ /* 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);
Hmm, I know you just copied qemu_process.c, but should we be doing best effort to proceed (and just skip the timestamp output) if we are low on memory, rather than flat out giving up? But changing that should be a separate patch, since qemu should also be changed, so: ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 05/05/2011 02:44 PM, Cole Robinson wrote:
Similar to virCommandArgAddFormat. We will use this shortly.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/libvirt_private.syms | 1 + src/util/command.c | 30 ++++++++++++++++++++++++++++++ src/util/command.h | 7 +++++++ 3 files changed, 38 insertions(+), 0 deletions(-)
Looks fine as-is, but now that we have it, I'm wondering if we should simplify: void virCommandAddEnvPair(virCommandPtr cmd, const char *name, const char *value) { virCommandAddEnvFormat(cmd, "%s=%s", name, value); } ACK -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Cole Robinson -
Eric Blake