Re: [libvirt] [PATCH] qemu: fix EFI nvram removal on domain undefine

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 <pmores(a)redhat.com&gt; > --- > 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 <mprivozn(a)redhat.com&gt; and pushed.