On 10/15/19 10:31 AM, Pavel Mores wrote:
When undefining a UEFI domain its nvram file has to be properly handled as
well. It's mandatory to use one of --nvram and --keep-nvram options when
'virsh undefine <domain>' is issued for a UEFI domain. To fix the bug as
reported, virsh should return an error message if neither option is used
and the nvram file should be removed when --nvram is given.
The cause of the problem is that when qemuDomainUndefineFlags() is invoked
on an inactive domain the path to its nvram file is empty. This commit
aims to fix this by formatting and filling in the path in time for the
nvram removal code to run properly.
https://bugzilla.redhat.com/show_bug.cgi?id=1751596
Signed-off-by: Pavel Mores <pmo...@redhat.com>
---
src/qemu/qemu_domain.c | 12 +++++++++---
src/qemu/qemu_domain.h | 4 ++++
src/qemu/qemu_driver.c | 20 +++++++++++++++-----
3 files changed, 28 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 35067c851f..1a0367cc27 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -15441,16 +15441,22 @@
qemuDomainDiskIsMissingLocalOptional(virDomainDiskDefPtr disk)
}
+int
+qemuDomainNVRAMPathFormat(virQEMUDriverConfigPtr cfg,
+ virDomainDefPtr def, char **path)
+{
+ return virAsprintf(path, "%s/%s_VARS.fd", cfg->nvramDir, def->name);
+}
+
We have two empty lines between functions in this file. And Also, one
argument per line please.
int
qemuDomainNVRAMPathGenerate(virQEMUDriverConfigPtr cfg,
virDomainDefPtr def)
{
if (def->os.loader &&
def->os.loader->type == VIR_DOMAIN_LOADER_TYPE_PFLASH &&
- def->os.loader->readonly == VIR_TRISTATE_SWITCH_ON &&
+ def->os.loader->readonly == VIR_TRISTATE_BOOL_YES &&
!def->os.loader->nvram) {
- return virAsprintf(&def->os.loader->nvram, "%s/%s_VARS.fd",
- cfg->nvramDir, def->name);
+ return qemuDomainNVRAMPathFormat(cfg, def, &def->os.loader->nvram);
}
return 0;
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 01a54d4265..07725c7cc4 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -1207,6 +1207,10 @@ qemuDomainIsUsingNoShutdown(qemuDomainObjPrivatePtr
priv);
bool
qemuDomainDiskIsMissingLocalOptional(virDomainDiskDefPtr disk);
+int
+qemuDomainNVRAMPathFormat(virQEMUDriverConfigPtr cfg,
+ virDomainDefPtr def, char **path);
+
int
qemuDomainNVRAMPathGenerate(virQEMUDriverConfigPtr cfg,
virDomainDefPtr def);
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index bc0ede2fb0..dcaadbdb52 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7828,6 +7828,8 @@ qemuDomainUndefineFlags(virDomainPtr dom,
int nsnapshots;
int ncheckpoints;
virQEMUDriverConfigPtr cfg = NULL;
+ char *nvram_path = NULL;
+ bool need_deallocate_nvram_path = false;
While this works, I'd rather have @nvram_path autofreed, and ... [1]
virCheckFlags(VIR_DOMAIN_UNDEFINE_MANAGED_SAVE |
VIR_DOMAIN_UNDEFINE_SNAPSHOTS_METADATA |
@@ -7905,14 +7907,20 @@ qemuDomainUndefineFlags(virDomainPtr dom,
}
}
- if (vm->def->os.loader &&
- vm->def->os.loader->nvram &&
- virFileExists(vm->def->os.loader->nvram)) {
+ if (vm->def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI) {
+ qemuDomainNVRAMPathFormat(cfg, vm->def, &nvram_path);
This needs a check for retval.
+ need_deallocate_nvram_path = true;
+ } else {
+ if (vm->def->os.loader)
+ nvram_path = vm->def->os.loader->nvram;
1: ... duplicate path here.
+ }
+
+ if (nvram_path && virFileExists(nvram_path)) {
if ((flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
- if (unlink(vm->def->os.loader->nvram) < 0) {
+ if (unlink(nvram_path) < 0) {
virReportSystemError(errno,
_("failed to remove nvram: %s"),
- vm->def->os.loader->nvram);
+ nvram_path);
goto endjob;
}
} else if (!(flags & VIR_DOMAIN_UNDEFINE_KEEP_NVRAM)) {
@@ -7948,6 +7956,8 @@ qemuDomainUndefineFlags(virDomainPtr dom,
virDomainObjEndAPI(&vm);
virObjectEventStateQueue(driver->domainEventState, event);
virObjectUnref(cfg);
+ if (need_deallocate_nvram_path)
+ VIR_FREE(nvram_path);
return ret;
}
Reviewed-by: Michal Privoznik <mpriv...@redhat.com> and pushed.
