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).

On Tue, Feb 24, 2015 at 15:11:28 +0000, Daniel Berrange wrote: > 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. In that case we probably should have negated the logic here and delete all the stuff by default and give the user option to leave the data behind. The original motivation is apparently that we should not allow anything that would represent state of the deleted VM to be transferred accidentaly to a new VM with same name. For the save image or snapshots the risk of persisting any data is low as a save image would not function without it's disk and still be somewhat secure as it would contain the whole memory image including security. For the NVRAM though it might uncover data stored there or even make the VM unbootable. I agree that the current state is not ideal as we basically force the user to specify all the necessary flags. I think we can safely avoid displaying the message in cases when it's not stored in the libvirt-internal path but for the internal path I'm not convinced that it would be a great idea to change the default. We can perhaps add a flag that woult either enable all the "UNDEFINE*" flags or perhaps invert the logic of them so the user could leave them behind.

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)

On Tue, Feb 24, 2015 at 17:46:12 +0100, Kashyap Chamarthy wrote: > On Tue, Feb 24, 2015 at 10:12:20AM +0000, Daniel P. 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) > > With the above patch applied, I built libvirt RPMs, installed and > restarted libvirtd then I tried the below test (which failed) an AArch64 > Fedora21 guest. > > Edit the Fedora 21 libvirt guest. Guest XML here[*]: > > $ virsh edit devstack > [. . .] > > Try to add the below fragment under 'os' element: > > <loader readonly='yes' type='pflash'>/usr/share/AAVMF/AAVMF_CODE.fd</loader> > <nvram>/var/lib/libvirt/nvram/fedora-21-aarch64-nvram</nvram> > > It fails to validate the XML: > > error: XML document failed to validate against schema: Unable to validate doc against /usr/share/libvirt/schemas/domain.rng > Extra element devices in interleave > Element domain failed to validate content > > Failed. Try again? [y,n,i,f,?]: > The <loader> and <nvram> need to be put after the <os><type ... > element due to the ordering in libvirt's schema.

I'll look into the schema whether we can make the interleave span the <type> element too. Until then either put it in the right place or use the 'i' key to ignore the schema check.

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)

On 23.05.2016 11:16, Maxim Nestratov wrote: > 24.02.2015 13:12, Daniel P. Berrange пишет: > >> 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) > As I found out the discussion followed this patch didn't come to a > conclusion and this or any other patches on the matter weren't commited. > We hit the problem with inability to undefine a domain leaving nvram > untouched recently and this patch would solve it perfectly. > I think it's worth commiting IMHO and maybe the documentation should > reflect this slight change in behavior. > Any new thoughts? Does this Cole's suggestion sounds reasonable? https://www.redhat.com/archives/libvir-list/2015-February/msg00974.html Michal