Welcome! Log In Create A New Profile

Advanced

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

Maxim Dounin
February 09, 2016 11:18PM
Hello!

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

> # HG changeset patch
> # User Pavel V. Rochnyack <pavel2000@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.

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

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

Pavel V. Rochnyack 485 February 09, 2016 01:10PM

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

Maxim Dounin 242 February 09, 2016 11:18PM

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

Pavel V. 217 February 10, 2016 12:32AM

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

Maxim Dounin 245 February 10, 2016 08:54AM

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

Valentin V. Bartenev 193 February 10, 2016 08:38AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 201
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 500 on July 15, 2024
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready