[libvirt] [PATCH v4 3/3] qemu: Automatically create NVRAM store

[Laszlo Ersek]

comments below

On 08/21/14 10:50, Michal Privoznik wrote:
> When using split UEFI image, it may come handy if libvirt manages per
> domain _VARS file automatically. While the _CODE file is RO and can be
> shared among multiple domains, you certainly don't want to do that on
> the _VARS file. This latter one needs to be per domain. So at the
> domain startup process, if it's determined that domain needs _VARS
> file it's copied from this master _VARS file. The location of the
> master file is configurable in qemu.conf.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  libvirt.spec.in                    |   2 +
>  src/Makefile.am                    |   1 +
>  src/qemu/libvirtd_qemu.aug         |   3 +
>  src/qemu/qemu.conf                 |  14 ++++
>  src/qemu/qemu_conf.c               |  93 ++++++++++++++++++++++++++
>  src/qemu/qemu_conf.h               |   5 ++
>  src/qemu/qemu_process.c            | 132 +++++++++++++++++++++++++++++++++++++
>  src/qemu/test_libvirtd_qemu.aug.in |   3 +
>  8 files changed, 253 insertions(+)

I compared this patch with its v2 counterpart. I can see that all of my
remarks have been addressed. Two notes:

> @@ -654,6 +711,42 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg,
>  
>      GET_VALUE_BOOL("log_timestamp", cfg->logTimestamp);
>  
> +    CHECK_TYPE("nvram", VIR_CONF_LIST);
> +    if ((p = virConfGetValue(conf, "nvram"))) {
> +        size_t len;
> +        virConfValuePtr pp;
> +
> +        while (cfg->nloader) {
> +            VIR_FREE(cfg->loader[cfg->nloader - 1]);
> +            VIR_FREE(cfg->nvram[cfg->nloader - 1]);
> +            cfg->nloader--;
> +        }
> +        VIR_FREE(cfg->loader);
> +        VIR_FREE(cfg->nvram);
> +
> +        /* Calc length and check items */
> +        for (len = 0, pp = p->list; pp; len++, pp = pp->next) {
> +            if (pp->type != VIR_CONF_STRING) {
> +                virReportError(VIR_ERR_CONF_SYNTAX, "%s",
> +                               _("nvram must be a list of strings"));
> +                goto cleanup;
> +            }
> +        }
> +
> +        if (len &&
> +            (VIR_ALLOC_N(cfg->loader, len) < 0 ||
> +             VIR_ALLOC_N(cfg->nvram, len) < 0))
> +            goto cleanup;
> +        cfg->nloader = len;
> +
> +        for (i = 0, pp = p->list; pp; i++, pp = pp->next) {
> +            if (virQEMUDriverConfigNVRAMParse(pp->str,
> +                                              &cfg->loader[i],
> +                                              &cfg->nvram[i]) < 0)
> +                goto cleanup;
> +        }
> +    }
> +
>      ret = 0;
>  
>   cleanup:

(a) In general, this looks like a good solution. This prevents freeing
garbage pointers, and also handles the nvram=[] (empty list) case:
nvram=[] will work as if nvram were absent completely. Okay.

(b) However, I think CHECK_TYPE() is used incorrectly:
'p = virConfGetValue(conf, "nvram")' must be done *before* CHECK_TYPE().

You didn't catch this in testing because the value of "p", before you
reach CHECK_TYPE(), has been set by

  p = virConfGetValue(conf, "hugetlbfs_mount");

That is, "p" was most likely NULL in your testing. And p == NULL
short-circuits CHECK_TYPE(), to success.

You need:

    p = virConfGetValue(conf, "nvram");
    CHECK_TYPE("nvram", VIR_CONF_LIST);
    if (p) {

Refer to the "cgroup_controllers" block in
virQEMUDriverConfigLoadFile(), a little bit higher up; that one uses
this same pattern.

The rest seems fine to me.

Thanks!
Laszlo