[PATCH] Limit conn: Added "off" parameter to the 'limit_conn' directive

[Maxim Dounin]

Hello!

On Wed, Feb 10, 2016 at 12:09:09AM +0600, Pavel V. Rochnyack wrote:

> # HG changeset patch
> # User Pavel V. Rochnyack <pavel2000 at ngs.ru>
> # Date 1454835814 -21600
> # Node ID b4748ebdd06ba79aa27e0c54fc1d627d13966bed
> # Parent  3577c021f21eb4de6d09c1d624ba77ee9ee1eb6d
> Limit conn: Added "off" parameter to the 'limit_conn' directive.

Please do not capitalize "Added".  Please do not use different 
quotes.

Something like

Limit conn: added "limit_conn off".

should be good enough.

> 
> Added support for inherited "limit_conn" directives disable.

This probably can be safely omitted.  Alternatively, please 
rewrite to something more readable, e.g.:

The "limit_conn" directive with the "off" parameter allows to
disable inheritance of limits from the previous level.

> 
> diff -r 3577c021f21e -r b4748ebdd06b src/http/modules/ngx_http_limit_conn_module.c
> --- a/src/http/modules/ngx_http_limit_conn_module.c	Fri Feb 05 21:48:25 2016 +0300
> +++ b/src/http/modules/ngx_http_limit_conn_module.c	Sun Feb 07 15:03:34 2016 +0600
> @@ -40,6 +40,7 @@ typedef struct {
>      ngx_array_t                limits;
>      ngx_uint_t                 log_level;
>      ngx_uint_t                 status_code;
> +    ngx_uint_t                 off;            /* unsigned  off:1 */
>  } ngx_http_limit_conn_conf_t;
>  
>  
> @@ -82,7 +83,7 @@ static ngx_command_t  ngx_http_limit_con
>        NULL },
>  
>      { ngx_string("limit_conn"),
> -      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE12,
>        ngx_http_limit_conn,
>        NGX_HTTP_LOC_CONF_OFFSET,
>        0,
> @@ -160,6 +161,10 @@ ngx_http_limit_conn_handler(ngx_http_req
>      lccf = ngx_http_get_module_loc_conf(r, ngx_http_limit_conn_module);
>      limits = lccf->limits.elts;
>  
> +    if (lccf->off) {
> +        return NGX_DECLINED;
> +    }
> +

This check should not be needed once configuration parsing 
properly implemented, as lccf->limits.nelts below will be 0 and 
the for() loop will do nothing.

>      for (i = 0; i < lccf->limits.nelts; i++) {
>          ctx = limits[i].shm_zone->data;
>  
> @@ -466,6 +471,7 @@ ngx_http_limit_conn_create_conf(ngx_conf
>  
>      conf->log_level = NGX_CONF_UNSET_UINT;
>      conf->status_code = NGX_CONF_UNSET_UINT;
> +    conf->off = NGX_CONF_UNSET_UINT;
>  
>      return conf;
>  }
> @@ -477,7 +483,9 @@ ngx_http_limit_conn_merge_conf(ngx_conf_
>      ngx_http_limit_conn_conf_t *prev = parent;
>      ngx_http_limit_conn_conf_t *conf = child;
>  
> -    if (conf->limits.elts == NULL) {
> +    ngx_conf_merge_uint_value(conf->off, prev->off, 0);
> +
> +    if (conf->off == 0 && conf->limits.elts == NULL) {
>          conf->limits = prev->limits;
>      }
>  
> @@ -603,6 +611,35 @@ ngx_http_limit_conn(ngx_conf_t *cf, ngx_
>  
>      value = cf->args->elts;
>  
> +    ngx_conf_merge_uint_value(lccf->off, lccf->off, 0);

Merging the value with itself looks like a dirty hack.  Correct 
solution would be to set conf->off to 0 in create_conf, and don't 
try to merge it neither here nor in merge_conf.

> +
> +    if (ngx_strcmp(value[1].data, "off") == 0) {
> +        if (cf->args->nelts != 2) {
> +            return "has invalid number of arguments";
> +        }
> +
> +        if (lccf->off || lccf->limits.elts) {
> +            ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> +                               "\"limit_conn off\" cannot be used with other "
> +                               "\"limit_conn\" directives on the same level");
> +            return NGX_CONF_ERROR;
> +        }
> +
> +        lccf->off = 1;
> +        return NGX_CONF_OK;
> +    }
> +
> +    if (cf->args->nelts != 3) {
> +        return "has invalid number of arguments";
> +    }
> +
> +    if (lccf->off) {
> +        ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> +                           "\"limit_conn off\" cannot be used with other "
> +                           "\"limit_conn\" directives on the same level");
> +        return NGX_CONF_ERROR;
> +    }
> +

This looks overcomplicated and far from what nginx normally prints 
on similar errors at the same time.  It should be possible to do 
this better.

>      shm_zone = ngx_shared_memory_add(cf, &value[1], 0,
>                                       &ngx_http_limit_conn_module);
>      if (shm_zone == NULL) {

-- 
Maxim Dounin
http://nginx.org/

p.s.  Not --reply-to, but --in-reply-to.