On 12.11.2014 19:10, Peter Krempa wrote:
On 11/12/14 18:26, Michal Privoznik wrote:
There are two ways how to use nvram variable store file in
libvirt:
1) create it by hand and pass the path in domain XML
2) let libvirt generate path and create the file
Now, we allow users to remove the file from case 2) by passing a
flag into the virDomainUndefineFlags(). But when implementing
this I forgot about transient domains. For them the file needs to
be removed in qemuProcessStop(). But not always, only if the file
was created by us. For that reason new attribute to <nvram/> is
invented: @generated to keep the info between daemon restarts.
However, now that we are removing autogenerated file for
transient domains automatically, we should do the same for
persistent domains too without requiring the
VIR_DOMAIN_UNDEFINE_NVRAM flag.
I agree that we should clean up the file once we create it as currently
the guest would be started with the NVRAM generated for a different
guest with the same name which might trigger some unwanted behavior.
The existing behavior (introduced in 1.2.8) though might lead the users
to use transient VMs with unspecified NVRAM path and rely on the fact
that the NVRAM file is present from the last run.
I think we should document that unless the NVRAM file is provided
externally the settings don't persist between runs and users using
transient VMs (hello oVirt) should take care of generating it by
themselves as they might get unexpected results and we should do it
rather sooner than later until somebody starts abusing it.
As a side note I find it borderline useful to have such configuration at
all as it will only survive until the VM is restarted and thus it's kind
of pointless to write the nvram variables to the disk at all.
Not really, transient domain is not destroyed on guest OS reboots. So it
makes a sense. A little.
Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
---
src/conf/domain_conf.c | 10 ++++++++--
src/conf/domain_conf.h | 1 +
src/qemu/qemu_driver.c | 3 ++-
src/qemu/qemu_process.c | 14 +++++++++++---
4 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index 24e5f0f..c0ab341 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3938,7 +3937,7 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
LOCALSTATEDIR, def->name) < 0)
goto cleanup;
- generated = true;
+ loader->generated = true;
if (virDomainSaveConfig(cfg->configDir, def) < 0)
goto cleanup;
Okay, so placement of qemuPrepareNVRAM is critical. It happens before
the "newDef" is created (for persistent vms) and before the implicit
virDomainSaveStatus thus everything will work as it should.
Took me a while to realize that though.
I'd like to see a version that touches the documentation before giving
my final word though.
Also possibly a second opinion on the change of the behavior is welcome too.
Okay, I see your point. I think we have two other options here:
1) document that the file is left behind for transient domains
2) introduce yet another XML attribute to <nvram/> which would tell what
to do with the NVRAM file on domain destroy (for transient domains).
Frankly, the more I think about this the more I like option 1). It's
possible to leave a file behind even for persistent domains. Just start
it with blank <nvram/>, let libvirt create the file for you, destroy the
domain, and just before undefying it change domain XML so that no OVMF
is configured for the domain.
Having said that, I believe my question is: does anybody object
documenting the fact that NVRAM file may be left behind and it's mgmt
app responsibility to remove the file once not needed?
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list