[libvirt] PATCH: Support SDL configuration for QEMU driver

[Jim Meyering]

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