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