Hey guys,

thanks for your work, Vladimir! To me, both the solution using getters
in the log module from Vladimir as well as just re-implementing the
methods in ngx_http_variables.c seem to have pros and cons. Using the
log module as a dependency from core might be a problem when compiling
without the log module (is that even possible?). However, duplicate
code for log/ngx_http variables also don't seem to be the ideal
solution.

Anyway, I am not very familiar with your conventions and what is rated
higher in nginx development, DRY code or core/module layering models.
I've attached my patch again; it does what you suggested, Maxim, in a
hardly intrusive way.

Looking forward to see one of the solutions merged.

Regards
Benjamin

2012/10/1 Vladimir Shebordaev <vshebordaev@mail.ru>:
> 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
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel