"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Thu, Oct 02, 2008 at 07:37:38PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
Now previously since we just use 'execv' the QEMU process would just inherit all libvirtd's environment variables. When we now use execve() no variables are inherited - we have to explicitly set all the ones we need. I'm not sure what we should consider the mimimum required?
I'm merely setting 'LC_ALL=C' to ensure it runs in C locale. Do we need to set $PATH for QEMU - maybe ? Anything else which is good practice to set ?
If QEMU uses PATH, then propagating that is necessary. I guess it's debatable whether to use PATH=$PATH or to use some hard-coded default on the RHS. But using PATH=$PATH seems friendlier, in case whatever QEMU uses is in some non-default location.
If it uses mkstemp or the like, then including TMPDIR would be good. Depending on QEMU, maybe things like HOME, USER, LOGNAME too.
Here's an update which sets those, if they're present in libvirtd env. The changed bit is here:
+ ADD_ENV_COPY("LD_PRELOAD"); + ADD_ENV_COPY("LD_LIBRARY_PATH");
Looks good. I guess you're adding those two in case qemu uses dlopen. This time I've reviewed the rest of the patch. Comments below:
+ ADD_ENV_COPY("PATH"); + ADD_ENV_COPY("HOME"); + ADD_ENV_COPY("USER"); + ADD_ENV_COPY("LOGNAME"); + ADD_ENV_COPY("TMPDIR"); ... diff --git a/src/qemu_conf.c b/src/qemu_conf.c +#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); \ + ADD_ENV_SPACE; \ + if (val != NULL && \ + (qenv[qenvc++] = strdup(val)) == NULL) \ + goto no_memory; \ + } while (0)
Doesn't this need to be adding "envname=val" strings, rather than just "val"? If it works as-is, then maybe none of these is actually used (or at least they're not exercised in tests).
snprintf(memory, sizeof(memory), "%lu", vm->def->memory/1024); snprintf(vcpus, sizeof(vcpus), "%lu", vm->def->vcpus); + + 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");
emulator = vm->def->emulator;
...
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -22,11 +22,14 @@ static struct qemud_driver driver;
#define MAX_FILE 4096
-static int testCompareXMLToArgvFiles(const char *xml, const char *cmd, int extraFlags) { +static int testCompareXMLToArgvFiles(const char *xml, ... + tmp = qenv; + len = 0; + while (*tmp) { + if (actualargv[0]) + strcat(actualargv, " "); + strcat(actualargv, *tmp); + tmp++; + } tmp = argv; len = 0;
No big deal, but both of those "len = 0" assignments can be removed, since "len" is no longer used.