Hi,
  I'll fix my non-string patch, here are my explanations to your comments ;)

Sam

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

On Wed, Feb 08, 2012 at 01:55:47PM +0100, SamB wrote:

> Hi,
>
>    I've added xslt_string_param and slightly changed parameter passing and
> transformation
>    to make it work as expected.
>    As a side effect of this change, might be usage of
> xslt_param/xslt_string_param faster,
>    because params are directly inserted to transformation context and do
> not need to be
>    copied to name/value parameter array that is passed to
> xsltApplyStylesheet() as it's now
>    by parameters passed via xslt_stylesheet directive.
>
>    Old parameter passing using xslt_stylesheet has not been changed to
> prevent unexpected behavior,
>    maybe this can be modified later if this patch will prove to be stable...

This looks intresting, though may be it would be better to finish
and commit the xslt_param patch first (see another message for a
review).

Below is review of the xslt_param + xslt_string_param patch
specific bits.

[...]

> 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-08 13:34:43.000000000 +0100
> @@ -10,6 +10,7 @@
>
>  #include <libxml/parser.h>
>  #include <libxml/tree.h>
> +#include <libxslt/variables.h>
>  #include <libxslt/xslt.h>
>  #include <libxslt/xsltInternals.h>
>  #include <libxslt/transform.h>
> @@ -48,6 +49,8 @@
>      ngx_array_t          sheets;       /* ngx_http_xslt_sheet_t */
>      ngx_hash_t           types;
>      ngx_array_t         *types_keys;
> +    ngx_array_t          string_params; /* ngx_http_complex_value_t */
> +    ngx_array_t          params;       /* ngx_http_complex_value_t */

I would like to see single "params" array instead, with both
string params and xpath ones.

Ok, I will merge them and distinguish by flag.
 
[...]

> @@ -84,6 +90,13 @@
>      void *conf);
>  static char *ngx_http_xslt_stylesheet(ngx_conf_t *cf, ngx_command_t *cmd,
>      void *conf);
> +static char *ngx_http_xslt_add_param(ngx_conf_t *cf, ngx_command_t *cmd,
> +    ngx_array_t *params);
> +static char *ngx_http_xslt_stylesheet_param(ngx_conf_t *cf, ngx_command_t *cmd,
> +    void *conf);
> +static char *ngx_http_xslt_stylesheet_string_param(ngx_conf_t *cf,
> +    ngx_command_t *cmd,
> +    void *conf);

There is no need for 3 functions here.  Just one is enough.

In your current code, you may (and probably have to) use "offset"
field of the command structure to pass offset of array you want to
work with.  With single params array as suggested earlier - you
may use "post" field to distinguish string and non-string
versions.


Ok.
Regarding rename of ngx_http_xslt_stylesheet_param to shorter ngx_http_xslt_param,
I've had again problem with existing function with similar name ngx_http_xslt_params.
 
>  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 +129,20 @@
>        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_string_param"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,
> +      ngx_http_xslt_stylesheet_string_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,
> @@ -450,6 +477,7 @@
>      ngx_uint_t                        i;
>      xmlChar                          *buf;
>      xmlDocPtr                         doc, res;
> +    xsltTransformContextPtr           xctxt;

In libxslt this is usually called "ctxt", I think we should
follow.


There exists variable ctx already in this context, and I find them too similar therefore this way.

 
>      ngx_http_xslt_sheet_t            *sheet;
>      ngx_http_xslt_filter_loc_conf_t  *conf;
>
> @@ -468,14 +496,28 @@
>
>      for (i = 0; i < conf->sheets.nelts; i++) {
>
> -        if (ngx_http_xslt_params(r, ctx, &sheet[i].params) != NGX_OK) {
> +        xctxt = xsltNewTransformContext(sheet[i].stylesheet, doc);
> +        if (xctxt == NULL) {
>              xmlFreeDoc(doc);
>              return NULL;
>          }
>
> -        res = xsltApplyStylesheet(sheet[i].stylesheet, doc, ctx->params.elts);
> +        xctxt->initialContextDoc = doc;
> +        xctxt->initialContextNode = (xmlNodePtr) doc;

According to

http://xmlsoft.org/XSLT/html/libxslt-xsltInternals.html

this is internal to libxslt, and using it may not be a good idea.
Are you sure it's needed?

As far as I can see in sources, it's ceratainly *not* needed for
xsltQuoteOneUserParam().  Not sure about xsltEvalOneUserParam().


This is needed for xsltEvalOneUserParam() to work correctly, there is no
other way how to make it work (libxslt bug?), and I don't think its
risky to use it.
But, I can avoid use of xsltEvalOneUserParam() at all and pass params
in the old way.

 
> +
> +        if (ngx_http_xslt_params(r, ctx, conf, xctxt,
> +                                 &sheet[i].params) != NGX_OK) {
> +            xmlFreeDoc(doc);
> +            xsltFreeTransformContext(xctxt);

It may be a good idea to free this in reverse order, i.e.
transformation context first.

Ok.
 
> +            return NULL;
> +        }
> +
> +        res = xsltApplyStylesheetUser(sheet[i].stylesheet, doc,
> +                                      ctx->params.elts,
> +                                      NULL, NULL, xctxt);
>
>          xmlFreeDoc(doc);
> +        xsltFreeTransformContext(xctxt);

Same here.

>
>          if (res == NULL) {
>              ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
> @@ -564,14 +606,92 @@

[...]

Maxim Dounin

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