On Mon, Sep 07, 2026 at 01:24:09 +0530, Arun Menon via Devel wrote:
> From: Arun Menon <[email protected]>
>
> Update logic for both creation and parsing the 2 new attributes backup
> and fsync introduced in swtpm.
>
> Signed-off-by: Arun Menon <[email protected]>
> ---
> 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.
