Re: [PATCH v2 1/8] Make NVRAM a virStorageSource type.
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
Signed-off-by: Florian Schmidt
Signed-off-by: Rohit Kumar
---
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, >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, >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, _path);
}
diff --git a/src/qemu/qemu_firmware.c
Re: [PATCH v2 1/8] Make NVRAM a virStorageSource type.
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
Signed-off-by: Florian Schmidt
Signed-off-by: Rohit Kumar
---
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, >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, >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, _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
+++
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
> Signed-off-by: Florian Schmidt
> Signed-off-by: Rohit Kumar
> ---
> 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, >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, >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, _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);
>
[PATCH v2 1/8] Make NVRAM a virStorageSource type.
Currently, libvirt allows only local filepaths to specify
a NVRAM disk. Since, VMs can migrate across hosts, so making
it to virStorageSource type would help in uninturrupted access
NVRAM disks over network.
Signed-off-by: Prerna Saxena
Signed-off-by: Florian Schmidt
Signed-off-by: Rohit Kumar
---
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(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 5dd269b283..b83c2f0e6a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -3540,7 +3540,7 @@ virDomainLoaderDefFree(virDomainLoaderDef *loader)
return;
g_free(loader->path);
-g_free(loader->nvram);
+virObjectUnref(loader->nvram);
g_free(loader->nvramTemplate);
g_free(loader);
}
@@ -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);
+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);
@@ -26971,7 +26975,10 @@ virDomainLoaderDefFormat(virBuffer *buf,
virXMLFormatElementInternal(buf, "loader", ,
, false, false);
virBufferEscapeString(, " template='%s'",
loader->nvramTemplate);
-virBufferEscapeString(, "%s", loader->nvram);
+if (loader->nvram) {
+if (loader->nvram->type == VIR_STORAGE_TYPE_FILE)
+virBufferEscapeString(, "%s", loader->nvram->path);
+}
virXMLFormatElementInternal(buf, "nvram", , ,
false, false);
}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 694491cd63..eecee1075c 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -2234,7 +2234,7 @@ struct _virDomainLoaderDef {
virTristateBool readonly;
virDomainLoader type;
virTristateBool secure;
-char *nvram;/* path to non-volatile RAM */
+virStorageSource *nvram;
char *nvramTemplate; /* user override of path to master nvram */
};
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 &&
+qemuSetupImagePathCgroup(vm, vm->def->os.loader->nvram->path, false) <
0)
return -1;
return 0;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index bb45954108..1393d4fc22 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -9648,7 +9648,7 @@ qemuBuildDomainLoaderPflashCommandLine(virCommand *cmd,
if (loader->nvram) {
virBufferAddLit(, "file=");
-virQEMUBuildBufferEscapeComma(, loader->nvram);
+virQEMUBuildBufferEscapeComma(, loader->nvram->path);
virBufferAsprintf(, ",if=pflash,format=raw,unit=%d", unit);
virCommandAddArg(cmd, "-drive");
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, >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);
+def->os.loader->nvram->type = VIR_STORAGE_TYPE_FILE;
+qemuDomainNVRAMPathFormat(cfg, def, >os.loader->nvram->path);
+}
if (qemuDomainDefAddDefaultDevices(driver, def, qemuCaps) < 0)
return -1;
@@ -11349,11 +11353,9 @@ qemuDomainInitializePflashStorageSource(virDomainObj
*vm)
if (def->os.loader->nvram) {
-pflash1 = virStorageSourceNew();
-pflash1->type = VIR_STORAGE_TYPE_FILE;
+if
