Re: [libvirt] PATCH: Generic internal API for domain XML parser/formatter
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/