Re: [PATCH v2 3/4] schema: add TPM emulator <source dir='..'>

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