Hi Michal
> On Jan 6, 2021, at 19:45, Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:
>
> On 1/4/21 3:31 AM, Eiichi Tsukata wrote:
>> Currently, swtpm TPM state file is removed when a transient domain is
>> powered off or undefined. When we store TPM state on a shared storage
>> such as NFS and use transient domain, TPM states should be kept as it is.
>> Add per-TPM emulator option `persistent_sate` for keeping TPM state.
>> This option only works for the emulator type backend and looks as follows:
>> <tpm model='tpm-tis'>
>> <backend type='emulator' persistent_state='yes'/>
>> </tpm>
>> Signed-off-by: Eiichi Tsukata <eiichi.tsukata@xxxxxxxxxxx>
>> Reviewed-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
>> ---
>> docs/formatdomain.rst | 7 ++++
>> docs/schemas/domaincommon.rng | 12 ++++++
>> src/conf/domain_conf.c | 21 ++++++++++
>> src/conf/domain_conf.h | 1 +
>> src/qemu/qemu_tpm.c | 3 +-
>> ...pm-emulator-tpm2-pstate.x86_64-latest.args | 38 +++++++++++++++++++
>> .../tpm-emulator-tpm2-pstate.xml | 30 +++++++++++++++
>> tests/qemuxml2argvtest.c | 1 +
>> ...tpm-emulator-tpm2-pstate.x86_64-latest.xml | 37 ++++++++++++++++++
>> tests/qemuxml2xmltest.c | 1 +
>> 10 files changed, 150 insertions(+), 1 deletion(-)
>> create mode 100644 tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.x86_64-latest.args
>> create mode 100644 tests/qemuxml2argvdata/tpm-emulator-tpm2-pstate.xml
>> create mode 100644 tests/qemuxml2xmloutdata/tpm-emulator-tpm2-pstate.x86_64-latest.xml
>
> Couple of comments.
>
>> diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
>> index 512939679b..87bbd1a4f1 100644
>> --- a/docs/formatdomain.rst
>> +++ b/docs/formatdomain.rst
>> @@ -6986,6 +6986,13 @@ Example: usage of the TPM Emulator
>> - '1.2' : creates a TPM 1.2
>> - '2.0' : creates a TPM 2.0
>> +``persistent_state``
>> + The ``persistent_state`` attribute indicates whether 'swtpm' TPM state is
>> + kept or not when a transient domain is powered off or undefined. This
>> + option can be used for preserving TPM state. By default the value is ``no``.
>> + This attribute only works with the ``emulator`` backend. The accepted values
>> + are ``yes`` and ``no``.
>
> It would be nice to have 'since 7.0.0' here so that users with older libvirt know they need a newer version.
>
>> +
>> ``encryption``
>> The ``encryption`` element allows the state of a TPM emulator to be
>> encrypted. The ``secret`` must reference a secret object that holds the
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index 795b654feb..d7cedc014c 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -4780,6 +4780,18 @@
>> </optional>
>> </group>
>> </choice>
>> + <choice>
>> + <group>
>> + <optional>
>> + <attribute name="persistent_state">
>> + <choice>
>> + <value>yes</value>
>> + <value>no</value>
>> + </choice>
>> + </attribute>
>> + </optional>
>> + </group>
>> + </choice>
>
> This looks needlessly complicated. I'd just put <optional>... under type="emulator" case. I know you were trying to mimic what version attribute does, but that's wrong too :-)
>
>> </element>
>> </define>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 23415b323c..82c3a68347 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -13178,6 +13178,12 @@ virDomainSmartcardDefParseXML(virDomainXMLOptionPtr xmlopt,
>> * <encryption secret='32ee7e76-2178-47a1-ab7b-269e6e348015'/>
>> * </backend>
>> * </tpm>
>> + *
>> + * Emulator persistent_state is supported with the following:
>> + *
>> + * <tpm model='tpm-tis'>
>> + * <backend type='emulator' version='2.0' persistent_state='yes'>
>> + * </tpm>
>> */
>> static virDomainTPMDefPtr
>> virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
>> @@ -13193,6 +13199,7 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
>> g_autofree char *backend = NULL;
>> g_autofree char *version = NULL;
>> g_autofree char *secretuuid = NULL;
>> + g_autofree char *persistent_state = NULL;
>> g_autofree xmlNodePtr *backends = NULL;
>> def = g_new0(virDomainTPMDef, 1);
>> @@ -13265,6 +13272,18 @@ virDomainTPMDefParseXML(virDomainXMLOptionPtr xmlopt,
>> }
>> def->data.emulator.hassecretuuid = true;
>> }
>> +
>> + persistent_state = virXMLPropString(backends[0], "persistent_state");
>> + if (persistent_state) {
>> + if (virStringParseYesNo(persistent_state,
>> + &def->data.emulator.persistent_state) < 0) {
>> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> + _("Invalid persistent_state value, either 'yes' or 'no'"));
>> + goto error;
>> + }
>> + } else {
>> + def->data.emulator.persistent_state = false;
>
> This is redundant. g_new0() makes sure the memory is zero initialized, and this this is already false.
>
> The rest looks good.
>
> I'm fixing all the small nits I've raised and pushing. Congratulations on your first libvirt contribution!
>
> Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>
> Michal
Thank you very much for detailed review!
Actually, this is second contribution :-)
The first one was 9 years ago:
https://libvirt.org/git/?p=libvirt.git;a=commit;h=0ac3baee2c2fd56ef89f24f5ea484e39d2bf35f5
Eiichi
