9:31 a.m.
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
On Thu, Oct 02, 2008 at 07:37:38PM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange(a)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.