[PATCH] export variables from the log module

[Benjamin Grössing]

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 at 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 at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: add_bytes_sent_variable.patch
Type: application/octet-stream
Size: 1664 bytes
Desc: not available
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20121001/35a139c5/attachment.obj>