Hello!

On Tue, Jan 24, 2012 at 11:02:13PM +0100, SamB wrote:

> Hi,
> next try...
>
> Attached patch adds xslt_stylesheet_param directive processing.
> This is equivalent of xslt parameter passed to xslt_stylesheet, but this
> works independently and globally.
> xslt config should be a bit simpler because 'common' xslt variables
> don't need to be repeated for each xslt_stylesheet.
>
> Comments/suggestion are welcomed.

You may have better lock pointing to a previous incarnation of the
patch and outlining changes made. (And please post in plain text,
not html.)

It was previously submitted here:

http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001669.html

And previous review was here:

http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001670.html


> 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
> --- nginx-1.1.13/src/http/modules/ngx_http_xslt_filter_module.c 2010-07-12 14:52:01.000000000 +0200
> +++ nginx-1.1.13-new/src/http/modules/ngx_http_xslt_filter_module.c 2012-01-24 22:33:24.000000000 +0100
> @@ -48,6 +48,7 @@
> ngx_array_t sheets; /* ngx_http_xslt_sheet_t */
> ngx_hash_t types;
> ngx_array_t *types_keys;
> + ngx_array_t params; /* ngx_http_complex_value_t */
> } ngx_http_xslt_filter_loc_conf_t;
>
>
> @@ -84,12 +85,16 @@
> void *conf);
> static char *ngx_http_xslt_stylesheet(ngx_conf_t *cf, ngx_command_t *cmd,
> void *conf);
> +static char *ngx_http_xslt_stylesheet_param(ngx_conf_t *cf, ngx_command_t *cmd,
> + void *conf);
> static void ngx_http_xslt_cleanup_dtd(void *data);
> static void ngx_http_xslt_cleanup_stylesheet(void *data);
> static void *ngx_http_xslt_filter_create_main_conf(ngx_conf_t *cf);
> static void *ngx_http_xslt_filter_create_conf(ngx_conf_t *cf);
> +static char *ngx_http_xslt_filter_merge_params(const ngx_array_t *src, ngx_array_t *dest);
> static char *ngx_http_xslt_filter_merge_conf(ngx_conf_t *cf, void *parent,
> void *child);
> +static ngx_int_t ngx_http_xslt_filter_preinit(ngx_conf_t *cf);
> static ngx_int_t ngx_http_xslt_filter_init(ngx_conf_t *cf);
> static void ngx_http_xslt_filter_exit(ngx_cycle_t *cycle);
>
> @@ -116,6 +121,13 @@
> 0,
> NULL },
>
> + { ngx_string("xslt_stylesheet_param"),
> + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
> + ngx_http_xslt_stylesheet_param,
> + NGX_HTTP_LOC_CONF_OFFSET,
> + 0,
> + NULL },

I tend to think that "xslt_stylesheet_param" is too long.
Something like "xslt_param" whould be better.

> +
> { ngx_string("xslt_types"),
> NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_1MORE,
> ngx_http_types_slot,
> @@ -128,7 +140,7 @@
>
>
> static ngx_http_module_t ngx_http_xslt_filter_module_ctx = {
> - NULL, /* preconfiguration */
> + ngx_http_xslt_filter_preinit, /* preconfiguration */

The "...preinit" change is unrelated. If you want it to happen -
please submit it seprately. (I think it's noop though.)

> ngx_http_xslt_filter_init, /* postconfiguration */
>
> ngx_http_xslt_filter_create_main_conf, /* create main configuration */
> @@ -865,6 +877,46 @@
> }
>
>
> +static char *
> +ngx_http_xslt_stylesheet_param(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
> +{
> + ngx_http_xslt_filter_loc_conf_t *xlcf = conf;
> +
> + ngx_http_complex_value_t *param;
> + ngx_http_compile_complex_value_t ccv;
> + ngx_str_t *value;
> +
> + value = cf->args->elts;
> +
> + /* alloc array */
> + if (xlcf->params.elts == NULL
> + && ngx_array_init(&xlcf->params, cf->pool, 1, sizeof(ngx_http_complex_value_t))
> + != NGX_OK)
> + {
> + return NGX_CONF_ERROR;
> + }

No lines longer than 80 chars, please. And alignment isn't
styl-complaint either. In this particular case, please just use
another nested if, much like in ngx_http_xslt_stylesheet():

if (xlcf->sheets.elts == NULL) {
if (ngx_array_init(&xlcf->sheets, cf->pool, 1,
sizeof(ngx_http_xslt_sheet_t))
!= NGX_OK)
{
return NGX_CONF_ERROR;
}
}

> +
> + param = ngx_array_push(&xlcf->params);
> + if (param == NULL) {
> + return NGX_CONF_ERROR;
> + }
> +
> + ngx_memzero(&ccv, sizeof(ngx_http_compile_complex_value_t));
> +
> + ccv.cf = cf;
> + ccv.value = &value[1];
> + ccv.complex_value = param;
> + ccv.zero = 1;
> +
> + if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
> + return NGX_CONF_ERROR;
> + }
> +
> + return NGX_CONF_OK;
> +}
> +
> +
> +

2 blank lines between functions, please. 3 blank lines before
ngx_http_xslt_stylesheet() is a bug, not a rule.

Another question to consider: there was at least one request to
simplify passing XPath expressions in params by allowing ':', see
here:

http://mailman.nginx.org/pipermail/nginx-devel/2010-July/000414.html
http://mailman.nginx.org/pipermail/nginx-devel/2010-September/000486.html

With separate directive we may want to allow this. Not sure
though, probably it would be better to preserve compatibility with
a way how parameters in xslt_stylesheet directive are handled.

> static void
> ngx_http_xslt_cleanup_dtd(void *data)
> {
> @@ -931,6 +983,37 @@
>
>
> static char *
> +ngx_http_xslt_filter_merge_params(const ngx_array_t *src, ngx_array_t *dest)
> +{
> + ngx_uint_t i;
> +
> + /* copy params */
> + if (src->nelts && !dest->nelts) {
> + dest->elts = src->elts;
> + dest->nelts = src->nelts;
> + }
> + /* join params */
> + else if (src->nelts && dest->nelts) {
> + ngx_http_complex_value_t *params;
> +
> + params = src->elts;
> + for (i = 0; i < src->nelts; i++) {
> + ngx_http_complex_value_t *new;
> +
> + new = ngx_array_push(dest);
> + if (new == NULL) {
> + return NGX_CONF_ERROR;
> + }
> +
> + *new = params[i]; /* copy */
> + }
> + }
> +
> + return NGX_CONF_OK;
> +}
> +
> +
> +static char *
> ngx_http_xslt_filter_merge_conf(ngx_conf_t *cf, void *parent, void *child)
> {
> ngx_http_xslt_filter_loc_conf_t *prev = parent;
> @@ -940,9 +1023,26 @@
> conf->dtd = prev->dtd;
> }
>
> + if (ngx_http_xslt_filter_merge_params(&prev->params, &conf->params)
> + != NGX_CONF_OK) {
> + return NGX_CONF_ERROR;
> + }

As I already said in the previous review, params should follow the
same logic as other array-type directives (access_log,
proxy_set_header, fastcgi_param, error_page, ...). I.e. if you
define any directive on a particular level, previous level data
should not be inherited:

location / {
xslt_param foo=1;

location /bar/ {
xslt_param bar=1;
}
}

In location /bar/ only "bar=1" parameter should be present.

Please also note that with your code it's not possible to clear
inherited params at all.

> +
> if (conf->sheets.nelts == 0) {
> conf->sheets = prev->sheets;
> }
> + else {
> + ngx_http_xslt_sheet_t *sheet;
> + ngx_uint_t i;
> +
> + sheet = conf->sheets.elts;
> + for (i = 0; i < conf->sheets.nelts; i++) {
> + if (ngx_http_xslt_filter_merge_params(&conf->params, &sheet[i].params)
> + != NGX_CONF_OK) {
> + return NGX_CONF_ERROR;
> + }
> + }
> + }

This is wrong. Consider configuration like this:

location / {
xslt_stylesheet root.xslt;

location /foo/ {
xslt_params foo=bar;
}
}

With the above merge logic in location /foo/ the root.xslt
stylesheet will be used without parameters, while "foo=bar" is
clearly expected.

I suggest to use simple array-type inheritance for params, like

if (conf->params == NULL) {
conf->params = prev->params;
}

and apply them separately to the stylesheet at runtime. This will
resolve both problems outlined above.

>
> if (ngx_http_merge_types(cf, &conf->types_keys, &conf->types,
> &prev->types_keys, &prev->types,
> @@ -957,7 +1057,7 @@
>
>
> static ngx_int_t
> -ngx_http_xslt_filter_init(ngx_conf_t *cf)
> +ngx_http_xslt_filter_preinit(ngx_conf_t *cf)
> {
> xmlInitParser();
>
> @@ -965,6 +1065,13 @@
> exsltRegisterAll();
> #endif
>
> + return NGX_OK;
> +}
> +
> +
> +static ngx_int_t
> +ngx_http_xslt_filter_init(ngx_conf_t *cf)
> +{
> ngx_http_next_header_filter = ngx_http_top_header_filter;
> ngx_http_top_header_filter = ngx_http_xslt_header_filter;
>

See above, this is unrelated change.

Maxim Dounin

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel