Hi, Benjamin,

On 01.10.2012 03:20, Benjamin Grössing wrote:
> 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.

As for me, I've also suggested to export the variables that way
due to they are documented in the log module. Also, having
separate implementations will require keeping 'em consistent upon
change but this is hardly probable though.

> 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.
>

I meant that you would like to implement other variables from the
log module in the way you did it with $bytes_sent. Sure, except
for the $status variable. I think so Maxim did.

Regards,
Vladimir

> 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