On Thu, Jun 26, 2008 at 02:48:58PM +0100, Daniel P. Berrange wrote:
> 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.
hum, not that simple one actually need to make the XPath queries relative
for this to work on the subtree independantly of the location of the top node
in the document tree. And it's better to set back the current node for the
query after each XPath lookup (i.e. reset ctxt->node back to the subtree top
node before each XPath lookup).
> > Do the Root lookup here and pass it down
>
> Or
>
> ctxt = xmlXPathNewContext(xml);
>
> And just pass 'ctxt' in.
I think we still need to pass the node in practice to be safe.
> > 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.
Hum, I suggested Dan Smith to do it the other way a couple days ago :-\ ...
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