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