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

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(a)redhat.com&gt; 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(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. >> > >> >> 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/B... 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.