[Libvir] [PATCH] XPath accessors cleanup
It a long time cleanup TODO, the XML parsing code of xml.c is invaded with libxml2 specific XPath lookup code, this patch defines 5 clearly isolated accessor functions and then fixes all the src/xml.c code to use those. IMHO this clearly improve readability and maintanability and I think I also found a couple of bugs in the process. The patch enclosed only fixes xml.c and xml.h, the next step is to chase other modules to apply the same treatment if needed. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Hi Daniel, Nice stuff ! On Fri, 2007-03-30 at 05:38 -0400, Daniel Veillard wrote:
+char * +virXPathString(const char *xpath, xmlXPathContextPtr ctxt) { ... + ret = (char *) obj->stringval; + obj->stringval = NULL;
+int +virXPathNodeSet(const char *xpath, xmlXPathContextPtr ctxt, xmlNodePtr **list) { ... + if (list != NULL) { + *list = obj->nodesetval->nodeTab; + obj->nodesetval->nodeTab = NULL; + }
This is all a little voodooish to me - e.g. you're assuming that libxml2 allocated these with malloc() and you're also kind of breaking the abstraction of xmlXPathObject by manipulating the structure like this. i.e. you know this works, because you wrote libxml2, but you're not using the abstraction very cleanly. I'd suggest either: - copy the memory from the xmlXPathObject and return that - make the functions return "libxml memory" and use xmlFree() in the callers Cheers, Mark.
On Fri, Mar 30, 2007 at 11:01:25AM +0100, Mark McLoughlin wrote:
Hi Daniel, Nice stuff !
On Fri, 2007-03-30 at 05:38 -0400, Daniel Veillard wrote:
+char * +virXPathString(const char *xpath, xmlXPathContextPtr ctxt) { ... + ret = (char *) obj->stringval; + obj->stringval = NULL;
+int +virXPathNodeSet(const char *xpath, xmlXPathContextPtr ctxt, xmlNodePtr **list) { ... + if (list != NULL) { + *list = obj->nodesetval->nodeTab; + obj->nodesetval->nodeTab = NULL; + }
This is all a little voodooish to me - e.g. you're assuming that libxml2 allocated these with malloc() and you're also kind of breaking the abstraction of xmlXPathObject by manipulating the structure like this.
Right, sorry I just wanted to make the API and code as less dependant from libxml2 idioms as possible, but you raise good point ;-)
i.e. you know this works, because you wrote libxml2, but you're not using the abstraction very cleanly.
I'd suggest either:
- copy the memory from the xmlXPathObject and return that
- make the functions return "libxml memory" and use xmlFree() in the callers
I prefer to copy and limit the libxml2 expertise needed to understand the normal parsing code, so I will implement #1, there is no harm is doing a few extra malloc()/free() at that point there. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
participants (2)
-
Daniel Veillard -
Mark McLoughlin