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

[Kashyap Chamarthy]

On Wed, Feb 25, 2015 at 11:11:37AM +0100, Peter Krempa wrote:
> 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.

Actually I tested that too, and forgot to mention -- validation still
fails the same way, desipte the above ordering.

This patch from Martin Kletzander fixes it:

    https://www.redhat.com/archives/libvir-list/2015-February/msg01022.html

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

Yes, 'i' is what I did to test it.

Thanks.

-- 
/kashyap