[libvirt] [PATCH 0/2] Couple of UEFI fixes
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
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@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
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@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
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@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
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
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@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
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@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
participants (2)
-
Michal Privoznik -
Peter Krempa