Hi,
  ok

On Fri, Feb 10, 2012 at 12:12 AM, Maxim Dounin <mdounin@mdounin.ru> wrote:
Hello!

On Sat, Feb 04, 2012 at 05:47:10PM +0100, SamB wrote:

> 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 ;)

Overall looks good, thank you for your work (and sorry for late
reply).  See below for some more mostly minor comments.

I think we want to commit this version for now, not the one with
xslt_string_param, as the one with xslt_string_param scares me a
bit due to use of internal xslt structures.  See reply there for
details.

And could you please provide your name for CHANGES?

 

[...]

> 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-02-04 17:24:38.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 */

Using additional type with both name and value combined should be
better.

And we probably want to use pointer here, i.e.

   ngx_array_t         *params;

to simplify inheritance, see below.

>  } ngx_http_xslt_filter_loc_conf_t;
>
>
> @@ -75,7 +76,9 @@
>  static ngx_buf_t *ngx_http_xslt_apply_stylesheet(ngx_http_request_t *r,
>      ngx_http_xslt_filter_ctx_t *ctx);
>  static ngx_int_t ngx_http_xslt_params(ngx_http_request_t *r,
> -    ngx_http_xslt_filter_ctx_t *ctx, ngx_array_t *params);
> +    ngx_http_xslt_filter_ctx_t *ctx,
> +    const ngx_http_xslt_filter_loc_conf_t *conf,
> +    ngx_array_t *params);

We don't generally use "const".  If we will, it should be used
consistently over code - marking just one of the arguments as
"const" is misleading.  Please leave it for now.

>  static u_char *ngx_http_xslt_content_type(xsltStylesheetPtr s);
>  static u_char *ngx_http_xslt_encoding(xsltStylesheetPtr s);
>  static void ngx_http_xslt_cleanup(void *data);
> @@ -84,6 +87,8 @@
>      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);

In should be named ngx_http_xslt_param(), there is no need for
"stylesheet" here.

>  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);
> @@ -116,6 +121,13 @@
>        0,
>        NULL },
>
> +    { ngx_string("xslt_param"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,
> +      ngx_http_xslt_stylesheet_param,
> +      NGX_HTTP_LOC_CONF_OFFSET,
> +      0,
> +      NULL },
> +
>      { ngx_string("xslt_types"),
>        NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_1MORE,
>        ngx_http_types_slot,
> @@ -468,7 +480,7 @@
>
>      for (i = 0; i < conf->sheets.nelts; i++) {
>
> -        if (ngx_http_xslt_params(r, ctx, &sheet[i].params) != NGX_OK) {
> +        if (ngx_http_xslt_params(r, ctx, conf, &sheet[i].params) != NGX_OK) {

Passing conf isn't really needed as it may be obtained within
ngx_http_xslt_params() function from request.

>              xmlFreeDoc(doc);
>              return NULL;
>          }
> @@ -564,7 +576,7 @@
>
>  static ngx_int_t
>  ngx_http_xslt_params(ngx_http_request_t *r, ngx_http_xslt_filter_ctx_t *ctx,
> -    ngx_array_t *params)
> +    const ngx_http_xslt_filter_loc_conf_t *conf, ngx_array_t *params)
>  {
>      u_char                    *p, *last, *value, *dst, *src, **s;
>      size_t                     len;
> @@ -572,6 +584,47 @@
>      ngx_str_t                  string;
>      ngx_http_complex_value_t  *param;
>
> +    /* location params */
> +    param = conf->params.elts;

Style nitpicking:
Blank line after comment, please.

> +
> +    for (i = 0; i < conf->params.nelts; i++) {
> +
> +        if (ngx_http_complex_value(r, &param[i], &string) != NGX_OK) {
> +            return NGX_ERROR;
> +        }
> +
> +        value = string.data;
> +        len = string.len;
> +
> +        if ((i & 1) == 0) {
> +            ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> +                           "xslt filter param name: \"%s\"", string.data);
> +        } else {

Style nitpicking:
Blank line before "} else {", please.

Though this is probably irrelevant, as I suggested to change the
code to use custom type instead, and this if() will go away.

> +            ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> +                           "xslt filter param value: \"%s\"", string.data);
> +
> +            dst = value;
> +            src = value;
> +
> +            ngx_unescape_uri(&dst, &src, len, 0);
> +
> +            *dst = '\0';
> +
> +            ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
> +                           "xslt filter param unescaped: \"%s\"", value);
> +        }
> +
> +        p = string.data;
> +
> +        s = ngx_array_push(&ctx->params);
> +        if (s == NULL) {
> +            return NGX_ERROR;
> +        }
> +
> +        *s = value;
> +    }
> +
> +    /* per-stylesheet params */
>      param = params->elts;
>
>      for (i = 0; i < params->nelts; i++) {
> @@ -865,6 +918,51 @@
>  }
>
>
> +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_uint_t                        i;
> +    ngx_http_complex_value_t         *param;
> +    ngx_http_compile_complex_value_t  ccv;
> +    ngx_str_t                        *value;
> +
> +    value = cf->args->elts;
> +
> +    /* alloc array */

This comment looks redundant.

> +    if (xlcf->params.elts == NULL) {
> +        if (ngx_array_init(&xlcf->params, cf->pool, 2,
> +                           sizeof(ngx_http_complex_value_t))
> +            != NGX_OK)

Just a note: this will be ngx_array_create() instead with a
pointer, as suggested above.

Something like this:

   if (xlcf->params == NULL) {
       xlcf->params = ngx_array_create(cf->pool, 2,
                                       sizeof(ngx_http_xslt_param_t));
       if (xlcf->params == NULL) {
           return NGX_CONF_ERROR;
       }
   }

> +        {
> +            return NGX_CONF_ERROR;
> +        }
> +    }
> +
> +    for (i = 1; i <= 2; i++) {
> +
> +         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[i];
> +         ccv.complex_value = param;
> +         ccv.zero = 1;
> +
> +         if (ngx_http_compile_complex_value(&ccv) != NGX_OK) {
> +             return NGX_CONF_ERROR;
> +         }
> +    }

I don't think we want to support variables within param name.  It
would be better to leave it as static string.

> +
> +    return NGX_CONF_OK;
> +}
> +
> +
>  static void
>  ngx_http_xslt_cleanup_dtd(void *data)
>  {
> @@ -944,6 +1042,10 @@
>          conf->sheets = prev->sheets;
>      }
>
> +    if (conf->params.elts == 0) {
> +        conf->params = prev->params;
> +    }

This will be

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

And please also add

   conf->params = NULL;

into comment in ngx_http_xslt_filter_create_conf() to clarify how
it's initialized.

> +
>      if (ngx_http_merge_types(cf, &conf->types_keys, &conf->types,
>                               &prev->types_keys, &prev->types,
>                               ngx_http_xslt_default_types)

Maxim Dounin

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