4:38 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
5:01 a.m.
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.
5:15 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
6480
days inactive
6480
days old
2 comments
2 participants
participants (2)
-
Daniel Veillard -
Mark McLoughlin