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@xxxxxxxxxx>
> ---
> 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list