Hi,<br> I'll fix my non-string patch, here are my explanations to your comments ;)<br><br>Sam<br><br><div class="gmail_quote">On Fri, Feb 10, 2012 at 12:12 AM, Maxim Dounin <span dir="ltr"><<a href="mailto:mdounin@mdounin.ru">mdounin@mdounin.ru</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello!<br>
<div class="im"><br>
On Wed, Feb 08, 2012 at 01:55:47PM +0100, SamB wrote:<br>
<br>
> Hi,<br>
><br>
> I've added xslt_string_param and slightly changed parameter passing and<br>
> transformation<br>
> to make it work as expected.<br>
> As a side effect of this change, might be usage of<br>
> xslt_param/xslt_string_param faster,<br>
> because params are directly inserted to transformation context and do<br>
> not need to be<br>
> copied to name/value parameter array that is passed to<br>
> xsltApplyStylesheet() as it's now<br>
> by parameters passed via xslt_stylesheet directive.<br>
><br>
> Old parameter passing using xslt_stylesheet has not been changed to<br>
> prevent unexpected behavior,<br>
> maybe this can be modified later if this patch will prove to be stable...<br>
<br>
</div>This looks intresting, though may be it would be better to finish<br>
and commit the xslt_param patch first (see another message for a<br>
review).<br>
<br>
Below is review of the xslt_param + xslt_string_param patch<br>
specific bits.<br>
<br>
[...]<br>
<div class="im"><br>
> diff -ur nginx-1.1.13/src/http/modules/ngx_http_xslt_filter_module.c nginx-1.1.13-new/src/http/modules/ngx_http_xslt_filter_module.c<br>
> --- nginx-1.1.13/src/http/modules/ngx_http_xslt_filter_module.c 2010-07-12 14:52:01.000000000 +0200<br>
</div>> +++ nginx-1.1.13-new/src/http/modules/ngx_http_xslt_filter_module.c 2012-02-08 13:34:43.000000000 +0100<br>
> @@ -10,6 +10,7 @@<br>
><br>
> #include <libxml/parser.h><br>
> #include <libxml/tree.h><br>
> +#include <libxslt/variables.h><br>
> #include <libxslt/xslt.h><br>
> #include <libxslt/xsltInternals.h><br>
> #include <libxslt/transform.h><br>
> @@ -48,6 +49,8 @@<br>
<div class="im">> ngx_array_t sheets; /* ngx_http_xslt_sheet_t */<br>
> ngx_hash_t types;<br>
> ngx_array_t *types_keys;<br>
</div>> + ngx_array_t string_params; /* ngx_http_complex_value_t */<br>
<div class="im">> + ngx_array_t params; /* ngx_http_complex_value_t */<br>
<br>
</div>I would like to see single "params" array instead, with both<br>
string params and xpath ones.<br>
<br></blockquote><div>Ok, I will merge them and distinguish by flag.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[...]<br>
<br>
> @@ -84,6 +90,13 @@<br>
<div class="im">> void *conf);<br>
> static char *ngx_http_xslt_stylesheet(ngx_conf_t *cf, ngx_command_t *cmd,<br>
> void *conf);<br>
</div>> +static char *ngx_http_xslt_add_param(ngx_conf_t *cf, ngx_command_t *cmd,<br>
> + ngx_array_t *params);<br>
<div class="im">> +static char *ngx_http_xslt_stylesheet_param(ngx_conf_t *cf, ngx_command_t *cmd,<br>
> + void *conf);<br>
</div>> +static char *ngx_http_xslt_stylesheet_string_param(ngx_conf_t *cf,<br>
<div class="im">> + ngx_command_t *cmd,<br>
> + void *conf);<br>
<br>
</div>There is no need for 3 functions here. Just one is enough.<br>
<br>
In your current code, you may (and probably have to) use "offset"<br>
field of the command structure to pass offset of array you want to<br>
work with. With single params array as suggested earlier - you<br>
may use "post" field to distinguish string and non-string<br>
versions.<br>
<div class="im"><br></div></blockquote><div><br>Ok.<br>Regarding rename of ngx_http_xslt_stylesheet_param to shorter ngx_http_xslt_param,<br>I've had again problem with existing function with similar name ngx_http_xslt_params.<br>
</div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">
> static void ngx_http_xslt_cleanup_dtd(void *data);<br>
> static void ngx_http_xslt_cleanup_stylesheet(void *data);<br>
> static void *ngx_http_xslt_filter_create_main_conf(ngx_conf_t *cf);<br>
</div>> @@ -116,6 +129,20 @@<br>
> 0,<br>
> NULL },<br>
><br>
> + { ngx_string("xslt_param"),<br>
> + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,<br>
<div class="im">> + ngx_http_xslt_stylesheet_param,<br>
> + NGX_HTTP_LOC_CONF_OFFSET,<br>
> + 0,<br>
> + NULL },<br>
</div>> +<br>
> + { ngx_string("xslt_string_param"),<br>
> + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,<br>
> + ngx_http_xslt_stylesheet_string_param,<br>
<div class="im">> + NGX_HTTP_LOC_CONF_OFFSET,<br>
> + 0,<br>
> + NULL },<br>
</div><div class="im">> +<br>
> { ngx_string("xslt_types"),<br>
> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_1MORE,<br>
> ngx_http_types_slot,<br>
</div>> @@ -450,6 +477,7 @@<br>
> ngx_uint_t i;<br>
> xmlChar *buf;<br>
> xmlDocPtr doc, res;<br>
> + xsltTransformContextPtr xctxt;<br>
<br>
In libxslt this is usually called "ctxt", I think we should<br>
follow.<br>
<br></blockquote><div><br>There exists variable ctx already in this context, and I find them too similar therefore this way.<br><br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> ngx_http_xslt_sheet_t *sheet;<br>
> ngx_http_xslt_filter_loc_conf_t *conf;<br>
><br>
> @@ -468,14 +496,28 @@<br>
<div class="im">><br>
> for (i = 0; i < conf->sheets.nelts; i++) {<br>
><br>
</div>> - if (ngx_http_xslt_params(r, ctx, &sheet[i].params) != NGX_OK) {<br>
> + xctxt = xsltNewTransformContext(sheet[i].stylesheet, doc);<br>
> + if (xctxt == NULL) {<br>
> xmlFreeDoc(doc);<br>
> return NULL;<br>
> }<br>
><br>
> - res = xsltApplyStylesheet(sheet[i].stylesheet, doc, ctx->params.elts);<br>
> + xctxt->initialContextDoc = doc;<br>
> + xctxt->initialContextNode = (xmlNodePtr) doc;<br>
<br>
According to<br>
<br>
<a href="http://xmlsoft.org/XSLT/html/libxslt-xsltInternals.html" target="_blank">http://xmlsoft.org/XSLT/html/libxslt-xsltInternals.html</a><br>
<br>
this is internal to libxslt, and using it may not be a good idea.<br>
Are you sure it's needed?<br>
<br>
As far as I can see in sources, it's ceratainly *not* needed for<br>
xsltQuoteOneUserParam(). Not sure about xsltEvalOneUserParam().<br>
<br></blockquote><div><br>This is needed for xsltEvalOneUserParam() to work correctly, there is no<br>other way how to make it work (libxslt bug?), and I don't think its<br>risky to use it.<br>But, I can avoid use of xsltEvalOneUserParam() at all and pass params<br>
in the old way.<br><br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> + if (ngx_http_xslt_params(r, ctx, conf, xctxt,<br>
> + &sheet[i].params) != NGX_OK) {<br>
> + xmlFreeDoc(doc);<br>
> + xsltFreeTransformContext(xctxt);<br>
<br>
It may be a good idea to free this in reverse order, i.e.<br>
transformation context first.<br>
<br></blockquote><div>Ok.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + return NULL;<br>
> + }<br>
> +<br>
> + res = xsltApplyStylesheetUser(sheet[i].stylesheet, doc,<br>
> + ctx->params.elts,<br>
> + NULL, NULL, xctxt);<br>
><br>
> xmlFreeDoc(doc);<br>
> + xsltFreeTransformContext(xctxt);<br>
<br>
Same here.<br>
<br>
><br>
> if (res == NULL) {<br>
> ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,<br>
> @@ -564,14 +606,92 @@<br>
<br>
[...]<br>
<font color="#888888"><br>
Maxim Dounin<br>
</font><div><div></div><div class="h5"><br>
_______________________________________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org">nginx-devel@nginx.org</a><br>
<a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
</div></div></blockquote></div><br>