Hi,

On 30.09.2012 22:54, Maxim Dounin wrote:
> Hello!
>
> On Sun, Sep 30, 2012 at 07:55:49AM +0400, Vladimir Shebordaev wrote:
>
> [...]
>
>> +static ngx_int_t
>> +ngx_http_log_add_variables(ngx_conf_t *cf)
>> +{
>> + ngx_http_log_var_t *v;
>> + ngx_http_variable_t *var;
>> + ngx_http_core_main_conf_t *cmcf;
>> +
>> + ngx_int_t rc;
>> +
>> + cmcf = ngx_http_conf_get_module_main_conf(cf, ngx_http_core_module);
>> +
>> + var = NULL;
>> +
>> + for (v = ngx_http_log_vars; v->name.len; v++) {
>> +
>> + if (var == NULL) {
>> + var = ngx_pcalloc(cf->pool, sizeof(ngx_http_variable_t));
>> + if (var == NULL) {
>> + return NGX_ERROR;
>> + }
>> + }
>> +
>> + var->name = v->name;
>> +
>> + rc = ngx_hash_add_key(cmcf->variables_keys, &v->name, var,
>> + NGX_HASH_READONLY_KEY);
>> +
>> + if (rc == NGX_BUSY) {
>> + continue;
>> + }
>
> No, it's layering violation. It's not log modules business to
> hack cmcf->variables_keys. And, as a side note, it's very-very
> wrong to just continue on conflicting variable names.


Well, the only conflicting variable is "status", but this is only
a patch. I guess, emergency stop would look rather strange here,
since this is a static array, Actually, there are variables in
http/ngx_http_variables.c whose getters implemented in other
modules. Basically, I intentionally suggested to implement those
ones in the log module to preserve existing module dependencies.

> I've already pointed out correct aproach to this: log module
> not-really-variables should be reimplemented/duplicated as real
> variables in ngx_http_variables.c.
>

I think that is exactly what Benjamin would like to do.

By the way, passing zeroed vv->data field to v->get_handler() in
ngx_http_get_variable() would be of some interest in itself. The
handlers could rely on it to reuse the value's data when it is
feasible. I think it could be done just for the sake of
consistency. As of now, it is possible just for the indexed
variables whose value data is explicitly zeroed out in
ngx_http_init_request() upon request->variables array allocation.

Hope it helps.

Regards,
Vladimir

> [...]
>
> Maxim Dounin
>
>

_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel