Hello,

   I've modified my patch, according to comments in http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001751.html.

   Directive has been renamed to xslt_param, taking 2 parameters - name and value separated (similarly to set directive),
   therefore it's now also possible to pass ':' character as it had been requested before.
   I think it's better and easier to have it this way + there is no need to do that ugly parsing in runtime ;)

Thanks
  Sam

On Tue, Jan 31, 2012 at 1:00 PM, <nginx-devel-request@nginx.org> wrote:

Message: 6
Date: Tue, 31 Jan 2012 05:52:23 +0400
From: Maxim Dounin <mdounin@mdounin.ru>
To: nginx-devel@nginx.org
Subject: Re: [PATCH] add xslt_stylesheet_param directive
Message-ID: <20120131015222.GH67687@mdounin.ru>
Content-Type: text/plain; charset=us-ascii

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

End of nginx-devel Digest, Vol 27, Issue 26
*******************************************