On Mon, Sep 07, 2026 at 01:24:09 +0530, Arun Menon via Devel wrote:
From: Arun Menon <armenon@redhat.com>
Update logic for both creation and parsing the 2 new attributes backup and fsync introduced in swtpm.
Signed-off-by: Arun Menon <armenon@redhat.com> --- src/conf/domain_conf.c | 31 +++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ 2 files changed, 33 insertions(+)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 34d3b00079..0326ad7c58 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11449,6 +11449,11 @@ virDomainSmartcardDefParseXML(virDomainXMLOption *xmlopt, * </backend> * </tpm> * + * Emulator state backup and fsync is supported with the following: + * <tpm model='tpm-crb'> + * <backend type='emulator' version='2.0' backup='yes' fsync='yes'> + * </tpm>
So usually the commit which implements the XML bits also adds docs. I'll borrow them from commit 3/4 here, because I wonder if these options even should exist. (In fact I also think the RNG schema should be part of this commit instead of being separate) Specifically 'fsync' is documented as: +``fsync`` + The ``fsync`` attribute indicates whether an fsync will be called on the + file and the directory of the file when the 'swtpm' TPM state is written + to disk. This option ensures that all data have been successfully written + to physical storage before the TPM processes the next command. It lowers + the probability of TPM state file corruption in case of a power loss. By + default the value is ``no``. This attribute only works with the directory + backend. The accepted values are ``yes`` and ``no``. :since:`Since 12.8.0` And I wonder why would anyone ever want to select 'no' here based on this description. The 'yes' value seems to only hold advantages as documented. Now with: +``backup`` + The ``backup`` attribute indicates whether a backup of the 'swtpm' TPM + state is taken or not. This option can be used also for restoring + the state from the backup. By default the value is ``no``. + This attribute only works with the directory backend. The accepted values + are ``yes`` and ``no``. :since:`Since 12.8.0` I'm a bit less sure, as the description is extremely vague. Anyways it still seems to be an option that IMO should be always enabled. Can you please elaborate why would anyone ever want to disable those?
+ * */ static virDomainTPMDef * virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, @@ -11465,6 +11470,8 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, g_autofree char *path = NULL; g_autofree char *secretuuid = NULL; g_autofree char *persistent_state = NULL; + g_autofree char *backup = NULL; + g_autofree char *fsync = NULL; g_autofree xmlNodePtr *backends = NULL; g_autofree xmlNodePtr *nodes = NULL; g_autofree char *type = NULL; @@ -11562,6 +11569,26 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt, } }
+ backup = virXMLPropString(backends[0], "backup"); + if (backup) { + if (virStringParseYesNo(backup, + &def->data.emulator.backup) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Invalid backup value, either 'yes' or 'no'")); + goto error; + } + }
Use virXMLPropTristateBool instead of open-coding it here. That also resolves a further complaint that VIR_ERR_INTERNAL_ERROR is inappropriate for user config errors. See below on the question why not use 'bool' type.
+ + fsync = virXMLPropString(backends[0], "fsync"); + if (fsync) { + if (virStringParseYesNo(fsync, + &def->data.emulator.fsync) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Invalid fsync value, either 'yes' or 'no'")); + goto error; + } + }
Same here.
+ if ((nnodes = virXPathNodeSet("./backend/active_pcr_banks/*", ctxt, &nodes)) < 0) break; if (nnodes > 0) @@ -26662,6 +26689,10 @@ virDomainTPMDefFormat(virBuffer *buf, } if (def->data.emulator.persistent_state) virBufferAddLit(&backendAttrBuf, " persistent_state='yes'"); + if (def->data.emulator.backup) + virBufferAddLit(&backendAttrBuf, " backup='yes'"); + if (def->data.emulator.fsync) + virBufferAddLit(&backendAttrBuf, " fsync='yes'"); if (def->data.emulator.debug != 0) virBufferAsprintf(&backendAttrBuf, " debug='%u'", def->data.emulator.debug); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3732525af4..0d93487689 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1546,6 +1546,8 @@ struct _virDomainTPMEmulatorDef { char *name; /* name read from active profile */ virDomainTPMProfileRemoveDisabled removeDisabled; } profile; + bool backup; + bool fsync;
These should IMO be tristate so it's clear if the user used them or not. We already had issues with cases where the 'no' option was default, but later qemu decided to change it to yes. Libvirt coudn't tell if the user requested 'no' or omitted the option. So we really must not do 'bool' for XML attributes ever again. All of the above is provided that either of the options in fact need to be configured, which I'm not sure about.