Hi Martin, Daniel
On Wed, Oct 2, 2024 at 6:01 PM Martin Kletzander <mkletzan@xxxxxxxxxx> wrote:
>
> On Wed, Oct 02, 2024 at 03:43:22PM +0400, Marc-André Lureau wrote:
> >Hi
> >
> >On Wed, Oct 2, 2024 at 11:34 AM Martin Kletzander <mkletzan@xxxxxxxxxx> wrote:
> >>
> >> On Wed, Oct 02, 2024 at 11:30:39AM +0400, Marc-André Lureau wrote:
> >> >Hi
> >> >
> >> >On Wed, Oct 2, 2024 at 11:19 AM Martin Kletzander <mkletzan@xxxxxxxxxx> wrote:
> >> >>
> >> >> On Tue, Sep 10, 2024 at 11:05:59AM +0400, marcandre.lureau@xxxxxxxxxx wrote:
> >> >> >From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> >> >> >
> >> >> >Learn to parse a directory for the TPM state.
> >> >> >
> >> >> >Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> >> >> >---
> >> >> > docs/formatdomain.rst | 3 +++
> >> >> > src/conf/domain_conf.c | 13 ++++++++++---
> >> >> > src/conf/domain_conf.h | 1 +
> >> >> > src/conf/schemas/domaincommon.rng | 15 ++++++++++++---
> >> >> > tests/qemuxmlconfdata/tpm-emulator-tpm2-enc.xml | 1 +
> >> >> > 5 files changed, 27 insertions(+), 6 deletions(-)
> >> >> >
> >> >> >diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst
> >> >> >index 4818113bc2..24dcc6daaa 100644
> >> >> >--- a/docs/formatdomain.rst
> >> >> >+++ b/docs/formatdomain.rst
> >> >> >@@ -8183,6 +8183,9 @@ Example: usage of the TPM Emulator
> >> >> >
> >> >> > This attribute requires that swtpm v0.7 or later is installed.
> >> >> >
> >> >> >+ ``dir``
> >> >> >+ The path to the TPM state storage directory.
> >> >> >+
> >> >> > :since:`Since v10.8.0`
> >> >> >
> >> >> > ``persistent_state``
> >> >> >diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> >> >> >index 18c58d16dc..d1e9e4a50c 100644
> >> >> >--- a/src/conf/domain_conf.c
> >> >> >+++ b/src/conf/domain_conf.c
> >> >> >@@ -10865,13 +10865,16 @@ virDomainTPMDefParseXML(virDomainXMLOption *xmlopt,
> >> >> >
> >> >> > source_node = virXPathNode("./backend/source", ctxt);
> >> >> > if (source_node) {
> >> >> >- path = virXMLPropString(source_node, "file");
> >> >> >+ if ((path = virXMLPropString(source_node, "file"))) {
> >> >> >+ def->data.emulator.storage_type = VIR_DOMAIN_TPM_STORAGE_FILE;
> >> >> >+ } else if ((path = virXMLPropString(source_node, "dir"))) {
> >> >> >+ def->data.emulator.storage_type = VIR_DOMAIN_TPM_STORAGE_DIR;
> >> >> >+ }
> >> >>
> >> >> It would be nicer to figure out which one to use based on a value of an
> >> >> attribute rather than what attribute we stumble upon first. Daniel
> >> >> mentioned <backend type="..."> but that is already used for the type of
> >> >> the backend, however we could have <source type="(dir|file)"> and maybe
> >> >> leave the path in as is in the docs (instead of having different
> >> >> attribute names).
> >> >
> >> >Not sure I follow. Doesnt the XML schema prevent having several
> >> ><source>, or "file" and "dir" attributes (or "type" for what you
> >> >suggest).
> >> >
> >> >So you would rather have:
> >> >
> >> ><source type="file">'/path/to/state'</source>
> >> >
> >> >rather than:
> >> >
> >> ><source file='/path/to/state'/>
> >> >
> >> >libvirt domain for <disk> for example uses the second form. I find it
> >> >more idiomatic now.
> >> >
> >>
> >> I meant <source type="(dir|file)" path="/path/to/state"/>
> >
> >I don't want to bikeshed that. I prefer consistency with other <source
> >file='...'>,
>
> Compared to disks we have:
>
> <source file=
> <source dir=
> <source dev=
> <source protocol=
> ...
> and more, based on the "type" of the device (backend).
>
> >but someone should decide and move on.
> >
>
> I think Daniel also suggested something similar in his review of v1:
>
> https://lists.libvirt.org/archives/list/devel@xxxxxxxxxxxxxxxxx/message/BVZGGFSHXNZQV5CA3TSOGSUQ2T5ZITIB/
>
> with the only difference that the <backend> already has a type attribute
> so it does not work precisely, but the basis of the point is similar I
> think.
I still don't understand what you suggest.
diff --git a/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
b/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
index 4c61e2645b..6ff8404d84 100644
--- a/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
+++ b/tests/qemuxmlconfdata/tpm-emulator-tpm2.xml
@@ -28,13 +28,13 @@
<input type='mouse' bus='ps2'/>
<input type='keyboard' bus='ps2'/>
<tpm model='tpm-tis'>
- <backend type='emulator' version='2.0' debug='3'>
+ <backend type='emulator' version='2.0' debug='3' storage='path' >
<encryption secret='b4a117f1-8af2-44a4-91b8-7f0d2d4d68a3'/>
<active_pcr_banks>
<sha256/>
<sha512/>
</active_pcr_banks>
- <source file='/path/to/state'/>
+ <source>/path/to/state'<source/>
</backend>
</tpm>
<audio id='1' type='none'/>
It feels wrong, please enlighten me :)
