From: Marc-André Lureau <marcandre.lureau@redhat.com>

Learn to parse a directory for the TPM state.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 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;
+            }
             if (!path) {
                 virReportError(VIR_ERR_XML_ERROR, "%s",
-                               _("missing TPM file source"));
+                               _("missing TPM file or directory source"));
                 goto error;
             }
-            def->data.emulator.storage_type = VIR_DOMAIN_TPM_STORAGE_FILE;
             def->data.emulator.storagepath = g_steal_pointer(&path);
         }
 
@@ -25084,6 +25087,10 @@ virDomainTPMDefFormat(virBuffer *buf,
                 virBufferAsprintf(&backendChildBuf, "<source file='%s'/>\n",
                                   def->data.emulator.storagepath);
                 break;
+            case VIR_DOMAIN_TPM_STORAGE_DIR:
+                virBufferAsprintf(&backendChildBuf, "<source dir='%s'/>\n",
+                                  def->data.emulator.storagepath);
+                break;
             case VIR_DOMAIN_TPM_STORAGE_DEFAULT:
                 break;
         }
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 371e6ecf6c..4e4ae2e048 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1465,6 +1465,7 @@ typedef enum {
 
 typedef enum {
     VIR_DOMAIN_TPM_STORAGE_DEFAULT,
+    VIR_DOMAIN_TPM_STORAGE_DIR,
     VIR_DOMAIN_TPM_STORAGE_FILE,
 } virDomainTPMStorage;
 
diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
index 62d3f0e6fe..f6b47ae97e 100644
--- a/src/conf/schemas/domaincommon.rng
+++ b/src/conf/schemas/domaincommon.rng
@@ -5985,9 +5985,18 @@
   <define name="tpm-backend-emulator-source">
     <optional>
       <element name="source">
-        <attribute name="file">
-          <ref name="filePath"/>
-        </attribute>
+        <choice>
+          <group>
+            <attribute name="dir">
+              <ref name="absDirPath"/>
+            </attribute>
+          </group>
+          <group>
+            <attribute name="file">
+              <ref name="filePath"/>
+            </attribute>
+          </group>
+        </choice>
       </element>
     </optional>
   </define>
diff --git a/tests/qemuxmlconfdata/tpm-emulator-tpm2-enc.xml b/tests/qemuxmlconfdata/tpm-emulator-tpm2-enc.xml
index 9c2279b28b..016c68296c 100644
--- a/tests/qemuxmlconfdata/tpm-emulator-tpm2-enc.xml
+++ b/tests/qemuxmlconfdata/tpm-emulator-tpm2-enc.xml
@@ -30,6 +30,7 @@
     <tpm model='tpm-tis'>
       <backend type='emulator' version='2.0'>
         <encryption secret='32ee7e76-2178-47a1-ab7b-269e6e348015'/>
+        <source dir='/some/dir'/>
       </backend>
     </tpm>
     <audio id='1' type='none'/>
-- 
2.45.2.827.g557ae147e6

On Tue, Sep 10, 2024 at 11:05:59AM +0400, marcandre.lureau@redhat.com wrote:
>From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
>Learn to parse a directory for the TPM state.
>
>Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>---
> 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).

Hi

On Wed, Oct 2, 2024 at 11:19 AM Martin Kletzander <mkletzan@redhat.com> wrote:
>
> On Tue, Sep 10, 2024 at 11:05:59AM +0400, marcandre.lureau@redhat.com wrote:
> >From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> >Learn to parse a directory for the TPM state.
> >
> >Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >---
> > 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.

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@redhat.com> wrote:
> >
> > On Tue, Sep 10, 2024 at 11:05:59AM +0400, marcandre.lureau@redhat.com wrote:
> > >From: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >
> > >Learn to parse a directory for the TPM state.
> > >
> > >Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > >---
> > > 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.

That isn't a valid comparison, because there is a "type" attribute on
the parent <disk> element that dictates what schema branch is followed
for <source>.

The TPM *must* have an equivalent attribute somewhere that dictates
the schema branch for <source>.

This means either we put "type=file|dir" on the <source>, or we invent
a new "source=file|dir" on the parent <tpm> element. I tend to favour
the former, since "type" is the common attribute name we use for
schema branch discriminators.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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@redhat.com> wrote:
>>
>> On Tue, Sep 10, 2024 at 11:05:59AM +0400, marcandre.lureau@redhat.com wrote:
>> >From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> >Learn to parse a directory for the TPM state.
>> >
>> >Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >---
>> > 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"/>

Hi

On Wed, Oct 2, 2024 at 11:34 AM Martin Kletzander <mkletzan@redhat.com> 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@redhat.com> wrote:
> >>
> >> On Tue, Sep 10, 2024 at 11:05:59AM +0400, marcandre.lureau@redhat.com wrote:
> >> >From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >
> >> >Learn to parse a directory for the TPM state.
> >> >
> >> >Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >---
> >> > 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='...'>, but someone should decide and move on.

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@redhat.com> 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@redhat.com> wrote:
>> >>
>> >> On Tue, Sep 10, 2024 at 11:05:59AM +0400, marcandre.lureau@redhat.com wrote:
>> >> >From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> >
>> >> >Learn to parse a directory for the TPM state.
>> >> >
>> >> >Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >> >---
>> >> > 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@lists.libvirt.org/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.

Hi Martin, Daniel

On Wed, Oct 2, 2024 at 6:01 PM Martin Kletzander <mkletzan@redhat.com> 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@redhat.com> 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@redhat.com> wrote:
> >> >>
> >> >> On Tue, Sep 10, 2024 at 11:05:59AM +0400, marcandre.lureau@redhat.com wrote:
> >> >> >From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >> >
> >> >> >Learn to parse a directory for the TPM state.
> >> >> >
> >> >> >Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> >> >---
> >> >> > 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@lists.libvirt.org/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 :)

On 9/10/24 3:05 AM, marcandre.lureau@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> Learn to parse a directory for the TPM state.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

> ---
>   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;
> +            }
>               if (!path) {
>                   virReportError(VIR_ERR_XML_ERROR, "%s",
> -                               _("missing TPM file source"));
> +                               _("missing TPM file or directory source"));
>                   goto error;
>               }
> -            def->data.emulator.storage_type = VIR_DOMAIN_TPM_STORAGE_FILE;
>               def->data.emulator.storagepath = g_steal_pointer(&path);
>           }
>   
> @@ -25084,6 +25087,10 @@ virDomainTPMDefFormat(virBuffer *buf,
>                   virBufferAsprintf(&backendChildBuf, "<source file='%s'/>\n",
>                                     def->data.emulator.storagepath);
>                   break;
> +            case VIR_DOMAIN_TPM_STORAGE_DIR:
> +                virBufferAsprintf(&backendChildBuf, "<source dir='%s'/>\n",
> +                                  def->data.emulator.storagepath);
> +                break;
>               case VIR_DOMAIN_TPM_STORAGE_DEFAULT:
>                   break;
>           }
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 371e6ecf6c..4e4ae2e048 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1465,6 +1465,7 @@ typedef enum {
>   
>   typedef enum {
>       VIR_DOMAIN_TPM_STORAGE_DEFAULT,
> +    VIR_DOMAIN_TPM_STORAGE_DIR,
>       VIR_DOMAIN_TPM_STORAGE_FILE,
>   } virDomainTPMStorage;
>   
> diff --git a/src/conf/schemas/domaincommon.rng b/src/conf/schemas/domaincommon.rng
> index 62d3f0e6fe..f6b47ae97e 100644
> --- a/src/conf/schemas/domaincommon.rng
> +++ b/src/conf/schemas/domaincommon.rng
> @@ -5985,9 +5985,18 @@
>     <define name="tpm-backend-emulator-source">
>       <optional>
>         <element name="source">
> -        <attribute name="file">
> -          <ref name="filePath"/>
> -        </attribute>
> +        <choice>
> +          <group>
> +            <attribute name="dir">
> +              <ref name="absDirPath"/>
> +            </attribute>
> +          </group>
> +          <group>
> +            <attribute name="file">
> +              <ref name="filePath"/>
> +            </attribute>
> +          </group>
> +        </choice>
>         </element>
>       </optional>
>     </define>
> diff --git a/tests/qemuxmlconfdata/tpm-emulator-tpm2-enc.xml b/tests/qemuxmlconfdata/tpm-emulator-tpm2-enc.xml
> index 9c2279b28b..016c68296c 100644
> --- a/tests/qemuxmlconfdata/tpm-emulator-tpm2-enc.xml
> +++ b/tests/qemuxmlconfdata/tpm-emulator-tpm2-enc.xml
> @@ -30,6 +30,7 @@
>       <tpm model='tpm-tis'>
>         <backend type='emulator' version='2.0'>
>           <encryption secret='32ee7e76-2178-47a1-ab7b-269e6e348015'/>
> +        <source dir='/some/dir'/>
>         </backend>
>       </tpm>
>       <audio id='1' type='none'/>