Re: [libvirt] [PATCH 2/2] qemuDomainUndefineFlags: unlink nvram file regardless of domain state
On Mon, Aug 07, 2017 at 02:20:06PM +0200, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1467245
>
> Currently, there's a bug when undefining a domain with NVRAM
> store. Basically, the unlink() of the NVRAM store file happens
> during the undefine procedure iff domain is inactive. So, if
> domain is running and undefine is called the file is left behind.
> It won't be removed in the domain cleanup process either
> (qemuProcessStop). One of the solutions is to remove if
> regardless of the domain state and rely on qemu having the file
> opened. This still has a downside that if the domain is defined
> back the NVRAM store file is going to be new, any changes to the
> current one are lost (just like with any other file that is
> deleted while a process has it opened). But is it really a
> downside?
We only unlink if the user explicitly gives VIR_DOMAIN_UNDEFINE_NVRAM,
so I think that "prolem" scenario you describe is exactly what the user
has asked for. ie not a bug - just don't pass VIR_DOMAIN_UNDEFINE_NVRAM
if they want to keep it around across an undefine+define pair.
>
> Signed-off-by: Michal Privoznik
> ---
> src/qemu/qemu_driver.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 574c351ae..992ae2a2e 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7367,8 +7367,8 @@ qemuDomainUndefineFlags(virDomainPtr dom,
> }
> }
>
> -if (!virDomainObjIsActive(vm) &&
> -vm->def->os.loader && vm->def->os.loader->nvram &&
> +if (vm->def->os.loader &&
> +vm->def->os.loader->nvram &&
> virFileExists(vm->def->os.loader->nvram)) {
> if ((flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
> if (unlink(vm->def->os.loader->nvram) < 0) {
ACK
Regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemuDomainUndefineFlags: unlink nvram file regardless of domain state
On Wed, Aug 09, 2017 at 01:13:02PM +0200, Michal Privoznik wrote: On 08/09/2017 11:41 AM, Martin Kletzander wrote: On Mon, Aug 07, 2017 at 02:20:06PM +0200, Michal Privoznik wrote: https://bugzilla.redhat.com/show_bug.cgi?id=1467245 Currently, there's a bug when undefining a domain with NVRAM store. Basically, the unlink() of the NVRAM store file happens during the undefine procedure iff domain is inactive. So, if domain is running and undefine is called the file is left behind. It won't be removed in the domain cleanup process either (qemuProcessStop). One of the solutions is to remove if regardless of the domain state and rely on qemu having the file opened. This still has a downside that if the domain is defined back the NVRAM store file is going to be new, any changes to the current one are lost (just like with any other file that is deleted while a process has it opened). But is it really a downside? It might be. Why don't we disable removing it when the domain is running? We have some precedence for this. The place which already deals with this possibility is tools/virsh-domain.c in the cmdUndefine() where we handle --remove-all-storage. If you look at the help of that command it also says: --nvram remove nvram file, if inactive And that makes sense to me. What doesn't, on the other hand, is that it also says: --keep-nvram keep nvram file, if inactive I don't get the "if inactive" there. But I'm not going to check who pushed that. At least not again =) Okay, I'll write a patch that: a) forbids undefine for active domains, unless b) KEEP_NVRAM flag is specified. c) or there is no nvram =) Yeah, well, that's caused by the unfortunate design of the way nvram is handled by qemu and qemu driver. Thanks Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemuDomainUndefineFlags: unlink nvram file regardless of domain state
On 08/09/2017 11:41 AM, Martin Kletzander wrote: > On Mon, Aug 07, 2017 at 02:20:06PM +0200, Michal Privoznik wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1467245 >> >> Currently, there's a bug when undefining a domain with NVRAM >> store. Basically, the unlink() of the NVRAM store file happens >> during the undefine procedure iff domain is inactive. So, if >> domain is running and undefine is called the file is left behind. >> It won't be removed in the domain cleanup process either >> (qemuProcessStop). One of the solutions is to remove if >> regardless of the domain state and rely on qemu having the file >> opened. This still has a downside that if the domain is defined >> back the NVRAM store file is going to be new, any changes to the >> current one are lost (just like with any other file that is >> deleted while a process has it opened). But is it really a >> downside? >> > > It might be. Why don't we disable removing it when the domain is > running? We have some precedence for this. The place which already > deals with this possibility is tools/virsh-domain.c in the cmdUndefine() > where we handle --remove-all-storage. If you look at the help of that > command it also says: > > --nvram remove nvram file, if inactive > > And that makes sense to me. What doesn't, on the other hand, is that it > also says: > > --keep-nvram keep nvram file, if inactive > > I don't get the "if inactive" there. But I'm not going to check who > pushed that. At least not again =) Okay, I'll write a patch that: a) forbids undefine for active domains, unless b) KEEP_NVRAM flag is specified. Michal -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/2] qemuDomainUndefineFlags: unlink nvram file regardless of domain state
On Mon, Aug 07, 2017 at 02:20:06PM +0200, Michal Privoznik wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1467245
Currently, there's a bug when undefining a domain with NVRAM
store. Basically, the unlink() of the NVRAM store file happens
during the undefine procedure iff domain is inactive. So, if
domain is running and undefine is called the file is left behind.
It won't be removed in the domain cleanup process either
(qemuProcessStop). One of the solutions is to remove if
regardless of the domain state and rely on qemu having the file
opened. This still has a downside that if the domain is defined
back the NVRAM store file is going to be new, any changes to the
current one are lost (just like with any other file that is
deleted while a process has it opened). But is it really a
downside?
It might be. Why don't we disable removing it when the domain is
running? We have some precedence for this. The place which already
deals with this possibility is tools/virsh-domain.c in the cmdUndefine()
where we handle --remove-all-storage. If you look at the help of that
command it also says:
--nvram remove nvram file, if inactive
And that makes sense to me. What doesn't, on the other hand, is that it
also says:
--keep-nvram keep nvram file, if inactive
I don't get the "if inactive" there. But I'm not going to check who
pushed that. At least not again =)
Signed-off-by: Michal Privoznik
---
src/qemu/qemu_driver.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 574c351ae..992ae2a2e 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -7367,8 +7367,8 @@ qemuDomainUndefineFlags(virDomainPtr dom,
}
}
-if (!virDomainObjIsActive(vm) &&
-vm->def->os.loader && vm->def->os.loader->nvram &&
+if (vm->def->os.loader &&
+vm->def->os.loader->nvram &&
virFileExists(vm->def->os.loader->nvram)) {
if ((flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
if (unlink(vm->def->os.loader->nvram) < 0) {
--
2.13.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
