Re: [PATCH v2 1/8] Make NVRAM a virStorageSource type.

On Fri, Apr 08, 2022 at 10:48:44 -0700, Rohit Kumar wrote: > Currently, libvirt allows only local filepaths to specify > a NVRAM disk. Since, VMs can migrate across hosts, so making Note that this is not strictly needed for migration to work. In fact qemu migratest he contents of the nvram inside the migration stream and flushes it out on the destination, thus migration will work as expected even when the storage is backed locally.

> it to virStorageSource type would help in uninturrupted access > NVRAM disks over network. Thus this statement is not accurate. I understand though that this gives you the flexibility to start the VM on any host without having to worry about where to get the latest nvram image.

> Signed-off-by: Prerna Saxena <prerna.saxena(a)nutanix.com&gt; > Signed-off-by: Florian Schmidt <flosch(a)nutanix.com&gt; > Signed-off-by: Rohit Kumar <rohit.kumar3(a)nutanix.com&gt; > --- > src/conf/domain_conf.c | 13 ++++++++++--- > src/conf/domain_conf.h | 2 +- > src/qemu/qemu_cgroup.c | 3 ++- > src/qemu/qemu_command.c | 2 +- > src/qemu/qemu_domain.c | 14 ++++++++------ > src/qemu/qemu_driver.c | 5 +++-- > src/qemu/qemu_firmware.c | 17 ++++++++++++----- > src/qemu/qemu_namespace.c | 5 +++-- > src/qemu/qemu_process.c | 5 +++-- > src/security/security_dac.c | 6 ++++-- > src/security/security_selinux.c | 6 ++++-- > src/security/virt-aa-helper.c | 5 +++-- > src/vbox/vbox_common.c | 2 +- > 13 files changed, 55 insertions(+), 30 deletions(-) [...] > @@ -18266,7 +18266,11 @@ virDomainDefParseBootLoaderOptions(virDomainDef *def, > fwAutoSelect) < 0) > return -1; > > - def->os.loader->nvram = virXPathString("string(./os/nvram[1])", ctxt); > + if (virXPathNode("./os/nvram[1]", ctxt)) { > + def->os.loader->nvram = g_new0(virStorageSource, 1); [1] This is wrong. virStorageSource is an virObject instance so allocating it like this doesn't initialize the virObject part. You must use virStorageSourceNew exclusively to do this.

> + def->os.loader->nvram->path = virXPathString("string(./os/nvram[1])", ctxt); > + def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE; > + } > if (!fwAutoSelect) > def->os.loader->nvramTemplate = virXPathString("string(./os/nvram[1]/@template)", ctxt); > [...] > diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c > index aa0c927578..d46c9ff36a 100644 > --- a/src/qemu/qemu_cgroup.c > +++ b/src/qemu/qemu_cgroup.c > @@ -581,7 +581,8 @@ qemuSetupFirmwareCgroup(virDomainObj *vm) > return -1; > > if (vm->def->os.loader->nvram && > - qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram, false) < 0) > + vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && [1] This is not future proof, this should rather use 'virStorageSourceIsLocalStorage()' so that it also covers cases when e.g. user deliberately selects a 'block' backed nvram.

> + qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram->path, false) < 0) > return -1; > > return 0; [...] > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index a4334de158..70e96cc5a5 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -4663,8 +4663,12 @@ qemuDomainDefPostParse(virDomainDef *def, > } > > if (virDomainDefHasOldStyleROUEFI(def) && > - !def->os.loader->nvram) > - qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); > + (!def->os.loader->nvram || !def->os.loader->nvram->path)) { > + if (!def->os.loader->nvram) > + def->os.loader->nvram = g_new0(virStorageSource, 1); See [1]

> + def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE; > + qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram->path); > + } > > if (qemuDomainDefAddDefaultDevices(driver, def, qemuCaps) < 0) > return -1; [...] > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 8c0e36e9b2..2b2bd8d20c 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -6615,8 +6615,9 @@ qemuDomainUndefineFlags(virDomainPtr dom, > } > } > > - if (vm->def->os.loader && vm->def->os.loader->nvram) { > - nvram_path = g_strdup(vm->def->os.loader->nvram); > + if (vm->def->os.loader && vm->def->os.loader->nvram && > + vm->def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE) { > + nvram_path = g_strdup(vm->def->os.loader->nvram->path); Consider [2].

> } else if (vm->def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI) { > qemuDomainNVRAMPathFormat(cfg, vm->def, &nvram_path); > } > diff --git a/src/qemu/qemu_firmware.c b/src/qemu/qemu_firmware.c > index 51223faadf..6556a613a8 100644 > --- a/src/qemu/qemu_firmware.c > +++ b/src/qemu/qemu_firmware.c > @@ -1192,13 +1192,16 @@ qemuFirmwareEnableFeatures(virQEMUDriver *driver, > VIR_FREE(def->os.loader->nvramTemplate); > def->os.loader->nvramTemplate = g_strdup(flash->nvram_template.filename); > > - if (!def->os.loader->nvram) > - qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram); > + if (!def->os.loader->nvram) { > + def->os.loader->nvram = g_new0(virStorageSource, 1); See [1].

> + def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE; > + qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram->path); > + } > > VIR_DEBUG("decided on firmware '%s' template '%s' NVRAM '%s'", > def->os.loader->path, > def->os.loader->nvramTemplate, > - def->os.loader->nvram); > + def->os.loader->nvram->path); > break; > > case QEMU_FIRMWARE_DEVICE_KERNEL: > @@ -1364,8 +1367,12 @@ qemuFirmwareFillDomain(virQEMUDriver *driver, > * its path in domain XML) but no template for NVRAM was > * specified and the varstore doesn't exist ... */ > if (!virDomainDefHasOldStyleROUEFI(def) || > - def->os.loader->nvramTemplate || > - (!reset_nvram && virFileExists(def->os.loader->nvram))) > + def->os.loader->nvramTemplate) > + return 0; > + > + if (!reset_nvram && def->os.loader->nvram && > + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE Consider [2]

> + && virFileExists(def->os.loader->nvram->path)) Also put the && on end of previous line as is common in the surrounding code.

> return 0; > > /* ... then we want to consult JSON FW descriptors first, > diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c > index 23681b14a4..18a24635ad 100644 > --- a/src/qemu/qemu_namespace.c > +++ b/src/qemu/qemu_namespace.c > @@ -572,8 +572,9 @@ qemuDomainSetupLoader(virDomainObj *vm, > case VIR_DOMAIN_LOADER_TYPE_PFLASH: > *paths = g_slist_prepend(*paths, g_strdup(loader->path)); > > - if (loader->nvram) > - *paths = g_slist_prepend(*paths, g_strdup(loader->nvram)); > + if (loader->nvram && > + loader->nvram->type == VIR_STORAGE_TYPE_FILE) [2]

> + *paths = g_slist_prepend(*paths, g_strdup(loader->nvram->path)); > break; > > case VIR_DOMAIN_LOADER_TYPE_NONE: > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 2e11d24be2..e53a26c6fd 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -4472,7 +4472,8 @@ qemuPrepareNVRAM(virQEMUDriver *driver, > struct qemuPrepareNVRAMHelperData data; > > if (!loader || !loader->nvram || > - (virFileExists(loader->nvram) && !reset_nvram)) > + loader->nvram->type != VIR_STORAGE_TYPE_FILE || [2]

> + (virFileExists(loader->nvram->path) && !reset_nvram)) > return 0; > > master_nvram_path = loader->nvramTemplate; [...] > diff --git a/src/security/security_dac.c b/src/security/security_dac.c > index e9e316551e..66c36c57a3 100644 > --- a/src/security/security_dac.c > +++ b/src/security/security_dac.c > @@ -1971,7 +1971,8 @@ virSecurityDACRestoreAllLabel(virSecurityManager *mgr, > } > > if (def->os.loader && def->os.loader->nvram && > - virSecurityDACRestoreFileLabel(mgr, def->os.loader->nvram) < 0) > + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && [2]

> + virSecurityDACRestoreFileLabel(mgr, def->os.loader->nvram->path) < 0) > rc = -1; > > if (def->os.kernel && > @@ -2182,8 +2183,9 @@ virSecurityDACSetAllLabel(virSecurityManager *mgr, > } > > if (def->os.loader && def->os.loader->nvram && > + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && [2]

> virSecurityDACSetOwnership(mgr, NULL, > - def->os.loader->nvram, > + def->os.loader->nvram->path, > user, group, true) < 0) > return -1; > > diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c > index 6f02baf2ce..1b6b67e8c7 100644 > --- a/src/security/security_selinux.c > +++ b/src/security/security_selinux.c > @@ -2806,7 +2806,8 @@ virSecuritySELinuxRestoreAllLabel(virSecurityManager *mgr, > } > > if (def->os.loader && def->os.loader->nvram && > - virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram, true) < 0) > + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && [2]

> + virSecuritySELinuxRestoreFileLabel(mgr, def->os.loader->nvram->path, true) < 0) > rc = -1; > > if (def->os.kernel && > @@ -3212,8 +3213,9 @@ virSecuritySELinuxSetAllLabel(virSecurityManager *mgr, > /* This is different than kernel or initrd. The nvram store > * is really a disk, qemu can read and write to it. */ > if (def->os.loader && def->os.loader->nvram && > + def->os.loader->nvram->type == VIR_STORAGE_TYPE_FILE && [2]

> secdef && secdef->imagelabel && > - virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram, > + virSecuritySELinuxSetFilecon(mgr, def->os.loader->nvram->path, > secdef->imagelabel, true) < 0) > return -1; > > diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c > index 1f1cce8b3d..d4eb8b08c9 100644 > --- a/src/security/virt-aa-helper.c > +++ b/src/security/virt-aa-helper.c > @@ -1006,8 +1006,9 @@ get_files(vahControl * ctl) > if (vah_add_file(&buf, ctl->def->os.loader->path, "rk") != 0) > goto cleanup; > > - if (ctl->def->os.loader && ctl->def->os.loader->nvram) > - if (vah_add_file(&buf, ctl->def->os.loader->nvram, "rwk") != 0) > + if (ctl->def->os.loader && ctl->def->os.loader->nvram && > + ctl->def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE) [2]

> + if (vah_add_file(&buf, ctl->def->os.loader->nvram->path, "rwk") != 0) > goto cleanup; > > for (i = 0; i < ctl->def->ngraphics; i++) { > diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c > index 34e555644c..214d5e84b8 100644 > --- a/src/vbox/vbox_common.c > +++ b/src/vbox/vbox_common.c > @@ -992,7 +992,7 @@ vboxSetBootDeviceOrder(virDomainDef *def, struct _vboxDriver *data, > VIR_DEBUG("def->os.loader->path %s", def->os.loader->path); > VIR_DEBUG("def->os.loader->readonly %d", def->os.loader->readonly); > VIR_DEBUG("def->os.loader->type %d", def->os.loader->type); > - VIR_DEBUG("def->os.loader->nvram %s", def->os.loader->nvram); > + VIR_DEBUG("def->os.loader->nvram->path %s", def->os.loader->nvram->path); Is 'def->os.loader->nvram' guaranteed to be allocated here? You are dereferencing it unconditionally. Previously the code was technically still wrong, but luckily 'printf' is tollerating NULL strings.

> } > VIR_DEBUG("def->os.bootloader %s", def->os.bootloader); > VIR_DEBUG("def->os.bootloaderArgs %s", def->os.bootloaderArgs); > -- > 2.25.1 >