[libvirt] PATCH: Generic internal API for domain XML parser/formatter

[Daniel Veillard]

On Tue, Jun 24, 2008 at 04:34:11PM +0100, Daniel P. Berrange wrote:
> We currently have five drivers which handle the domain XML containing
> duplicated parsers and formatters for the XML with varying degrees of
> buginess, and often very similar structs. This patch introduces a new
> general purpose internal API for parsing and formatting network XML, 
> and representing it as a series of structs.

  Okay, very good, +1 on principle 

> This code is derived from the current equivalent code in the QEMU driver
> for domains, but with alot of extra config parsing added for stuff needed
> by the Xen drivers. I have not yet added the extra bits needed by the
> container based drivers, LXC and OpenVZ, but don't anticipate any problems
> in this regard.

  The question is how much the data structure will need to grow to cover
new hypervisor cases, the understainties on the set of data was one of
the primary reasons of an XML only API, ultimately we may be able to 
bet with a fixed set and include it in the API with ABI guarantees. Still
a bit premature IMHO but something we need to keep in mind for long term.
Those datastructures are in my opinion what will allow us to declare
victory in the end and push a 1.0.0 release at some point.

[...]
> Given an XML document describing a network, parses the doc and generates
> a virDomainDefPtr to represent it in memory. This is a little more 
> advanced than the network parser because the various hypervisor drivers
> have slightly varying capabilities. So we pass a virCapsPtr object in
> so the parser can validate against the actual driver's capabilities.
> I'd like to extend the capabilities format to cover things like the
> boot parameters supported - eg PXE vs kernel+initrd vs bootloader vs
> BIOS, so we can further validate at time of parsing, instead of time
> of use.

  Clearly it's time to move from ad-hoc parsing to generic and the caps
makes perfect sense here.

>   char *virDomainDefFormat(virConnectPtr conn,
>                            const virDomainDefPtr def,
>                            int secure);
> 
> Given a virDomainDefPtr object, generate a XML document describing the
> domain. The secure flag controls whether passwords are included in the
> XML output.

  hum, maybe it's a good idea to keep that flag generic with secure being just
one bit. Maybe things like difference between runtime and defined versions
will need to be provided there as an example too.

>   int virDomainCpuSetParse(virConnectPtr conn,
>                            const char **str,
>                            char sep,
>                            char *cpuset,
>                            int maxcpu);
>   char *virDomainCpuSetFormat(virConnectPtr conn,
>                               char *cpuset,
>                               int maxcpu);
> 
> These are just the APIs for dealing with CPU ranges moved from the xml.c
> file so they are in the expected place. A future patch will remove them
> from xml.c when the Xen driver is ported to this API.

  yup makes sense.

> There is no test suite explicitly against these parsers because when the
> QEMU and  Xen drivers are ported to this API their existing test suites
> exercise nearly all the code.

  no problem, full testing will come with the additional patches


Okay, I have read though the full patch, it's not trivial because it's both
new code due to the new data structure abut also old code based on the
previous parsing code. Diff really doesn't help there. I didn't spot anything
suspicious, i think the best strategy is to really push the switch in the
QEmu and Xen code and get as much testing as possible :-)

  Looks good, +1 it's definitely in the right direction !

Daniel

-- 
Red Hat Virtualization group http://redhat.com/virtualization/
Daniel Veillard      | virtualization library  http://libvirt.org/
veillard at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/