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