httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: svn commit: r1203634 - in /httpd/httpd/trunk: CHANGES include/ap_mmn.h include/http_log.h server/log.c
Date Sun, 20 Nov 2011 20:09:01 GMT
On Sun, Nov 20, 2011 at 3:41 AM, Stefan Fritsch <sf@sfritsch.de> wrote:
> On Fri, 18 Nov 2011, trawick@apache.org wrote:
>
>> Author: trawick
>> Date: Fri Nov 18 13:10:06 2011
>> New Revision: 1203634
>>
>> URL: http://svn.apache.org/viewvc?rev=1203634&view=rev
>> Log:
>> add conn_rec to error log hook
>
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/log.c?rev=1203634&r1=1203633&r2=1203634&view=diff
>>
>> ==============================================================================
>> --- httpd/httpd/trunk/server/log.c (original)
>> +++ httpd/httpd/trunk/server/log.c Fri Nov 18 13:10:06 2011
>> @@ -1264,8 +1264,8 @@ static void log_error_core(const char *f
>>        if (!log_format) {
>
> Not your change, but since you seem to use the error_log hook: Is this the
> correct behavior? It seems the hook is only called if there is no custom log
> format set. I think it should actually be "if (done)", i.e. call it only for
> the real message, not for the per-req/per-conn messages.

wow, I didn't notice any of that!  (I actually have the no-conn_rec
problem with code that runs only with 2.2 at present)

call it only for the real message; if the module needs the per-request
or per-connection information for some reason, it is already available
from c or r IIUC


>
>
>>            /* only pass the real error string to the hook */
>>            errstr[errstr_end] = '\0';
>> -            ap_run_error_log(file, line, module_index, level, status, s,
>> r, pool,
>> -                             errstr + errstr_start);
>> +            ap_run_error_log(file, line, module_index, level, status, s,
>> c, r,
>> +                             pool, errstr + errstr_start);
>>        }
>
> Would it make sense to pass the ap_errorlog_info struct instead? It has
> contains most of the args and is extensible with only a minor MMN bump.

(shrug)

it might be less aggravating to existing callers to simply add the
conn_rec parm prior to 2.4.x GA vs. changing the entire interface (not
many existing callers, dunno if we are entertaining API changes prior
to GA anyway)

>
>
>>        *errstr = '\0';
>> @@ -1761,9 +1761,9 @@ AP_DECLARE(const char *) ap_parse_log_le
>> AP_IMPLEMENT_HOOK_VOID(error_log,
>>                       (const char *file, int line, int module_index, int
>> level,
>>                        apr_status_t status, const server_rec *s,
>> -                        const request_rec *r, apr_pool_t *pool,
>> -                        const char *errstr), (file, line, module_index,
>> level,
>> -                        status, s, r, pool, errstr))
>> +                        const conn_rec *c, const request_rec *r,
>> +                        apr_pool_t *pool, const char *errstr), (file,
>> line,
>> +                        module_index, level, status, s, c, r, pool,
>> errstr))
>>
>> AP_IMPLEMENT_HOOK_RUN_FIRST(int, generate_log_id,
>>                            (const conn_rec *c, const request_rec *r,
>>
>>
>>
>



-- 
Born in Roswell... married an alien...

Mime
View raw message