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@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