On Tue, Oct 15, 2019 at 01:51:32PM +0200, Michal Privoznik wrote: > 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@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxx> and pushed. Cheers! One question though - the pushed version duplicates the path as you mention in [1] above, and if the strdup fails it does 'goto endjob'. This means that a failure in an avoidable string copy operation might make the whole domain undefinition fail, making this formulation slightly less robust I believe. Now, this is probably mostly theoretical, with no effect in practice. I thought though I'd mention it in case there was a corner case after all where this could make a difference. Thanks, pvl -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
