Hi,<br> ok <br><br><div class="gmail_quote">On Fri, Feb 10, 2012 at 12:12 AM, Maxim Dounin <span dir="ltr"><<a href="mailto:mdounin@mdounin.ru">mdounin@mdounin.ru</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hello!<br>
<div class="im"><br>
On Sat, Feb 04, 2012 at 05:47:10PM +0100, SamB wrote:<br>
<br>
> Hello,<br>
><br>
> I've modified my patch, according to comments in<br>
> <a href="http://mailman.nginx.org/pipermail/nginx-devel/2012-January/001751.html" target="_blank">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<br>
> value separated (similarly to set directive),<br>
> therefore it's now also possible to pass ':' character as it had been<br>
> requested before.<br>
> I think it's better and easier to have it this way + there is no need to<br>
> do that ugly parsing in runtime ;)<br>
<br>
</div>Overall looks good, thank you for your work (and sorry for late<br>
reply). See below for some more mostly minor comments.<br>
<br>
I think we want to commit this version for now, not the one with<br>
xslt_string_param, as the one with xslt_string_param scares me a<br>
bit due to use of internal xslt structures. See reply there for<br>
details.<br>
<br>
And could you please provide your name for CHANGES?<br></blockquote><div><br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[...]<br>
<div class="im"><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>
</div>> +++ nginx-1.1.13-new/src/http/modules/ngx_http_xslt_filter_module.c 2012-02-04 17:24:38.000000000 +0100<br>
<div class="im">> @@ -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>
<br>
</div>Using additional type with both name and value combined should be<br>
better.<br>
<br>
And we probably want to use pointer here, i.e.<br>
<br>
ngx_array_t *params;<br>
<br>
to simplify inheritance, see below.<br>
<br>
> } ngx_http_xslt_filter_loc_conf_t;<br>
><br>
><br>
> @@ -75,7 +76,9 @@<br>
> static ngx_buf_t *ngx_http_xslt_apply_stylesheet(ngx_http_request_t *r,<br>
> ngx_http_xslt_filter_ctx_t *ctx);<br>
> static ngx_int_t ngx_http_xslt_params(ngx_http_request_t *r,<br>
> - ngx_http_xslt_filter_ctx_t *ctx, ngx_array_t *params);<br>
> + ngx_http_xslt_filter_ctx_t *ctx,<br>
> + const ngx_http_xslt_filter_loc_conf_t *conf,<br>
> + ngx_array_t *params);<br>
<br>
We don't generally use "const". If we will, it should be used<br>
consistently over code - marking just one of the arguments as<br>
"const" is misleading. Please leave it for now.<br>
<br>
> static u_char *ngx_http_xslt_content_type(xsltStylesheetPtr s);<br>
> static u_char *ngx_http_xslt_encoding(xsltStylesheetPtr s);<br>
> static void ngx_http_xslt_cleanup(void *data);<br>
> @@ -84,6 +87,8 @@<br>
<div class="im">> 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>
<br>
</div>In should be named ngx_http_xslt_param(), there is no need for<br>
"stylesheet" here.<br>
<div class="im"><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>
</div><div class="im">> @@ -116,6 +121,13 @@<br>
> 0,<br>
> NULL },<br>
><br>
</div>> + { ngx_string("xslt_param"),<br>
> + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,<br>
<div class="im">> + ngx_http_xslt_stylesheet_param,<br>
> + NGX_HTTP_LOC_CONF_OFFSET,<br>
> + 0,<br>
> + NULL },<br>
</div><div class="im">> +<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>
</div>> @@ -468,7 +480,7 @@<br>
<div class="im">><br>
> for (i = 0; i < conf->sheets.nelts; i++) {<br>
><br>
</div><div class="im">> - if (ngx_http_xslt_params(r, ctx, &sheet[i].params) != NGX_OK) {<br>
</div>> + if (ngx_http_xslt_params(r, ctx, conf, &sheet[i].params) != NGX_OK) {<br>
<br>
Passing conf isn't really needed as it may be obtained within<br>
ngx_http_xslt_params() function from request.<br>
<br>
> xmlFreeDoc(doc);<br>
> return NULL;<br>
> }<br>
> @@ -564,7 +576,7 @@<br>
><br>
> static ngx_int_t<br>
> ngx_http_xslt_params(ngx_http_request_t *r, ngx_http_xslt_filter_ctx_t *ctx,<br>
> - ngx_array_t *params)<br>
> + const ngx_http_xslt_filter_loc_conf_t *conf, ngx_array_t *params)<br>
> {<br>
> u_char *p, *last, *value, *dst, *src, **s;<br>
> size_t len;<br>
> @@ -572,6 +584,47 @@<br>
> ngx_str_t string;<br>
> ngx_http_complex_value_t *param;<br>
><br>
> + /* location params */<br>
> + param = conf->params.elts;<br>
<br>
Style nitpicking:<br>
Blank line after comment, please.<br>
<br>
> +<br>
> + for (i = 0; i < conf->params.nelts; i++) {<br>
> +<br>
> + if (ngx_http_complex_value(r, ¶m[i], &string) != NGX_OK) {<br>
> + return NGX_ERROR;<br>
> + }<br>
> +<br>
> + value = string.data;<br>
> + len = string.len;<br>
> +<br>
> + if ((i & 1) == 0) {<br>
> + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,<br>
> + "xslt filter param name: \"%s\"", string.data);<br>
> + } else {<br>
<br>
Style nitpicking:<br>
Blank line before "} else {", please.<br>
<br>
Though this is probably irrelevant, as I suggested to change the<br>
code to use custom type instead, and this if() will go away.<br>
<br>
> + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,<br>
> + "xslt filter param value: \"%s\"", string.data);<br>
> +<br>
> + dst = value;<br>
> + src = value;<br>
> +<br>
> + ngx_unescape_uri(&dst, &src, len, 0);<br>
> +<br>
> + *dst = '\0';<br>
> +<br>
> + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,<br>
> + "xslt filter param unescaped: \"%s\"", value);<br>
> + }<br>
> +<br>
> + p = string.data;<br>
> +<br>
> + s = ngx_array_push(&ctx->params);<br>
> + if (s == NULL) {<br>
> + return NGX_ERROR;<br>
> + }<br>
> +<br>
> + *s = value;<br>
> + }<br>
> +<br>
> + /* per-stylesheet params */<br>
> param = params->elts;<br>
><br>
> for (i = 0; i < params->nelts; i++) {<br>
> @@ -865,6 +918,51 @@<br>
<div class="im">> }<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>
</div>> + ngx_uint_t i;<br>
<div class="im">> + 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>
<br>
</div>This comment looks redundant.<br>
<br>
> + if (xlcf->params.elts == NULL) {<br>
> + if (ngx_array_init(&xlcf->params, cf->pool, 2,<br>
> + sizeof(ngx_http_complex_value_t))<br>
> + != NGX_OK)<br>
<br>
Just a note: this will be ngx_array_create() instead with a<br>
pointer, as suggested above.<br>
<br>
Something like this:<br>
<br>
if (xlcf->params == NULL) {<br>
xlcf->params = ngx_array_create(cf->pool, 2,<br>
sizeof(ngx_http_xslt_param_t));<br>
if (xlcf->params == NULL) {<br>
return NGX_CONF_ERROR;<br>
<div class="im"> }<br>
}<br>
<br>
> + {<br>
> + return NGX_CONF_ERROR;<br>
> + }<br>
</div>> + }<br>
> +<br>
> + for (i = 1; i <= 2; i++) {<br>
<div class="im">> +<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>
</div>> + ccv.value = &value[i];<br>
<div class="im">> + 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>
<br>
</div>I don't think we want to support variables within param name. It<br>
would be better to leave it as static string.<br>
<div class="im"><br>
> +<br>
> + return NGX_CONF_OK;<br>
> +}<br>
> +<br>
> +<br>
</div>> static void<br>
> ngx_http_xslt_cleanup_dtd(void *data)<br>
> {<br>
> @@ -944,6 +1042,10 @@<br>
<div class="im">> conf->sheets = prev->sheets;<br>
> }<br>
><br>
</div>> + if (conf->params.elts == 0) {<br>
> + conf->params = prev->params;<br>
> + }<br>
<br>
This will be<br>
<div class="im"><br>
if (conf->params == NULL) {<br>
conf->params = prev->params;<br>
}<br>
<br>
</div>And please also add<br>
<br>
conf->params = NULL;<br>
<br>
into comment in ngx_http_xslt_filter_create_conf() to clarify how<br>
it's initialized.<br>
<div class="im"><br>
> +<br>
> if (ngx_http_merge_types(cf, &conf->types_keys, &conf->types,<br>
> &prev->types_keys, &prev->types,<br>
</div>> ngx_http_xslt_default_types)<br>
<font color="#888888"><br>
Maxim Dounin<br>
</font><div><div></div><div class="h5"><br>
_______________________________________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org">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>
</div></div></blockquote></div><br>