[libvirt] [PATCH] qemu: fix EFI nvram removal on domain undefine

[Pavel Mores]

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 at redhat.com>
> > ---
> >   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 at redhat.com> 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