"Daniel P. Berrange" <berrange@xxxxxxxxxx> wrote:
> On Thu, Oct 02, 2008 at 07:37:38PM +0200, Jim Meyering wrote:
>> "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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.
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list