[PATCH v2 1/8] Make NVRAM a virStorageSource type.
Rohit Kumar
rohit.kumar3 at nutanix.com
Tue Apr 26 10:46:35 UTC 2022
Thanks for reviewing the patches !
On 20/04/22 6:18 pm, Peter Krempa wrote:
> 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.
Right, this does not help with migration. This will provide us
flexibilty to access NVRAM disk over network,
just like libvirt support disks over network. I will update the commit
message.
>
>
>> 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.
Yes, right. I will update it in the next patch.
>
>> Signed-off-by: Prerna Saxena <prerna.saxena at nutanix.com>
>> Signed-off-by: Florian Schmidt <flosch at nutanix.com>
>> Signed-off-by: Rohit Kumar <rohit.kumar3 at nutanix.com>
>> ---
>> 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.
Ack. Thanks!
>> + 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.
Sure, thanks! I will update it.
>
>> + 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]
Ack.
>
>> + 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].
Ack.
>> } 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].
Ack.
>
>> + 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]
Ack.
>
>> + && virFileExists(def->os.loader->nvram->path))
> Also put the && on end of previous line as is common in the surrounding
> code.
Ack. Thanks!
>> 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]
Ack.
>
>> + *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]
Ack.
>
>> + (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]
Ack.
>
>> + 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]
Ack.
>
>> 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]
Ack.
>
>> + 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]
Ack.
>
>> 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]
Ack.
>
>> + 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.
Yes, right. I will add an if condition before accessing nvram->path.
Thanks!
>
>> }
>> VIR_DEBUG("def->os.bootloader %s", def->os.bootloader);
>> VIR_DEBUG("def->os.bootloaderArgs %s", def->os.bootloaderArgs);
>> --
>> 2.25.1
>>
More information about the libvir-list
mailing list