[PATCH] Limit req, Limit conn: The "off" parameter of the 'limit_conn' and 'limit_req' directives

[Pavel V.]

Hi, Maxim.

Thanks for reviewing my patch.

> See no reasons why limits from previous level should be merged if
> "off" is used on the current level.  It's just unneeded work, 
> which also add complexity in other places (like a need to check 
> off at runtime, and merge off values from previous levels).

> Better approach would be to don't inherit limits if "off" is set 
> on the current level.

Ok, this part will be reworked.

>> @@ -603,6 +610,13 @@
>>  
>>      value = cf->args->elts;
>>  
>> +    lccf->off = 0;
>> +
>> +    if (ngx_strcmp(value[1].data, "off") == 0 && cf->args->nelts == 2) {
>> +        lccf->off = 1;
>> +        return NGX_CONF_OK;
>> +    }
>> +
>>      shm_zone = ngx_shared_memory_add(cf, &value[1], 0,
>>                                       &ngx_http_limit_conn_module);
>>      if (shm_zone == NULL) {


> This variant of the code doesn't check "off" if additional 
> arguments are used.  Limit req's version however does (and 
> complains if number of arguments is incorrect).  Reasons for the 
> difference are not clear.

These directives has different syntax at the moment. Directive 'limit_conn'
currently allows exactly two arguments and someone can configure Nginx to use
zone 'off' by a line

   ...
   limit_conn off 42;
   ...

The 'limit_req' directive expects prefix 'zone=' and there is no way to use
keyword 'off' as a standalone argument.

So, the reason for the difference is backward compatibility with existing
configurations. Should we care about this or not?

> With this code something like "limit_conn foo" will try to access
> value[2] unconditionally, potentially resulting in segmentation 
> fault during configuration parsing.

I has not considered such variant. Thank you for catching this.

> This code silently allows "limit_conn off" to be used multiple 
> times, as well as with other limit_conn directives.  This leads to 
> a situation where:

>    limit_conn off;
>    limit_conn foo 10;

> will not use "limit_conn foo 10" configured without any 
> indication, which looks incorrect and confusing.  It should either 
> complain, or use limits configured.

I used http_log_module as a guide for such behaviour. It has directive
"access_log" with similar syntax "access_log off;" and it does not complains
when other directives specified with 'access_log off'. Why this should be
handled in the other way here?

-- 
Regards,
 Pavel                          mailto:pavel2000 at ngs.ru