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@xxxxxxxxxx | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/ -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list