[netcf-devel] [PATCH] Fix initialization of libxslt

Daniel Veillard veillard at redhat.com
Fri Aug 14 12:20:43 UTC 2009


On Fri, Aug 14, 2009 at 09:48:33AM +0200, Daniel Veillard wrote:
> On Thu, Aug 13, 2009 at 06:12:38PM -0700, David Lutterkort wrote:
> > We used to initialize libxslt (and register extension modules) every time
> > ncf_init was called, and deregister/free libxslt globals every time
> > ncf_close was called. That breaks when libnetcf is used together with other
> > libraries that use libxslt, since they might continue using it after a call
> > to ncf_close.
> > 
> > To work around this, we now initialize and register our extension modules
> > only once, essentially statically, and just live with the small leak that
> > not unregistering and cleaning up globals causes.
> > 
> > Bug report and suggested fix from Daniel Veillard (<veillard at redhat.com>)
> 
>   Yes, thanks, the patch does what I think is needed to avoid threading
> problems with libxslt in netcf, unfortunately the modules registered
> globally can't be unplugged in ncf_close as other threads may still use
> netcf and need those, same for the global variables of libxslt.
>   My suggestion is that applications using netcf should call
> 
>   - xslt_ext_unregister(); <- or another public cleanup function
>                               to be called when libnetcf is not used
>                               anymore in the application
>   - xsltCleanupGlobals();
>   - xmlCleanupParser();
> 
> when they are about to exit, this will allow to have clean runs when
> using valgrind --leak-check or other leak checking tools on the
> application. The amount of memory left used is rather minimal, most of
> the data like the stylesheet are still cleaned up on ncf_close()
> the amount maintained by the libraries should be in the order of
> a few kilobytes, neglectible by current standards.

  Actually I think there is a nicer way to do this, by never
registering any global extensions, just registering the functions
on the transformation context. This mean you just call xsltInit()
from the initialization routine, and forget about any global
registration altogether, no need to add pthread specific changes
either, see patch attached. It also take care of one of your
TODOs i.e detect XSLT failures, you should just check that
return values from apply_stylesheet() are not NULL (left TODO).

  Oh another cleanup is that you should not call
xmlDocDumpFormatMemory() on an XSLT output document but use
one of the xsltSaveResultTo...() function which will handle
the xsl:output parameters from the stylesheet, in the case of
drv_xml_desc() use xsltSaveResultToString()

http://xmlsoft.org/XSLT/html/libxslt-xsltutils.html#xsltSaveResultToString

 not in that patch since it's orthogonal to the topic of this patch,
I think the patch works okay, the only thing missing is that
  int register_exts(xsltTransformContextPtr ctxt)
should be exported internally in some .h but I wasn't sure where you
would prefer to do this... Compiles and passes regression tests for
me,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/
-------------- next part --------------
diff --git a/src/drv_initscripts.c b/src/drv_initscripts.c
index 550df65..ee7f082 100644
--- a/src/drv_initscripts.c
+++ b/src/drv_initscripts.c
@@ -245,8 +245,6 @@ static void bridge_physdevs(struct netcf *ncf) {
 }
 
 int drv_init(struct netcf *ncf) {
-    int r;
-
     if (ALLOC(ncf->driver) < 0)
         return -1;
 
@@ -256,8 +254,6 @@ int drv_init(struct netcf *ncf) {
 
     // FIXME: Check for errors
     xsltInit();
-    r = xslt_ext_register();
-    ERR_THROW(r < 0, ncf, EINTERNAL, "xsltRegisterExtModule failed");
     ncf->driver->get = parse_stylesheet(ncf, "initscripts-get.xsl");
     ncf->driver->put = parse_stylesheet(ncf, "initscripts-put.xsl");
     ncf->driver->rng = rng_parse(ncf, "interface.rng");
@@ -280,8 +276,6 @@ int drv_init(struct netcf *ncf) {
 void drv_close(struct netcf *ncf) {
     xsltFreeStylesheet(ncf->driver->get);
     xsltFreeStylesheet(ncf->driver->put);
-    xslt_ext_unregister();
-    xsltCleanupGlobals();
     if (ncf->driver->ioctl_fd >= 0)
         close(ncf->driver->ioctl_fd);
     aug_close(ncf->driver->augeas);
@@ -504,7 +498,7 @@ char *drv_xml_desc(struct netcf_if *nif) {
     ERR_BAIL(ncf);
 
     aug_xml = aug_get_xml(ncf, nint, intf);
-    ncf_xml = xsltApplyStylesheet(ncf->driver->put, aug_xml, NULL);
+    ncf_xml = apply_stylesheet(ncf, ncf->driver->put, aug_xml);
 
     xmlDocDumpFormatMemory(ncf_xml, (xmlChar **) &result, NULL, 1);
 
@@ -602,8 +596,7 @@ struct netcf_if *drv_define(struct netcf *ncf, const char *xml_str) {
     r = aug_rm(aug, path);
     ERR_COND_BAIL(r < 0, ncf, EOTHER);
 
-    // FIXME: Check for errors from ApplyStylesheet
-    aug_xml = xsltApplyStylesheet(ncf->driver->get, ncf_xml, NULL);
+    aug_xml = apply_stylesheet(ncf, ncf->driver->get, ncf_xml);
 
     aug_put_xml(ncf, aug_xml);
     ERR_BAIL(ncf);
diff --git a/src/dutil.c b/src/dutil.c
index 28832b3..8b4d3df 100644
--- a/src/dutil.c
+++ b/src/dutil.c
@@ -200,6 +200,28 @@ xsltStylesheetPtr parse_stylesheet(struct netcf *ncf,
     return result;
 }
 
+xmlDocPtr apply_stylesheet(struct netcf *ncf, xsltStylesheetPtr style,
+                           xmlDocPtr doc) {
+    xsltTransformContextPtr ctxt;
+    xmlDocPtr res = NULL;
+    int r;
+
+    ctxt = xsltNewTransformContext(style, doc);
+    ERR_COND_BAIL(ctxt == NULL, ncf, ENOMEM);
+
+    r = register_exts(ctxt);
+    ERR_COND_BAIL(r < 0, ncf, ENOMEM);
+
+    res = xsltApplyStylesheetUser(style, doc, NULL, NULL, NULL, ctxt);
+    if ((ctxt->state == XSLT_STATE_ERROR) ||
+        (ctxt->state == XSLT_STATE_STOPPED))
+        report_error(ncf, NETCF_EXSLTFAILED,
+           "XSLT transformation failed");
+
+error:
+    xsltFreeTransformContext(ctxt);
+    return res;
+}
 /* Callback for reporting RelaxNG errors */
 void rng_error(void *ctx, const char *format, ...) {
     struct netcf *ncf = ctx;
@@ -354,8 +376,7 @@ int dutil_get_aug(struct netcf *ncf, const char *ncf_xml, char **aug_xml) {
     rng_validate(ncf, ncf_doc);
     ERR_BAIL(ncf);
 
-    // FIXME: Check for errors from ApplyStylesheet
-    aug_doc = xsltApplyStylesheet(ncf->driver->get, ncf_doc, NULL);
+    aug_doc = apply_stylesheet(ncf, ncf->driver->get, ncf_doc);
 
     xmlDocDumpFormatMemory(aug_doc, (xmlChar **) aug_xml, NULL, 1);
     ERR_COND_BAIL(*aug_xml == NULL, ncf, EXMLINVALID);
@@ -375,8 +396,7 @@ int dutil_put_aug(struct netcf *ncf, const char *aug_xml, char **ncf_xml) {
     aug_doc = parse_xml(ncf, aug_xml);
     ERR_BAIL(ncf);
 
-    // FIXME: Check for errors from ApplyStylesheet
-    ncf_doc = xsltApplyStylesheet(ncf->driver->put, aug_doc, NULL);
+    ncf_doc = apply_stylesheet(ncf, ncf->driver->put, aug_doc);
 
     xmlDocDumpFormatMemory(ncf_doc, (xmlChar **) ncf_xml, NULL, 1);
     ERR_COND_BAIL(*ncf_xml == NULL, ncf, EXMLINVALID);
diff --git a/src/dutil.h b/src/dutil.h
index 6dc05e2..fbf11fb 100644
--- a/src/dutil.h
+++ b/src/dutil.h
@@ -64,6 +64,10 @@ void free_matches(int nint, char ***intf);
 /* Parse an XSLT stylesheet residing in the file NCF->data_dir/xml/FNAME */
 xsltStylesheetPtr parse_stylesheet(struct netcf *ncf, const char *fname);
 
+/* Apply an XSLT stylesheet to a document with our extensions */
+xmlDocPtr apply_stylesheet(struct netcf *ncf, xsltStylesheetPtr style,
+                           xmlDocPtr doc);
+
 /* Callback for reporting RelaxNG errors */
 void rng_error(void *ctx, const char *format, ...);
 
diff --git a/src/netcf.h b/src/netcf.h
index 032be78..e08e94b 100644
--- a/src/netcf.h
+++ b/src/netcf.h
@@ -48,8 +48,9 @@ typedef enum {
     NETCF_ENOENT,        /* Required entry in a tree is missing */
     NETCF_EEXEC,         /* external program execution failed or returned
                           * non-0 */
-    NETCF_EINUSE         /* attempt to close a netcf instance that is still
+    NETCF_EINUSE,        /* attempt to close a netcf instance that is still
                           * used by other data structures */
+    NETCF_EXSLTFAILED    /* XSLT transformation failed */
 } netcf_errcode_t;
 
 
diff --git a/src/xslt_ext.c b/src/xslt_ext.c
index ea6d916..d599212 100644
--- a/src/xslt_ext.c
+++ b/src/xslt_ext.c
@@ -169,34 +169,26 @@ static void bond_option(xmlXPathParserContextPtr ctxt, int nargs) {
     xmlFree(bond_opts);
 }
 
-static void *xslt_ipcalc_init(ATTRIBUTE_UNUSED xsltTransformContextPtr ctxt,
-                              ATTRIBUTE_UNUSED const xmlChar *URI) {
-    xsltRegisterExtModuleFunction(BAD_CAST "netmask", XSLT_EXT_IPCALC_NS,
-                                  ipcalc_netmask);
-    xsltRegisterExtModuleFunction(BAD_CAST "prefix", XSLT_EXT_IPCALC_NS,
-                                  ipcalc_prefix);
-    return NULL;
-}
+int register_exts(xsltTransformContextPtr ctxt) {
+    int r;
 
-static void *xslt_bond_init(ATTRIBUTE_UNUSED xsltTransformContextPtr ctxt,
-                            ATTRIBUTE_UNUSED const xmlChar *URI) {
-    xsltRegisterExtModuleFunction(BAD_CAST "option", XSLT_EXT_BOND_NS,
-                                  bond_option);
-    return NULL;
-}
+    r = xsltRegisterExtFunction(ctxt, BAD_CAST "netmask", XSLT_EXT_IPCALC_NS,
+                                ipcalc_netmask);
+    if (r < 0)
+        return(r);
 
-int xslt_ext_register(void) {
-    int r = 0;
-    r = xsltRegisterExtModule(XSLT_EXT_IPCALC_NS, xslt_ipcalc_init, NULL);
-    if (r < 0) return r;
-    r = xsltRegisterExtModule(XSLT_EXT_BOND_NS, xslt_bond_init, NULL);
-    return r;
-}
+    r = xsltRegisterExtFunction(ctxt, BAD_CAST "prefix", XSLT_EXT_IPCALC_NS,
+                                ipcalc_prefix);
+    if (r < 0)
+        return(r);
 
-void xslt_ext_unregister(void) {
-    xsltUnregisterExtModule	(XSLT_EXT_IPCALC_NS);
+    r = xsltRegisterExtFunction(ctxt, BAD_CAST "option", XSLT_EXT_BOND_NS,
+                                bond_option);
+    if (r < 0)
+        return(r);
 }
 
+
 /*
  * Local variables:
  *  indent-tabs-mode: nil


More information about the netcf-devel mailing list