11:26 a.m.
Literally couple.
Michal Privoznik (2):
qemu: Delete nvram store for transient domains too
qemuPrepareNVRAM: Save domain conf only if domain's persistent
src/conf/domain_conf.c | 10 ++++++++--
src/conf/domain_conf.h | 1 +
src/qemu/qemu_driver.c | 3 ++-
src/qemu/qemu_process.c | 25 +++++++++++++++++--------
4 files changed, 28 insertions(+), 11 deletions(-)
--
2.0.4
11:26 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Delete nvram store for transient domains too
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.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
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/conf/domain_conf.c b/src/conf/domain_conf.c
index 2c65276..2bcf123 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -13290,6 +13290,8 @@ virDomainDefParseXML(xmlDocPtr xml,
def->os.loader->nvram =
virXPathString("string(./os/nvram[1])", ctxt);
def->os.loader->templt =
virXPathString("string(./os/nvram[1]/@template)", ctxt);
+ if (flags & VIR_DOMAIN_XML_INTERNAL_STATUS)
+ def->os.loader->generated =
virXPathBoolean("boolean(./os/nvram[1]/@generated)", ctxt);
}
}
@@ -18694,7 +18696,8 @@ virDomainHugepagesFormat(virBufferPtr buf,
static void
virDomainLoaderDefFormat(virBufferPtr buf,
- virDomainLoaderDefPtr loader)
+ virDomainLoaderDefPtr loader,
+ unsigned int flags)
{
const char *readonly = virTristateBoolTypeToString(loader->readonly);
const char *type = virDomainLoaderTypeToString(loader->type);
@@ -18710,6 +18713,9 @@ virDomainLoaderDefFormat(virBufferPtr buf,
if (loader->nvram || loader->templt) {
virBufferAddLit(buf, "<nvram");
virBufferEscapeString(buf, " template='%s'",
loader->templt);
+ if (loader->generated &&
+ flags & VIR_DOMAIN_XML_INTERNAL_STATUS)
+ virBufferAddLit(buf, " generated='yes'");
if (loader->nvram)
virBufferEscapeString(buf, ">%s</nvram>\n",
loader->nvram);
else
@@ -19056,7 +19062,7 @@ virDomainDefFormatInternal(virDomainDefPtr def,
virBufferEscapeString(buf, "<initarg>%s</initarg>\n",
def->os.initargv[i]);
if (def->os.loader)
- virDomainLoaderDefFormat(buf, def->os.loader);
+ virDomainLoaderDefFormat(buf, def->os.loader, flags);
virBufferEscapeString(buf, "<kernel>%s</kernel>\n",
def->os.kernel);
virBufferEscapeString(buf, "<initrd>%s</initrd>\n",
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 530a3ca..19b282b 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1692,6 +1692,7 @@ struct _virDomainLoaderDef {
int readonly; /* enum virTristateBool */
virDomainLoader type;
char *nvram; /* path to non-volatile RAM */
+ bool generated; /* Is the @nvram path provided from XML or generated? */
char *templt; /* user override of path to master nvram */
};
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index acf2b9a..9f51b98 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6816,7 +6816,8 @@ qemuDomainUndefineFlags(virDomainPtr dom,
if (!virDomainObjIsActive(vm) &&
vm->def->os.loader && vm->def->os.loader->nvram
&&
virFileExists(vm->def->os.loader->nvram)) {
- if (!(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
+ if (!vm->def->os.loader->generated &&
+ !(flags & VIR_DOMAIN_UNDEFINE_NVRAM)) {
virReportError(VIR_ERR_OPERATION_INVALID, "%s",
_("cannot delete inactive domain with nvram"));
goto cleanup;
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
@@ -3909,7 +3909,6 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
int srcFD = -1;
int dstFD = -1;
virDomainLoaderDefPtr loader = def->os.loader;
- bool generated = false;
bool created = false;
/* Unless domain has RO loader of pflash type, we have
@@ -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;
@@ -4022,8 +4021,10 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
if (ret < 0) {
if (created)
unlink(loader->nvram);
- if (generated)
+ if (loader->generated) {
VIR_FREE(loader->nvram);
+ loader->generated = false;
+ }
}
VIR_FORCE_CLOSE(srcFD);
@@ -5035,6 +5036,13 @@ void qemuProcessStop(virQEMUDriverPtr driver,
}
}
+ /* Remove nvram file, if needed */
+ if (!vm->persistent &&
+ def->os.loader &&
+ def->os.loader->nvram &&
+ def->os.loader->generated)
+ unlink(vm->def->os.loader->nvram);
+
vm->taint = 0;
vm->pid = -1;
virDomainObjSetState(vm, VIR_DOMAIN_SHUTOFF, reason);
--
2.0.4
12:10 p.m.
New subject: [libvirt] [PATCH 1/2] qemu: Delete nvram store for transient domains too
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.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
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.
Peter
2:34 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Delete nvram store for transient domains too
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(a)redhat.com>
> ---
> 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
6:45 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Delete nvram store for transient domains too
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.
Okay, we should document that the file is left behind in case of
transient domains and it's mgmt app responsibility to remove the file
once not needed. What persuaded me is the fact, that it would be almost
impossible for mgmt app to keep nvram file if this patch is merged. I
mean, it's not possible from the host to tell when is the nvram file
written or whether are some writes still pending in the guest. So the
app can't just copy the file shortly before guest termination and hope
for the best. Moreover, the guest can die without mgmt app knowledge
beforehand.
Self NACK. Watch out for v2.
Michal
11:26 a.m.
New subject: [libvirt] [PATCH 2/2] qemuPrepareNVRAM: Save domain conf only if domain's persistent
In one of my previous patches (3a3c3780b) I've tried to fix the
problem of nvram path disappearing on a domain that's been
started and shut down again. I fixed this by explicitly saving
domain's config file. However, I did a bit of clumsy without
realizing we have a transient domains for which we don't save the
config file. Hence, any domain using UEFI became persistent.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c0ab341..1f44683 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3902,13 +3902,13 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver,
static int
qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
- virDomainDefPtr def,
+ virDomainObjPtr vm,
bool migrated)
{
int ret = -1;
int srcFD = -1;
int dstFD = -1;
- virDomainLoaderDefPtr loader = def->os.loader;
+ virDomainLoaderDefPtr loader = vm->def->os.loader;
bool created = false;
/* Unless domain has RO loader of pflash type, we have
@@ -3934,12 +3934,13 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
if (!loader->nvram) {
if (virAsprintf(&loader->nvram,
"%s/lib/libvirt/qemu/nvram/%s_VARS.fd",
- LOCALSTATEDIR, def->name) < 0)
+ LOCALSTATEDIR, vm->def->name) < 0)
goto cleanup;
loader->generated = true;
- if (virDomainSaveConfig(cfg->configDir, def) < 0)
+ if (vm->persistent &&
+ virDomainSaveConfig(cfg->configDir, vm->def) < 0)
goto cleanup;
}
@@ -4106,7 +4107,7 @@ int qemuProcessStart(virConnectPtr conn,
* Fill them in prior to setting the domain def as transient. */
VIR_DEBUG("Generating paths");
- if (qemuPrepareNVRAM(cfg, vm->def, migrateFrom) < 0)
+ if (qemuPrepareNVRAM(cfg, vm, migrateFrom) < 0)
goto cleanup;
/* Do this upfront, so any part of the startup process can add
--
2.0.4
11:47 a.m.
New subject: [libvirt] [PATCH 2/2] qemuPrepareNVRAM: Save domain conf only if domain's persistent
On 11/12/14 18:26, Michal Privoznik wrote:
In one of my previous patches (3a3c3780b) I've tried to fix the
problem of nvram path disappearing on a domain that's been
started and shut down again. I fixed this by explicitly saving
domain's config file. However, I did a bit of clumsy without
realizing we have a transient domains for which we don't save the
config file. Hence, any domain using UEFI became persistent.
Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
---
src/qemu/qemu_process.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index c0ab341..1f44683 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -3902,13 +3902,13 @@ qemuProcessVerifyGuestCPU(virQEMUDriverPtr driver,
static int
qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
This function is called just before cloning vm->def into vm->newDef and
thus vm->def becoming the live definition.
- virDomainDefPtr def,
+ virDomainObjPtr vm,
bool migrated)
{
int ret = -1;
int srcFD = -1;
int dstFD = -1;
- virDomainLoaderDefPtr loader = def->os.loader;
+ virDomainLoaderDefPtr loader = vm->def->os.loader;
bool created = false;
/* Unless domain has RO loader of pflash type, we have
@@ -3934,12 +3934,13 @@ qemuPrepareNVRAM(virQEMUDriverConfigPtr cfg,
if (!loader->nvram) {
if (virAsprintf(&loader->nvram,
"%s/lib/libvirt/qemu/nvram/%s_VARS.fd",
- LOCALSTATEDIR, def->name) < 0)
+ LOCALSTATEDIR, vm->def->name) < 0)
goto cleanup;
loader->generated = true;
- if (virDomainSaveConfig(cfg->configDir, def) < 0)
+ if (vm->persistent &&
+ virDomainSaveConfig(cfg->configDir, vm->def) < 0)
So saving the vm->def here is okay. In other place you'd need to use the
helper to get the right pointer according to the vm->persistent flag.
goto cleanup;
}
@@ -4106,7 +4107,7 @@ int qemuProcessStart(virConnectPtr conn,
* Fill them in prior to setting the domain def as transient. */
VIR_DEBUG("Generating paths");
- if (qemuPrepareNVRAM(cfg, vm->def, migrateFrom) < 0)
+ if (qemuPrepareNVRAM(cfg, vm, migrateFrom) < 0)
goto cleanup;
/* Do this upfront, so any part of the startup process can add
ACK,
Peter
3657
days inactive
3663
days old
6 comments
2 participants
participants (2)
-
Michal Privoznik -
Peter Krempa