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

Michal Privoznik mprivozn at redhat.com
Tue Oct 15 11:51:32 UTC 2019


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.

Michal




More information about the libvir-list mailing list