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

On Thu, Jun 26, 2008 at 08:36:46AM -0400, Daniel Veillard wrote: > [...] > > +static virNetworkDefPtr > > +virNetworkDefParseXML(virConnectPtr conn, > > + xmlDocPtr xml) > > +{ > > + xmlNodePtr root = NULL; > > + xmlXPathContextPtr ctxt = NULL; > > + virNetworkDefPtr def; > > + char *tmp; > > + > > + if (VIR_ALLOC(def) < 0) { > > + virNetworkReportError(conn, VIR_ERR_NO_MEMORY, NULL); > > + return NULL; > > + } > > + > > + /* Prepare parser / xpath context */ > > + root = xmlDocGetRootElement(xml); > > + if ((root == NULL) || (!xmlStrEqual(root->name, BAD_CAST "network"))) { > > + virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, > > + "%s", _("incorrect root element")); > > + goto error; > > + } > > One thing i'm wondering is if it would not make a bit more sense to cut > the API at a different level, getting the root element in the caller and > have the ParseXML() routines operates from an xmlNodePtr. That would allow > easy refactoring if we want to pack multiple XML definitions in a single > instance and just call the ParseXML() on each network (or other) subtree. Hmm, i think I'd probably go a ittle further still - this line you quote is the only place which uses the 'root' element directly - the rest is all just XPath queries. So if we changed that to just query '/network' or similar, we can just pass an xmlXPathContextPtr object into this method instead.

> Do the Root lookup here and pass it down Or ctxt = xmlXPathNewContext(xml); And just pass 'ctxt' in.

> Hum, what about just pointing to the COPYING.LIB file instead of > including the text below ? I prefer to keep the full boiler-plate text since that's what's recommended by the COPYING.LIB file itself. It also means that if someone takes libvirt code and merges it into another library the copying terms are clearly shown.