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

Hi Ryan and Eunice, First, thanks for the contribution ! > LDoms Support > > This patch adds the Logical Domains (LDoms) support for the SPARC > platforms. LDoms software is Sun Microsystem's virtualization technology > to subdivide a supported system's resources (CPUs, memory, I/O, and > storage) creating partitions called logical domains. The Logical Domains > Manager is used to create and manage logical domains and maps logical > domains to physical resources. The LDoms Manager provides a command-line > interface and also exports an XML-based control interface. The Libvirt > for LDoms uses this XML interface to communicate with the LDoms Manager > to retrieve the LDoms data for: > - Listing domains > - Requesting CPU and memory resource updates > - Performing life-cycle actions for logical domains > > This libvirt patch supports LDoms 1.0.1 and 1.0.2. okay > This patch will modify the following existing files: > src/libvirt.c > src/virsh.c > src/virterror.c > src/driver.h > src/Makefile.am > include/libvirt/libvirt.h.in > include/libvirt/virterror.h > configure.in > > and add the following new files: > src/ldoms_common.h > src/ldoms_internal.h > src/ldoms_internal.c > src/ldoms_intfc.h > src/ldoms_intfc.c > src/ldoms_xml_parse.h > src/ldoms_xml_parse.c 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).

> --- a/src/libvirt.c > +++ b/src/libvirt.c > @@ -48,6 +48,9 @@ > #ifdef WITH_LXC > #include "lxc_driver.h" > #endif > +#ifdef WITH_LDOMS > +extern int ldomsRegister(void); > +#endif 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.

> +#ifdef WITH_LDOMS > +/** > + * virLDomConsole: > + * @domain: the domain if available > + * > + * Opens a terminal window to the console for a domain > + * > + * Returns -1 in case of error, LDom console port number in case of success > + */ > +int > +virLDomConsole(virDomainPtr domain) 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.

> --- a/src/virsh.c Basically no change to virsh should be needed for adding a new driver, so it's a NACk here.

> diff --git a/src/virterror.c b/src/virterror.c In general most of the #ifdef/#ifndef WITH_LDOMS need to go. The public API need to be identical for all drivers and platforms.

> - > +#ifdef WITH_LDOMS > + case VIR_FROM_LDOMS: > + dom = "LDoms "; > + break; > +#endif [...] > diff --git a/src/driver.h b/src/driver.h > --- a/src/driver.h > +++ b/src/driver.h > @@ -24,7 +24,10 @@ typedef enum { > VIR_DRV_QEMU = 3, > VIR_DRV_REMOTE = 4, > VIR_DRV_OPENVZ = 5, > - VIR_DRV_LXC = 6 > + VIR_DRV_LXC = 6, > +#ifdef WITH_LDOMS > + VIR_DRV_LDOMS = 7 > +#endif > } virDrvNo; > > @@ -253,6 +256,11 @@ typedef virDomainPtr > const char *uri, > unsigned long flags); > > +#ifdef WITH_LDOMS > +typedef int > + (*virDrvLDomConsole) (virDomainPtr domain); > +#endif > + > typedef struct _virDriver virDriver; > typedef virDriver *virDriverPtr; > > @@ -337,6 +345,9 @@ struct _virDriver { > virDrvDomainInterfaceStats domainInterfaceStats; > virDrvNodeGetCellsFreeMemory nodeGetCellsFreeMemory; > virDrvNodeGetFreeMemory getFreeMemory; > +#ifdef WITH_LDOMS > + virDrvLDomConsole ldomConsole; > +#endif > }; 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. > typedef int > diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in > --- a/include/libvirt/libvirt.h.in > +++ b/include/libvirt/libvirt.h.in > @@ -549,6 +549,9 @@ typedef enum { > VIR_VCPU_OFFLINE = 0, /* the virtual CPU is offline */ > VIR_VCPU_RUNNING = 1, /* the virtual CPU is running */ > VIR_VCPU_BLOCKED = 2, /* the virtual CPU is blocked on resource */ > +#ifdef WITH_LDOMS > + VIR_VCPU_UNKNOWN = 3, /* the virtual CPU state is unknown */ > +#endif > } virVcpuState; > > typedef struct _virVcpuInfo virVcpuInfo; > diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h > --- a/include/libvirt/virterror.h > +++ b/include/libvirt/virterror.h > @@ -56,6 +56,9 @@ typedef enum { > VIR_FROM_STATS_LINUX, /* Error in the Linux Stats code */ > VIR_FROM_LXC, /* Error from Linux Container driver */ > VIR_FROM_STORAGE, /* Error from storage driver */ > +#ifdef WITH_LDOMS > + VIR_FROM_LDOMS, /* Error from LDoms driver */ > +#endif > } virErrorDomain; > > > @@ -139,6 +142,9 @@ typedef enum { > VIR_WAR_NO_STORAGE, /* failed to start storage */ > VIR_ERR_NO_STORAGE_POOL, /* storage pool not found */ > VIR_ERR_NO_STORAGE_VOL, /* storage pool not found */ > +#ifdef WITH_LDOMS > + VIR_ERR_INVALID_OPTION, /* invalid command line option */ > +#endif > } virErrorNumber; removal of all #ifdef/#endif , one API to bind them all

> diff --git a/configure.in b/configure.in > --- a/configure.in > +++ b/configure.in > @@ -246,6 +246,10 @@ if test "$with_remote" = "yes" ; then > LIBVIRT_FEATURES="$LIBVIRT_FEATURES -DWITH_REMOTE" > fi > > +if test "$with_ldoms" = "yes" ; then > + LIBVIRT_FEATURES="$LIBVIRT_FEATURES -DWITH_LDOMS" > +fi > + 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. > +++ b/src/ldoms_common.h > @@ -0,0 +1,80 @@ > +/* > + * ldoms_common.h: LDoms common definitions > + * > + * Copyright 2008 Sun Microsystems, Inc. > + * > + * See COPYING.LIB for the License of this software [...] looks fine to me but ... > +/* Debug print */ > +extern void dprt(const char *template, ...); 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.

> +++ b/src/ldoms_internal.h > @@ -0,0 +1,29 @@ [...] > +int ldomsRegister(void); #include "ldoms_internal.h" should be added in libvirt.c and then the declaration of ldomsRegister should be removed there

> +void ldomsError(virConnectPtr, virDomainPtr, virErrorNumber, const char*, int); [...] > diff --git a/src/ldoms_internal.c b/src/ldoms_internal.c [...] > + * Open a connection to the LDoms Manager and handshake to > + * verify the a connection can be made for future XML requests. interesting to see you are basically using XML as the intercahnge format with the manager. [...] > +/* > + * ldomsListDomains > + * > + * What: Return the array of Domain ID's for all the active Domains. > + * Send an XML request to LDM (LDoms Manager) for a list of all Domains. > + * > + * Yes, this functions does pretty much the same as ldomsNumOfDomains() with > + * the addition of returning the ID numbers for the valid Domains. > + * > + * Input: conn - The Connection structure > + * maxids - The total number of Domains to look at to > + * determine what Domain IDs to return. > + * Output: ids - An array of integers to hold the IDs of the > + * Domains whose state is other than 'inactive' - > + * VIR_DOMAIN_SHUTOFF 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.

[...] > +/* > + * ldomsDomainCreateXML > + * > + * What: Create a domain from an XML file so that it is left in the > + * inactive state. Just call the DefineXML function. The XML input > + * has to be a complete and valid XML requeset for the LDM. No > + * modification is done to this file. > + * > + * Input: xml - The xml file > + * Output: function return - A ptr to a virDomain or NULL > + */ > +virDomainPtr > +ldomsDomainCreateXML(virConnectPtr conn, const char *xml, unsigned int flags) > +{ > + virDomainPtr domPtr = NULL; > + char *domainName = NULL; > + > + if (ldoms_debug) dprt("LDOMS_DEBUG: ldomsDomainCreateXML(ENTER) \n"); > + if (ldoms_detailed_debug) dprt("LDOMS_DETAILED_DEBUG: ldomsDomainCreateXML(ENTER) xmldoc=\n%s\n",xml); > + > + /* Send the XML file along with the lifecycle action */ > + domainName = send_ldom_create_domain((char *)xml, XML_ADD_DOMAIN); > + > + /* > + * If the create/bind domain was successful, then we have to create a DomainInfo > + * structure to pass back to the caller. We need the Domain name that was > + * created, and the only way to get that is from parsing the input xml > + * document. This is done in send_ldom_create_domain() and passed back as the return. > + * Also we create the uuid for the domain name. > + */ 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.

[...] > + /* get the current ldom data from LDM */ > + (void) pthread_rwlock_wrlock(&update_lock); > + refresh_ldom_data(); > + (void) pthread_rwlock_unlock(&update_lock); > + 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.

> +/* > + * refresh_ldom_data > + * > + * Send an XML request to LDM for a list of all Domains > + * and get the basic information of each Domain. > + * > + * This function will be called for each commands to > + * guarantee that we have the current LDom info for any > + * virsh CLI command. > + * > + * Input: > + * Output: > + */ [...] > +/* > + * ldomsDomainDumpXML > + * > + * What: Send a list-constraints request to LDM for the domain specified > + * by the input virDomain. > + * > + * Input: domain - Pointer to a virDomain structure > + * Output: function return - char ptr to the XML data > + */ > +char * > +ldomsDomainDumpXML(virDomainPtr domain, int flags) [...] > + /* Dump the response to memory */ > + xml = malloc(sizeof(char) * 10000); > + xmlKeepBlanksDefault(0); 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. > + xmlDocDumpFormatMemory(xml_received, &xml, &xmlSize, 1); 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. [...] > +static void > +refresh_ldom_data() > +{ > + struct timeval tv; > + long delta; Question: why not do the locking here instead of around all the calling functions ? Simpler code, less error prone, no ?

> + /* Try and throttle calling the LDM to update the list of > + * LDoms. If the calls are 2 secs or less apart, then we just > + * use the current LDoms info. This is because each > + * command from virsh can turn into several calls into > + * function in this file, which all call this function > + * to refresh the LDom data. > + */ > + if (gettimeofday(&tv, NULL) >= 0) { > + /* See if the time since the last call to this functions > + * is less than 3 seconds. If so, just return. > + */ > + if ((delta=tv.tv_sec - last_ldom_refresh) < 3) { > + if (ldoms_detailed_debug) dprt("LDOMS_DETAILED_DEBUG: refresh_ldom_data(NO Refresh) delta=%d\n", > + delta); > + return; > + } > + last_ldom_refresh = tv.tv_sec; > + } [...] > +int ldomsRegister(void) { > + > + /* for debug statements */ > +#ifdef LDOMS_DEBUG > + ldoms_debug = 1; > + dprt("LDOMS_DEBUG on\n"); > +#endif > +#ifdef LDOMS_DETAILED_DEBUG > + ldoms_detailed_debug = 1; > + dprt("LDOMS_DETAILED_DEBUG on\n"); > +#endif 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.

[...] > diff --git a/src/ldoms_xml_parse.h b/src/ldoms_xml_parse.h > new file mode 100644 > --- /dev/null > +++ b/src/ldoms_xml_parse.h [...] > diff --git a/src/ldoms_xml_parse.c b/src/ldoms_xml_parse.c > new file mode 100644 a few comments on libxml2 use [...] > +xml_find_subnode(xmlNodePtr node, const xmlChar *name) > +{ > + xmlNodePtr subnode; > + > + subnode = node->xmlChildrenNode; > + while (subnode != NULL) { > + if (xmlStrcmp(subnode->name, name) == 0) > + break; 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.

[...] > + * Example: The following XML file will be created for LDom ldom1 > + * and a new memory value of 256 Kilobytes. > + * > + * <?xml version="1.0"?> > + * <LDM_interface version="1.0"> > + * <cmd> > + * <action>set-memory</action> > + * <data version="2.0"> > + * <ldom> > + * <ldom_info> > + * <ldom_name>ldom1</ldom_name> > + * </ldom_info> > + * <memory> > + * <size>256K</size> > + * </memory> > + * </ldom> > + * </data> > + * </cmd> > + * </LDM_interface> > + */ > + > +xmlDocPtr > +create_xml_file_4_set_memory(char *ldom_name, unsigned long memory) > +{ > + xmlDocPtr xml_output; > + xmlNodePtr root, cmd, data, ldom, info_node, mem; > + char mem_str[10]; /* ascii version of input int memory */ > + > + if (ldoms_debug) dprt("LDOMS_DEBUG: create_xml_file_4_set_memory(ENTER): ldom=%s, memory=%lu\n", > + (ldom_name==NULL? "NULL" : ldom_name), memory); > + > + xml_output = xmlNewDoc(XML_VERSION); > + > + /* <LDM_interface> tag with version sttribute */ > + root = xmlNewDocNode(xml_output, NULL, XML_LDM_INTERFACE, NULL); > + xmlDocSetRootElement(xml_output, root); > + xmlNewProp(root, VERSION_ATTR, LDOM_INTERFACE_VERSION); 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, [...] > +parse_xml_get_ldominfo(xmlDoc *doc, int *num_cpu, int *mem_size, int *mem_unit, int *num_crypto, int *num_iobus) > +{ > + data_node = xml_find_subnode(cmd_node, XML_DATA); > + if (data_node != NULL) > + ldom_node = xml_find_subnode(data_node, LDOM_NODE); > + else { > + if (ldoms_debug) dprt("LDOMS_DEBUG: parse_xml_get_ldominfo.. XML file does not have <data> tag\n"); > + return (ret); > + } > + > + if (ldom_node == NULL) { > + if (ldoms_debug) dprt("LDOMS_DEBUG: parse_xml_get_ldominfo.. XML file does not have <ldom> tag\n"); > + return (ret); > + } > + > + /* get number of cpu */ > + cpu_node = xml_find_subnode(ldom_node, CPU_NODE); > + if (cpu_node == NULL) { > + if (ldoms_debug) dprt("LDOMS_DEBUG: parse_xml_get_ldominfo.. XML file does not have <cpu> tag\n"); > + num_cpu_xml = 0; > + } else { > + subnode = xml_find_subnode(cpu_node, NUMBER_NODE); > + if (subnode == NULL) { > + if (ldoms_debug) dprt("LDOMS_DEBUG: parse_xml_get_ldominfo.. XML file does not have <number> tag within <cpu> tag\n"); > + num_cpu_xml = 0; > + } else { > + > + content = xmlNodeGetContent(subnode); > + > + num_cpu_xml = strtoll((char *)content, NULL, 0); > + if (num_cpu_xml <= 0) { > + if (ldoms_debug) dprt("LDOMS_DEBUG: parse_xml_get_ldominfo.. Invalid cpu num specified: %s\n", content); > + } > + xmlFree(content); > + } > + } 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