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

[Martin Kletzander]

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) {
    ...
  }

>+            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.

>+        }
>+    }
>+
>+    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.

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150126/b22a2aa3/attachment-0001.sig>