Hello,<br><br> I've modified my patch, according to comments in <a href="http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001751.html">http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001751.html</a>.<br>
<br> Directive has been renamed to xslt_param, taking 2 parameters - name and value separated (similarly to set directive),<br> therefore it's now also possible to pass ':' character as it had been requested before.<br>
I think it's better and easier to have it this way + there is no need to do that ugly parsing in runtime ;)<br><br>Thanks<br> Sam<br><br><div class="gmail_quote">On Tue, Jan 31, 2012 at 1:00 PM, <span dir="ltr"><<a href="mailto:nginx-devel-request@nginx.org" target="_blank">nginx-devel-request@nginx.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Message: 6<br>
Date: Tue, 31 Jan 2012 05:52:23 +0400<br>
From: Maxim Dounin <<a href="mailto:mdounin@mdounin.ru" target="_blank">mdounin@mdounin.ru</a>><br>
To: <a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
Subject: Re: [PATCH] add xslt_stylesheet_param directive<br>
Message-ID: <<a href="mailto:20120131015222.GH67687@mdounin.ru" target="_blank">20120131015222.GH67687@mdounin.ru</a>><br>
Content-Type: text/plain; charset=us-ascii<br>
<br>
Hello!<br>
<br>
On Tue, Jan 24, 2012 at 11:02:13PM +0100, SamB wrote:<br>
<br>
> Hi,<br>
> next try...<br>
><br>
> Attached patch adds xslt_stylesheet_param directive processing.<br>
> This is equivalent of xslt parameter passed to xslt_stylesheet, but this<br>
> works independently and globally.<br>
> xslt config should be a bit simpler because 'common' xslt variables<br>
> don't need to be repeated for each xslt_stylesheet.<br>
><br>
> Comments/suggestion are welcomed.<br>
<br>
You may have better lock pointing to a previous incarnation of the<br>
patch and outlining changes made. (And please post in plain text,<br>
not html.)<br>
<br>
It was previously submitted here:<br>
<br>
<a href="http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001669.html" target="_blank">http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001669.html</a><br>
<br>
And previous review was here:<br>
<br>
<a href="http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001670.html" target="_blank">http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001670.html</a><br>
<br>
<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>
> +++ nginx-1.1.13-new/src/http/modules/ngx_http_xslt_filter_module.c 2012-01-24 22:33:24.000000000 +0100<br>
> @@ -48,6 +48,7 @@<br>
> ngx_array_t sheets; /* ngx_http_xslt_sheet_t */<br>
> ngx_hash_t types;<br>
> ngx_array_t *types_keys;<br>
> + ngx_array_t params; /* ngx_http_complex_value_t */<br>
> } ngx_http_xslt_filter_loc_conf_t;<br>
><br>
><br>
> @@ -84,12 +85,16 @@<br>
> void *conf);<br>
> static char *ngx_http_xslt_stylesheet(ngx_conf_t *cf, ngx_command_t *cmd,<br>
> void *conf);<br>
> +static char *ngx_http_xslt_stylesheet_param(ngx_conf_t *cf, ngx_command_t *cmd,<br>
> + void *conf);<br>
> 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>
> static void *ngx_http_xslt_filter_create_conf(ngx_conf_t *cf);<br>
> +static char *ngx_http_xslt_filter_merge_params(const ngx_array_t *src, ngx_array_t *dest);<br>
> static char *ngx_http_xslt_filter_merge_conf(ngx_conf_t *cf, void *parent,<br>
> void *child);<br>
> +static ngx_int_t ngx_http_xslt_filter_preinit(ngx_conf_t *cf);<br>
> static ngx_int_t ngx_http_xslt_filter_init(ngx_conf_t *cf);<br>
> static void ngx_http_xslt_filter_exit(ngx_cycle_t *cycle);<br>
><br>
> @@ -116,6 +121,13 @@<br>
> 0,<br>
> NULL },<br>
><br>
> + { ngx_string("xslt_stylesheet_param"),<br>
> + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,<br>
> + ngx_http_xslt_stylesheet_param,<br>
> + NGX_HTTP_LOC_CONF_OFFSET,<br>
> + 0,<br>
> + NULL },<br>
<br>
I tend to think that "xslt_stylesheet_param" is too long.<br>
Something like "xslt_param" whould be better.<br>
<br>
> +<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>
> @@ -128,7 +140,7 @@<br>
><br>
><br>
> static ngx_http_module_t ngx_http_xslt_filter_module_ctx = {<br>
> - NULL, /* preconfiguration */<br>
> + ngx_http_xslt_filter_preinit, /* preconfiguration */<br>
<br>
The "...preinit" change is unrelated. If you want it to happen -<br>
please submit it seprately. (I think it's noop though.)<br>
<br>
> ngx_http_xslt_filter_init, /* postconfiguration */<br>
><br>
> ngx_http_xslt_filter_create_main_conf, /* create main configuration */<br>
> @@ -865,6 +877,46 @@<br>
> }<br>
><br>
><br>
> +static char *<br>
> +ngx_http_xslt_stylesheet_param(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)<br>
> +{<br>
> + ngx_http_xslt_filter_loc_conf_t *xlcf = conf;<br>
> +<br>
> + ngx_http_complex_value_t *param;<br>
> + ngx_http_compile_complex_value_t ccv;<br>
> + ngx_str_t *value;<br>
> +<br>
> + value = cf->args->elts;<br>
> +<br>
> + /* alloc array */<br>
> + if (xlcf->params.elts == NULL<br>
> + && ngx_array_init(&xlcf->params, cf->pool, 1, sizeof(ngx_http_complex_value_t))<br>
> + != NGX_OK)<br>
> + {<br>
> + return NGX_CONF_ERROR;<br>
> + }<br>
<br>
No lines longer than 80 chars, please. And alignment isn't<br>
styl-complaint either. In this particular case, please just use<br>
another nested if, much like in ngx_http_xslt_stylesheet():<br>
<br>
if (xlcf->sheets.elts == NULL) {<br>
if (ngx_array_init(&xlcf->sheets, cf->pool, 1,<br>
sizeof(ngx_http_xslt_sheet_t))<br>
!= NGX_OK)<br>
{<br>
return NGX_CONF_ERROR;<br>
}<br>
}<br>
<br>
> +<br>
> + param = ngx_array_push(&xlcf->params);<br>
> + if (param == NULL) {<br>
> + return NGX_CONF_ERROR;<br>
> + }<br>
> +<br>
> + ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));<br>
> +<br>
> + <a href="http://ccv.cf" target="_blank">ccv.cf</a> = cf;<br>
> + ccv.value = &value[1];<br>
> + ccv.complex_value = param;<br>
> + ccv.zero = 1;<br>
> +<br>
> + if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {<br>
> + return NGX_CONF_ERROR;<br>
> + }<br>
> +<br>
> + return NGX_CONF_OK;<br>
> +}<br>
> +<br>
> +<br>
> +<br>
<br>
2 blank lines between functions, please. 3 blank lines before<br>
ngx_http_xslt_stylesheet() is a bug, not a rule.<br>
<br>
Another question to consider: there was at least one request to<br>
simplify passing XPath expressions in params by allowing ':', see<br>
here:<br>
<br>
<a href="http://mailman.nginx.org/pipermail/nginx-devel/2010-July/000414.html" target="_blank">http://mailman.nginx.org/pipermail/nginx-devel/2010-July/000414.html</a><br>
<a href="http://mailman.nginx.org/pipermail/nginx-devel/2010-September/000486.html" target="_blank">http://mailman.nginx.org/pipermail/nginx-devel/2010-September/000486.html</a><br>
<br>
With separate directive we may want to allow this. Not sure<br>
though, probably it would be better to preserve compatibility with<br>
a way how parameters in xslt_stylesheet directive are handled.<br>
<br>
> static void<br>
> ngx_http_xslt_cleanup_dtd(void *data)<br>
> {<br>
> @@ -931,6 +983,37 @@<br>
><br>
><br>
> static char *<br>
> +ngx_http_xslt_filter_merge_params(const ngx_array_t *src, ngx_array_t *dest)<br>
> +{<br>
> + ngx_uint_t i;<br>
> +<br>
> + /* copy params */<br>
> + if (src->nelts && !dest->nelts) {<br>
> + dest->elts = src->elts;<br>
> + dest->nelts = src->nelts;<br>
> + }<br>
> + /* join params */<br>
> + else if (src->nelts && dest->nelts) {<br>
> + ngx_http_complex_value_t *params;<br>
> +<br>
> + params = src->elts;<br>
> + for (i = 0; i < src->nelts; i++) {<br>
> + ngx_http_complex_value_t *new;<br>
> +<br>
> + new = ngx_array_push(dest);<br>
> + if (new == NULL) {<br>
> + return NGX_CONF_ERROR;<br>
> + }<br>
> +<br>
> + *new = params[i]; /* copy */<br>
> + }<br>
> + }<br>
> +<br>
> + return NGX_CONF_OK;<br>
> +}<br>
> +<br>
> +<br>
> +static char *<br>
> ngx_http_xslt_filter_merge_conf(ngx_conf_t *cf, void *parent, void *child)<br>
> {<br>
> ngx_http_xslt_filter_loc_conf_t *prev = parent;<br>
> @@ -940,9 +1023,26 @@<br>
> conf->dtd = prev->dtd;<br>
> }<br>
><br>
> + if (ngx_http_xslt_filter_merge_params(&prev->params, &conf->params)<br>
> + != NGX_CONF_OK) {<br>
> + return NGX_CONF_ERROR;<br>
> + }<br>
<br>
As I already said in the previous review, params should follow the<br>
same logic as other array-type directives (access_log,<br>
proxy_set_header, fastcgi_param, error_page, ...). I.e. if you<br>
define any directive on a particular level, previous level data<br>
should not be inherited:<br>
<br>
location / {<br>
xslt_param foo=1;<br>
<br>
location /bar/ {<br>
xslt_param bar=1;<br>
}<br>
}<br>
<br>
In location /bar/ only "bar=1" parameter should be present.<br>
<br>
Please also note that with your code it's not possible to clear<br>
inherited params at all.<br>
<br>
> +<br>
> if (conf->sheets.nelts == 0) {<br>
> conf->sheets = prev->sheets;<br>
> }<br>
> + else {<br>
> + ngx_http_xslt_sheet_t *sheet;<br>
> + ngx_uint_t i;<br>
> +<br>
> + sheet = conf->sheets.elts;<br>
> + for (i = 0; i < conf->sheets.nelts; i++) {<br>
> + if (ngx_http_xslt_filter_merge_params(&conf->params, &sheet[i].params)<br>
> + != NGX_CONF_OK) {<br>
> + return NGX_CONF_ERROR;<br>
> + }<br>
> + }<br>
> + }<br>
<br>
This is wrong. Consider configuration like this:<br>
<br>
location / {<br>
xslt_stylesheet root.xslt;<br>
<br>
location /foo/ {<br>
xslt_params foo=bar;<br>
}<br>
}<br>
<br>
With the above merge logic in location /foo/ the root.xslt<br>
stylesheet will be used without parameters, while "foo=bar" is<br>
clearly expected.<br>
<br>
I suggest to use simple array-type inheritance for params, like<br>
<br>
if (conf->params == NULL) {<br>
conf->params = prev->params;<br>
}<br>
<br>
and apply them separately to the stylesheet at runtime. This will<br>
resolve both problems outlined above.<br>
<br>
><br>
> if (ngx_http_merge_types(cf, &conf->types_keys, &conf->types,<br>
> &prev->types_keys, &prev->types,<br>
> @@ -957,7 +1057,7 @@<br>
><br>
><br>
> static ngx_int_t<br>
> -ngx_http_xslt_filter_init(ngx_conf_t *cf)<br>
> +ngx_http_xslt_filter_preinit(ngx_conf_t *cf)<br>
> {<br>
> xmlInitParser();<br>
><br>
> @@ -965,6 +1065,13 @@<br>
> exsltRegisterAll();<br>
> #endif<br>
><br>
> + return NGX_OK;<br>
> +}<br>
> +<br>
> +<br>
> +static ngx_int_t<br>
> +ngx_http_xslt_filter_init(ngx_conf_t *cf)<br>
> +{<br>
> ngx_http_next_header_filter = ngx_http_top_header_filter;<br>
> ngx_http_top_header_filter = ngx_http_xslt_header_filter;<br>
><br>
<br>
See above, this is unrelated change.<br>
<br>
Maxim Dounin<br>
<br>
<br>
<br>
------------------------------<br>
<br>
_______________________________________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org" target="_blank">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>
<br>
End of nginx-devel Digest, Vol 27, Issue 26<br>
*******************************************<br>
</blockquote></div><br>