[PATCH v2 1/8] Make NVRAM a virStorageSource type.
Rohit Kumar
rohit.kumar3 at nutanix.com
Thu Apr 21 14:29:19 UTC 2022
Thanks for the review, Peter!
Can you please look at patch 2nd of this patch series too ? It will be
easier if I make all the necessary changes, if any, at once.
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.
Thanks, I will update it.
>
>> 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.
Ack.
>
>> 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]
Ack.
>
> 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]
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.
Sure, Ack.
>
>> 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.
Right, I will add the check for def->os.loader->nvram existance, 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