Re: [Libvir] [PATCH][RFC] libvirt ldoms support

Hi Ryan and Eunice, First, thanks for the contribution ! okay I think most of the issues have been raised in the thread started by Richard One think I appreciate in the code is the amount of comments (would have been perfect if the function comment style could be realigned to libvirt one, i don't know if that can be done automatically). Do you have example of the XML domain configurations, it's a bit hard to derive from the parsing code, and i would prefer to have them aligned with other container/partition hypervisors. And then have them documented in the public format pages like the others http://libvirt.org/format.html (we need to add OpenVZ/LinuxContainers exampels too there). Sounds to me it's cleaner to export the API from an ldom driver header and include the header, that way the signature is checked between the producer and the consumer. virLDomConsole is really a no go, this can't be added to the list of exported symbols from the library. this was already discussed in the thread, but I want to raise it again w.r.t. the exported symbols set, this is regulated on the gcc tool chain with the src/libvirt_sym.version file, it's not modified anytwhere in the file and since you use virLDomConsole from virsh this implies that you export all C public symbols by default in the library, I really think this need fixing, we can add a specific linker file for Solaris in the distribution. that's not a problem, but the exported symbols set need to be controlled. Basically no change to virsh should be needed for adding a new driver, so it's a NACk here. In general most of the #ifdef/#ifndef WITH_LDOMS need to go. The public API need to be identical for all drivers and platforms. [...] in the short term export of the console informations in the XML is the normal option, then a generic console extracting API may be provided if needed , but adding this is premature. removal of all #ifdef/#endif , one API to bind them all I'm a bit surprized to not see the AC_ARG_WITH(ldom, .... option block in the configure.in, was that dropped from the patch ? I think this should be in a large part based on patform autodetection and the --with-ldom/--without-ldom should only be used for code portability tests and development. [...] looks fine to me but ... i would prefer to see the debug integrated with the existing debug support. Since we now compile with debug support enabled by default it is really useful to allow user provide debug informations e.g. by running the virsh command after having set the LIVIRT_DEBUG=1 environment variable. If you have more complex debugging need (levels of debug) the existing routine should be able to cope with this (see virErrorLevel in virterror.h). All those if (ldoms_debug) dprt(...) should be relatively easy to unify into DEBUG() macros like in other parts of the library. [...] #include "ldoms_internal.h" should be added in libvirt.c and then the declaration of ldomsRegister should be removed there [...] [...] interesting to see you are basically using XML as the intercahnge format with the manager. [...] I'm a bit confused about how active/inactive domains are handled in lDOM, and i don't really have a way to just get my hand on it. [...] I am completely unable to find informations about the XML format for a domain i looked at send_ldom_create_domain and it seems to send the XML directly to the domain manager, ther don't seems to be any parsing or analysis or checking of the XML input data at the libvirt level. To me this is a bit problematic as the XML is part of the API in practice. This really need to be documentd as the support is added to libvirt. [...] I'm a bit surprized by this, this looks a bit heavy to keep a complete list in libvirt memory updated with a global lock, that doesn't look optimal for example when vish asks for the list of domain you need first to update to get the number of domains and then reupdate in the next call(s) to extract the domain data. And that won't protect you from races anyway, so i'm a bit surprized. [...] [...] hum, i usually suggest libxml2 users don't use xmlKeepBlanksDefault anymore as this uses a global variable and modify the behaviour of all libxml2 processing including parsing, this is especially nasty in a library. the 1 for the format argument should be sufficient Also using xmlSaveToBuffer http://xmlsoft.org/html/libxml-xmlsave.html#xmlSaveToBuffer would avoid a fixed output buffer limitation which is really not necessary. I'm still a bit puzzled that the manager uses the libvirt XML format directly as the input. [...] Question: why not do the locking here instead of around all the calling functions ? Simpler code, less error prone, no ? [...] maybe the debug vs detailed_debug could be integarted in libvirt normal debugging model, for example based on the value of LIBVIRT_DEBUG env variable. [...] [...] a few comments on libxml2 use [...] not that with recent libxml2 (any 2.6.x will do) all the markup name strings are interned in a dictionary associated to the document. With a bit of tweaking on your parsing routines would could also get all domuments parsed to share the same dictionary 9basically by always reusing the same XML parsing context, meaning all those strings could be interned for the whole process and let you fallback to pure string pointer comparisons, instead of comparing content. But it's probably a bit complex unless you're chasing performances issues. [...] In practice building the tree may be a bit more complex than generating the XmL serialization directly makes for simpler code and you don't have to serialize later, [...] Direct XPath queries on the tree make for simpler code, and are probably as efficient (if needed the XPath expressions can be precompiled) virXPathLong("number(//ldom/memory/size[1], ctxt, &size) is more readable I'm sure, should simplify long term maintainance too, it's easy to forget an xmlFree in some path ... I'm not gonna complain, but it seems you had to deal with libxml2 way more than necessary, that's make for an awful lot of code in the end too. 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/