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

Pavel Mores posted 1 patch 4 years, 5 months ago
Test syntax-check passed
Failed in applying to current master (apply log)
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(-)
[libvirt] [PATCH] qemu: fix EFI nvram removal on domain undefine
Posted by Pavel Mores 4 years, 5 months ago
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@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);
+}
+
 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;
 
     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);
+        need_deallocate_nvram_path = true;
+    } else {
+        if (vm->def->os.loader)
+            nvram_path = vm->def->os.loader->nvram;
+    }
+
+    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;
 }
 
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix EFI nvram removal on domain undefine
Posted by Michal Privoznik 4 years, 5 months ago
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@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@redhat.com> and pushed.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix EFI nvram removal on domain undefine
Posted by Pavel Mores 4 years, 5 months ago
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@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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix EFI nvram removal on domain undefine
Posted by Michal Privoznik 4 years, 5 months ago
On 10/15/19 3:15 PM, Pavel Mores wrote:
> 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@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@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.

By all means, I like to discuss theoretical problems :-D

String duplication can fail only if there is not enough memory. And 
while I agree that this approach I changed your patch to uses more 
memory, it's also the best approach from all wrong approaches there are 
(read as least wrong). Let me elaborate. I see three options:

1)
     char *str = NULL;
     bool free_str = false;
     ...
     if () {
         allocate(&str);
         free_str = true;
     } else {
         str = some_const_str;
     }
     check(str);
     ...
     if (free_str)
         VIR_FREE(str);


2)
     char *str = NULL;
     const char *cstr = NULL;
     ...
     if () {
         allocate(&str);
         cstr = str;
     } else {
         cstr = some_const_str;
     }
     check(cstr);
     ...
     VIR_FREE(str);


3)
     char *str = NULL;
      ...
     if () {
         allocate(&str);
      } else {
         str = strdup(some_const_str);
     }
     check(cstr);
     ...
     VIR_FREE(str);


What you proposed/had in your patch was 1), what I changed it to is 3). 
The advantade of 2) and 3) is that we can use VIR_AUTOFREE, and thus 
it's fewer lines of code, more readable IMO but more importantly, const 
correctness is preserved. That is, if by mistake somebody would do 
str[1] = 'a'; somewhere around check(str); for instance, then the 
@some_const_str is not modified.

Yes, 3) uses more memory (for a short time), but with recent discussion, 
libvirt is going to abort on OOM anyway. And if you don't have enough 
memory to duplicate a path, then 1) would not fail but in case of 
qemuDomainUndefineFlags() a path is constructed in 
virDomainDeleteConfig() - in fact two paths are constructed there so 
you'd only postpone the error.

Also, pattern 3) is kind of used in other areas of the code (at least in 
the code I've introduced), becasue I find it least evil. But I agree, C 
doesn't offer us a good solution here. Maybe go/rust with their clear 
ownership and borrow/reference counting model have something to offer us 
here, but honestly I don't know neither of the language so I cannot tell.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: fix EFI nvram removal on domain undefine
Posted by Pavel Mores 4 years, 5 months ago
On Tue, Oct 15, 2019 at 03:45:55PM +0200, Michal Privoznik wrote:
> On 10/15/19 3:15 PM, Pavel Mores wrote:
> > 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@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@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.
> 
> By all means, I like to discuss theoretical problems :-D
> 
> String duplication can fail only if there is not enough memory. And while I
> agree that this approach I changed your patch to uses more memory, it's also
> the best approach from all wrong approaches there are (read as least wrong).
> Let me elaborate. I see three options:
> 
> 1)
>     char *str = NULL;
>     bool free_str = false;
>     ...
>     if () {
>         allocate(&str);
>         free_str = true;
>     } else {
>         str = some_const_str;
>     }
>     check(str);
>     ...
>     if (free_str)
>         VIR_FREE(str);
> 
> 
> 2)
>     char *str = NULL;
>     const char *cstr = NULL;
>     ...
>     if () {
>         allocate(&str);
>         cstr = str;
>     } else {
>         cstr = some_const_str;
>     }
>     check(cstr);
>     ...
>     VIR_FREE(str);
> 
> 
> 3)
>     char *str = NULL;
>      ...
>     if () {
>         allocate(&str);
>      } else {
>         str = strdup(some_const_str);
>     }
>     check(cstr);
>     ...
>     VIR_FREE(str);
> 
> 
> What you proposed/had in your patch was 1), what I changed it to is 3). The
> advantade of 2) and 3) is that we can use VIR_AUTOFREE, and thus it's fewer
> lines of code, more readable IMO but more importantly, const correctness is
> preserved. That is, if by mistake somebody would do str[1] = 'a'; somewhere
> around check(str); for instance, then the @some_const_str is not modified.
> 
> Yes, 3) uses more memory (for a short time), but with recent discussion,
> libvirt is going to abort on OOM anyway. And if you don't have enough memory
> to duplicate a path, then 1) would not fail but in case of
> qemuDomainUndefineFlags() a path is constructed in virDomainDeleteConfig() -
> in fact two paths are constructed there so you'd only postpone the error.
> 
> Also, pattern 3) is kind of used in other areas of the code (at least in the
> code I've introduced), becasue I find it least evil. But I agree, C doesn't
> offer us a good solution here. Maybe go/rust with their clear ownership and
> borrow/reference counting model have something to offer us here, but
> honestly I don't know neither of the language so I cannot tell.

Alright, you convinced me :-), your formulation is better.  I'm not familiar
enough with the code between the strdup and the end of the function to realise
that it would fail anyway if the strdup failed.  And I overlooked the
const-correctness issue altogether.

Thanks!

	pvl

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list