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

[Peter Krempa]

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.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150224/58a4bda2/attachment-0001.sig>