Re: [libvirt] [PATCH] qemu: don't refuse to undefine a guest with NVRAM file

On Tue, Feb 24, 2015 at 04:07:02PM +0100, Peter Krempa wrote: > On Tue, Feb 24, 2015 at 10:12:20 +0000, Daniel Berrange wrote: > > The undefine operation should always be allowed to succeed > > regardless of whether any NVRAM file exists. ie we should > > not force the application to use the VIR_DOMAIN_UNDEFINE_NVRAM > > flag. It is valid for the app to decide it wants the NVRAM > > file left on disk, in the same way that disk images are left > > on disk at undefine. > > --- > > src/qemu/qemu_driver.c | 20 +++++++------------- > > 1 file changed, 7 insertions(+), 13 deletions(-) > > > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > > index bec05d4..302bf48 100644 > > --- a/src/qemu/qemu_driver.c > > +++ b/src/qemu/qemu_driver.c > > @@ -6985,19 +6985,13 @@ qemuDomainUndefineFlags(virDomainPtr dom, > > > > if (!virDomainObjIsActive(vm) && > > vm->def->os.loader && vm->def->os.loader->nvram && > > - virFileExists(vm->def->os.loader->nvram)) { > > - if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) { > > - virReportError(VIR_ERR_OPERATION_INVALID, "%s", > > - _("cannot delete inactive domain with nvram")); > > - goto cleanup; > > - } > > - > > - if (unlink(vm->def->os.loader->nvram) < 0) { > > - virReportSystemError(errno, > > - _("failed to remove nvram: %s"), > > - vm->def->os.loader->nvram); > > - goto cleanup; > > - } > > + virFileExists(vm->def->os.loader->nvram) && > > + (flags & VIR_DOMAIN_UNDEFINE_NVRAM) && > > + (unlink(vm->def->os.loader->nvram) < 0)) { > > + virReportSystemError(errno, > > + _("failed to remove nvram: %s"), > > + vm->def->os.loader->nvram); > > + goto cleanup; > > } > > > > if (virDomainDeleteConfig(cfg->configDir, cfg->autostartDir, vm) < 0) > > > We have prior art in denying to undefine a domain that has information > stored in libvirt-internal locations such as the managed save image and > snapshot metadata. > > While it makes sense to allow removing the VM without deleting the NVRAM > file when the user specified an external path, we should avoid doing so > if the NVRAM is in the libvirt managed path. (Same with externaly > managed snapshots or save images). I'm not a real fan of refusal in the managed save case either. The issue is that we're forcing a policy onto the application and not giving it any chance to define a different policy. ie currently apps have a choice of getting an error, or always deleting the nvram, not giving them any chance to delete guest without deleting nvram which is a valid choice they should be permitted to have. By removing this check we allow apps to make their own policy decision with their more complete view of the world. I don't think the quesiton of managed vs unmanaged storage should even come into it either, because that presupposes the application will only ever use managed storage. OpenStack does not currently use libvirt storage pools at all but does wish to use NVRAM. We should not deny the option to undefine the guest in this case.