Should I send patch here? - xslt_enable "only enable xslt transform for certain user agent"

Maxim Dounin mdounin at mdounin.ru
Wed Jan 27 15:49:35 MSK 2010


Hello!

Just a note: it's probably better idea to post patches to
nginx-devel at .  Cc'd.

On Wed, Jan 27, 2010 at 06:04:16PM +0800, tOmasEn wrote:

> Maybe not many people using xslt as main web layout. but I do.
> 
> This patch is similar to gzip_disable.  xslt_enable only enable xslt
> transform for certain user agent.
> 
> e.g.
> xslt_enable "Googlebot"
> will only enable xslt transform output for google's crawler.

While I tend to think this is good feature (though not sure), I 
also think that it probably needs some better name.  Something 
more self-explaining.

Also it's probably better to make xslt controllable via variable 
instead.  This will handle you "disable via special header" case 
as well.

> And, I'm also planing some other improve on xslt module. Like: when xslt
> transform failed, output original xml file instead of 500 server error; Skip
> xslt transform if client send specified HTTP header like "No-XSLT", etc.
> Should I talk to somebody and discuss the architect?
> 
> -- 
> Tomasen
> http://twitter.com/ShooterPlayer
> http://t.sina.com.cn/Tomasen

> *** ../nginx-0.8.31/src/http/modules/ngx_http_xslt_filter_module.c	2010-01-27 17:23:37.000000000 +0800
> --- nginx-0.8.31/src/http/modules/ngx_http_xslt_filter_module.c	2009-11-30 21:15:10.000000000 +0800
> ***************
> *** 48,54 ****

Please use unified diff patches, and not reversed ones.

>       ngx_array_t          sheets;       /* ngx_http_xslt_sheet_t */
>       ngx_hash_t           types;
>       ngx_array_t         *types_keys;
> - 	ngx_array_t         *xslt_enable;            /* xslt_enable */

Comment is pointless and wrongly placed.  Just remove it.

>   } ngx_http_xslt_filter_loc_conf_t;
>   
>   
> --- 48,53 ----
> ***************
> *** 135,143 ****
>       void *child);
>   static ngx_int_t ngx_http_xslt_filter_init(ngx_conf_t *cf);
>   static void ngx_http_xslt_filter_exit(ngx_cycle_t *cycle);
> ! 
> ! static char *ngx_http_xslt_enable(ngx_conf_t *cf, ngx_command_t *cmd,
> ! 								   void *conf);

1. Please follow nginx style.

2. Please move this to other config parsing functions.

>   
>   ngx_str_t  ngx_http_xslt_default_types[] = {
>       ngx_string("text/xml"),
> --- 134,140 ----
>       void *child);
>   static ngx_int_t ngx_http_xslt_filter_init(ngx_conf_t *cf);
>   static void ngx_http_xslt_filter_exit(ngx_cycle_t *cycle);
> ! 
>   
>   ngx_str_t  ngx_http_xslt_default_types[] = {
>       ngx_string("text/xml"),
> ***************
> *** 168,183 ****
>         offsetof(ngx_http_xslt_filter_loc_conf_t, types_keys),
>         &ngx_http_xslt_default_types[0] },
>   
> ! 	{ ngx_string("xslt_enable"),
> ! 	  NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_1MORE,
> ! 	  ngx_http_xslt_enable,
> ! 	  NGX_HTTP_LOC_CONF_OFFSET,
> ! 	  offsetof(ngx_http_xslt_filter_loc_conf_t, xslt_enable),
> ! 	  NULL },
> !   
> !     ngx_null_command
>   };
>   
>   static ngx_http_module_t  ngx_http_xslt_filter_module_ctx = {
>       NULL,                                  /* preconfiguration */
>       ngx_http_xslt_filter_init,             /* postconfiguration */
> --- 165,174 ----
>         offsetof(ngx_http_xslt_filter_loc_conf_t, types_keys),
>         &ngx_http_xslt_default_types[0] },
>   
> !       ngx_null_command
>   };
>   
> + 

Unrelated (and wrong) whitespace change here.

>   static ngx_http_module_t  ngx_http_xslt_filter_module_ctx = {
>       NULL,                                  /* preconfiguration */
>       ngx_http_xslt_filter_init,             /* postconfiguration */
> ***************
> *** 212,289 ****
>   static ngx_http_output_header_filter_pt  ngx_http_next_header_filter;
>   static ngx_http_output_body_filter_pt    ngx_http_next_body_filter;
>   
> - 
> - static char *
> - ngx_http_xslt_enable(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
> - {

Please move config parsing function to other config parsing 
functions.  

> - 	ngx_http_xslt_filter_loc_conf_t  *clcf = conf;
> - 
> - #if (NGX_PCRE)
> - 	ngx_str_t            *value;
> - 	ngx_uint_t            i;
> - 	ngx_regex_elt_t      *re;
> - 	ngx_regex_compile_t   rc;
> - 	u_char                errstr[NGX_MAX_CONF_ERRSTR];
> - 
> - 	if (clcf->xslt_enable == NULL) {
> - 		clcf->xslt_enable = ngx_array_create(cf->pool, 2,
> - 			sizeof(ngx_regex_elt_t));
> - 		if (clcf->xslt_enable == NULL) {
> - 			return NGX_CONF_ERROR;
> - 		}

No tabs, please.

> - 	}else{
> - 		//xslt_enable_is_not_required
> - 		return NGX_CONF_OK;

1. No C++ comments, please.

2. You should either return error here (something like return "is 
duplicate") or append to list as gzip_disable do.  Blindly 
ignoring configuration directive isn't what nginx generally do.

> - 	}
> - 
> - 	value = cf->args->elts;
> - 
> - 	ngx_memzero(&rc, sizeof(ngx_regex_compile_t));
> - 
> - 	rc.pool = cf->pool;
> - 	rc.err.len = NGX_MAX_CONF_ERRSTR;
> - 	rc.err.data = errstr;
> - 
> - 	for (i = 1; i < cf->args->nelts; i++) {
> - 
> - 		re = ngx_array_push(clcf->xslt_enable);
> - 		if (re == NULL) {
> - 			return NGX_CONF_ERROR;
> - 		}
> - 
> - 		rc.pattern = value[1];
> - 		rc.options = NGX_REGEX_CASELESS;
> - 
> - 		if (ngx_regex_compile(&rc) != NGX_OK) {
> - 			ngx_conf_log_error(NGX_LOG_EMERG, cf, 0, "%V", &rc.err);
> - 			return NGX_CONF_ERROR;
> - 		}
> - 
> - 		re->regex = rc.regex;
> - 		re->name = value[i].data;
> - 
> - 	}
> - 
> - 	return NGX_CONF_OK;
> - 
> - #else
> - 	ngx_str_t  *value;
> - 
> - 	value = cf->args->elts;

Any reason to use value here?  Looks like cut-n-paste leftover 
from gzip_disable.

> - 
> - 	ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> - 		"without PCRE library \"xslt_enable\" won't work ");
> - 
> - 	return NGX_CONF_ERROR;
> - #endif
> - }
>   
>   static ngx_int_t
>   ngx_http_xslt_header_filter(ngx_http_request_t *r)
>   {
>       ngx_http_xslt_filter_ctx_t       *ctx;
>       ngx_http_xslt_filter_loc_conf_t  *conf;
> ! 	
>       ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
>                      "xslt filter header");
>   
> --- 203,215 ----
>   static ngx_http_output_header_filter_pt  ngx_http_next_header_filter;
>   static ngx_http_output_body_filter_pt    ngx_http_next_body_filter;
>   
>   
>   static ngx_int_t
>   ngx_http_xslt_header_filter(ngx_http_request_t *r)
>   {
>       ngx_http_xslt_filter_ctx_t       *ctx;
>       ngx_http_xslt_filter_loc_conf_t  *conf;
> ! 
>       ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,
>                      "xslt filter header");
>   
> ***************
> *** 299,318 ****
>           return ngx_http_next_header_filter(r);
>       }
>   
> - #if (NGX_PCRE)
> - 	
> - 	if (conf->xslt_enable && r->headers_in.user_agent) {
> - 
> - 		if (ngx_regex_exec_array(conf->xslt_enable,
> - 			&r->headers_in.user_agent->value,
> - 			r->connection->log)
> - 			== NGX_DECLINED)
> - 		{
> - 			return ngx_http_next_header_filter(r);
> - 		}
> - 	}
> - 
> - #endif     
>       ctx = ngx_http_get_module_ctx(r, ngx_http_xslt_filter_module);
>   
>       if (ctx) {
> --- 225,230 ----

Maxim Dounin



More information about the nginx-devel mailing list