[libvirt] [PATCH v4 1/2] qemu: Allow UEFI paths to be specified at compile time

[Michal Privoznik]

On 26.01.2015 10:34, Martin Kletzander wrote:
> On Wed, Jan 21, 2015 at 08:21:28PM +0100, Michal Privoznik wrote:
>> Up until now there are just two ways how to specify UEFI paths to
>> libvirt. The first one is editing qemu.conf, the other is editing
>> qemu_conf.c and recompile which is not that fancy. So, new
>> configure option is introduced: --with-loader-nvram which takes a
>> list of pairs of UEFI firmware and NVRAM store. This way, the
>> compiled in defaults can be passed during compile time without
>> need to change the code itself.
>>
>> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>> ---
>> configure.ac           | 12 ++++++++++++
>> src/qemu/qemu_conf.c   | 50
>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>> tests/domaincapstest.c | 15 ++++++++-------
>> 3 files changed, 70 insertions(+), 7 deletions(-)
>>
>> diff --git a/configure.ac b/configure.ac
>> index f370475..7153fa5 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -2789,6 +2789,18 @@ AC_ARG_WITH([default-editor],
>>   [DEFAULT_EDITOR=vi])
>> AC_DEFINE_UNQUOTED([DEFAULT_EDITOR], ["$DEFAULT_EDITOR"], [Default
>> editor to use])
>>
>> +AC_ARG_WITH([loader-nvram],
>> +  [AS_HELP_STRING([--with-loader-nvram],
>> +    [Pass list of pairs of <loader>:<nvram> paths. Both
>> +     pairs and list items are separated by a colon.
>> +     @<:default=paths to OVMF and its clones@:>@])],
>> +     [if test "$withval" = "no"; then
>> +        withval=""
>> +      fi
>> +      AC_DEFINE_UNQUOTED([DEFAULT_LOADER_NVRAM],
>> +                          ["$withval"],
>> +                          [List of laoder:nvram pairs])])
>> +
>> # Some GNULIB base64 symbols clash with a kerberos library
>> AC_DEFINE_UNQUOTED([isbase64],[libvirt_gl_isbase64],[Hack to avoid
>> symbol clash])
>> AC_DEFINE_UNQUOTED([base64_encode],[libvirt_gl_base64_encode],[Hack to
>> avoid symbol clash])
>> diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>> index a24c5c5..3e51b49 100644
>> --- a/src/qemu/qemu_conf.c
>> +++ b/src/qemu/qemu_conf.c
>> @@ -107,6 +107,49 @@ void
>> qemuDomainCmdlineDefFree(qemuDomainCmdlineDefPtr def)
>>     VIR_FREE(def);
>> }
>>
>> +
>> +static int ATTRIBUTE_UNUSED
>> +virQEMUDriverConfigLoaderNVRAMParse(virQEMUDriverConfigPtr cfg,
>> +                                    const char *list)
>> +{
>> +    int ret = -1;
>> +    char **token;
>> +    size_t i;
>> +
>> +    if (!(token = virStringSplit(list, ":", 0)))
>> +        goto cleanup;
>> +
>> +    for (i = 0; token[i]; i += 2) {
>> +        if (!token[i] || !token[i + 1] ||
> 
> Double check for "token[i]"; it'd be easier to do:
> 
>  for (i = 0; token[i] && token[i + 1]; i += 2) {
>    ...
>  }

Well, I put the check there to mark it explicitly that both values are
required. I hoped it will be optimized out, but whatever. Your code
won't work in case user had supplied an odd number of tokens. Which btw
is what the loop body tries to find out too. Therefore I'd rather keep
the code as is.

> 
>> +            STREQ(token[i], "") || STREQ(token[i + 1], "")) {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                           _("Invalid --with-loader-nvram list: %s"),
>> +                           list);
>> +            goto cleanup;
> 
> But erroring out here is maybe even more ugly than that bash parsing
> in configure.ac :(  There really is no other option to check this
> built-time?  Can we at least do something like:
> 
>  l="$(echo "$with_loader_nvram" | tr ':' '\n' | wc -l)"
>  test $((l % 2)) -eq 0 || echo ERROR
> 
> this only checks that there's even number of parameters when split by
> ':', but it works in dash (and csh should be fine too, but I haven't
> tested that).  And the only thing you were checking was the number of
> parameters, so this has the same meaning and it's done compile-time.
> 

Whatever, I don't care anymore.

>> +        }
>> +    }
>> +
>> +    if (i) {
>> +        if (VIR_ALLOC_N(cfg->loader, (i + 1) / 2) < 0 ||
>> +            VIR_ALLOC_N(cfg->nvram, (i + 1) / 2) < 0)
>> +            goto cleanup;
>> +        cfg->nloader = (i + 1) / 2;
>> +
>> +        while (i) {
>> +            if (VIR_STRDUP(cfg->loader[(i - 1) / 2 ], token[i - 2]) <
>> 0 ||
>> +                VIR_STRDUP(cfg->nvram[(i - 1) / 2], token[i - 1]) < 0)
> 
> You are not checking whether token[i] is '\0', but I don't know
> whether that breaks later in the code or not.

I don't see what good will the double check bring. I mean, tokens are
checked for empty strings just in the previous 'for' loop that you've
pointed out.

> 
> Anyway, if something like this happens, I think working around that
> and just printing a warning would be better.  You can behave like if
> the file does not exist -- we don't quit the daemon/driver in that
> case, do we?

No. This is different. If either loader or nvram file does not exists,
it's okay and code just handles that perfectly. However, if a token is
an empty string it means that the list of defaults was mangled and I
think that's something that deserves an explicit error message.

Michal