[libvirt] [PATCH 10/16] conf: Add separate defaults addition and validation for XML parsing

[Daniel P. Berrange]

On Mon, Mar 04, 2013 at 04:04:43PM +0100, Peter Krempa wrote:
> On 03/01/13 20:10, Laine Stump wrote:
> >On 02/20/2013 12:06 PM, Peter Krempa wrote:
> >>This patch adds instrumentation that will ultimately allow to split out
> >>filling of defaults and input validation from the XML parser to separate
> >>functions.
> >>
> >>With this patch, after the XML is parsed, a callback to the driver is
> >>issued requesing to fill and validate driver specific details of the
> >>configuration. This allows to use sensible defaults and checks on a per
> >>driver basis at the time the XML is parsed.
> >>
> >>Two callback pointers are exposed in the virCaps object:
> >>* virDriverDeviceDefCallback - called for a single device parsed and for
> >>                                every single device in a domain config.
> >>                                A virDomainDeviceDefPtr is passed.
> >>* virDriverDomainDefCallback - called if a domain definition is parsed
> >>                                device specific callbacks were called.
> >>                                A virDomainDefPtr is passed.
> >>
> >>Errors may be reported in those callbacks resulting in a XML parsing
> >>failure.
> >>
> >>Additionally two internal filler functions are added:
> >>virDomainDeviceDefUpdateDefaultsInternal and
> >>virDomainDefUpdateDefaultsInternal that are meant to be used for
> >>separating the validation and defaults assignment code from the parser
> >>function.
> >>---
> >>  src/conf/capabilities.h |  6 +++++
> >>  src/conf/domain_conf.c  | 72 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>  2 files changed, 78 insertions(+)
> >>
> >>diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h
> >>index cc01765..56cd09f 100644
> >>--- a/src/conf/capabilities.h
> >>+++ b/src/conf/capabilities.h
> >>@@ -171,6 +171,12 @@ struct _virCaps {
> >>      bool hasWideScsiBus;
> >>      const char *defaultInitPath;
> >>
> >>+
> >>+    /* these callbacks are called after a XML definition of a device or domain
> >>+     * is parsed. They are meant to validate and fill driver-specific values. */
> >>+    int (*virDriverDeviceDefCallback)(void *); /* virDomainDeviceDefPtr is passed */
> >>+    int (*virDriverDomainDefCallback)(void *); /* virDomainDefPtr is passed */
> >
> >If you know that  virDriverDeviceDefCallback is always given a
> >virDomainDeviceDefPtr, why prototype it as void*? Same question for
> >virDriverDomainDefCallback.
> 
> Those two types are defined in conf/domain_conf.h.
> conf/domain_conf.h in return needs conf/capabilities.h. This creates
> a circular dependency chain if I try to include domain_conf.h in
> capabilities.h. I tried to to come up with a different solution than
> void * but none of the others were nicer than that. The options
> were:
> 
> 1) use a separate header file for one of the types
> 2) use a extern declaration
> 3) include domain_conf.h into capabilities.h after all needed types
> were declared.
> 
> I'll welcome any tips how to solve this problem. (cc-d Dan and Eric).

This says to me that capabilities.h is not the right place for this
data. Originally virCapabilities was just used to hold information
about the guests/machine types / etc supported by a driver. Then we
extended the domain XML parsers to require this object to be passed
in. Then we added namespace hooks to it. Now we're adding yet more
stuff.

IMHO it is time to undo this mess. We need a virCapabilities object
that just contains the data associated with the capabilities XML
schema, nothing more.

Then we need a virDomainParserConfigPtr struct that contains the
information required by the virDomainDef parsers.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|