Re: [libvirt] [PATCH v2 1/6] conf: Introduce virStoragePoolXMLNamespace

On 1/9/19 12:06 PM, Daniel P. Berrangé wrote: > On Tue, Jan 08, 2019 at 12:52:21PM -0500, John Ferlan wrote: >> Introduce the infrastructure necessary to manage a Storage Pool XML >> Namespace. The general concept is similar to virDomainXMLNamespace, >> except that for Storage Pools the storage backend specific details >> can be stored within the _virStoragePoolOptions unlike the domain >> processing code which manages its xmlopt's via the virDomainXMLOption >> which is allocated/passed around for each domain. >> >> This patch defines the add the parse, format, free, and href methods >> required to process the XML and callout from the Storage Pool Source >> parse, format, and clear/free API's to perform the action on the >> XML data for/from the backend. >> >> Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; >> --- >> src/conf/storage_conf.c | 51 +++++++++++++++++++++++++++++++++++++++- >> src/conf/storage_conf.h | 24 +++++++++++++++++++ >> src/libvirt_private.syms | 1 + >> 3 files changed, 75 insertions(+), 1 deletion(-) >> [...] >> + >> +int >> +virStoragePoolOptionsPoolTypeSetXMLNamespace(int type, >> + virStoragePoolXMLNamespacePtr ns); >> + >> /* >> * How the volume's data is stored on underlying >> * physical devices - can potentially span many >> @@ -197,6 +217,10 @@ struct _virStoragePoolSource { >> * or lvm version, etc. >> */ >> int format; >> + >> + /* Pool backend specific XML namespace data */ >> + void *namespaceData; >> + virStoragePoolXMLNamespace ns; >> }; > > I don't like this placement. For the domain and network XML > we put namespace data at the top level of the XML document. > This puts it one level further down, which makes it unusable > if we need it for parts of the storage pool which are not > related to the <source>. I think consistency across all our > XML documents is important here. > Understood... Still does it make sense at the <pool> level to have something that only matters to the <source>? That's what I struggled with for placement. What if someone wanted something <target> specific or <pool> specific. In the long run I don't care where it's placed - it's just moving code around. It just doesn't seem logical elsewhere though. For the domain, it's "logically" part of the "qemu", "lxc", or "vmx" domains in some manner when building the command line or setting environment variables for a command to be run. I didn't look at the network example. For storage, while it is backend/pool specific, it's even more specific to the source and some command to run or API to call to ensure the source is configured in some specific manner.