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
>
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel