httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Rainer Jung <rainer.j...@kippdata.de>
Subject Re: Per-module / per-dir loglevel configuration version 4
Date Thu, 03 Jun 2010 21:49:34 GMT
On 02.06.2010 22:42, Stefan Fritsch wrote:
> I have updated my patch to the latest trunk and to use the variant
> where the module for logging is selected with APLOG_USE_MODULE and
> AP_DECLARE_MODULE macros:
...

> Multi-file modules have to use APLOG_USE_MODULE in the other
> files.
>
> The patch is at
>
> 	http://people.apache.org/~sf/per-module-loglevel-v4/ ,
>
> both as one file and as a series split into more or less logical
> chunks. Comments are very welcome. Also, it would be nice if
> someone could try it with a different compiler than gcc.

Some comments based on visual inspection. Next step (building/testing) 
is on my ToDo list. No major findings, only minor stuff.

0002-Introduce-log-levels-trace1-.-trace8.patch
-----------------------------------------------

Minor: Use of LOG_PRIMASK and APLOG_LEVELMASK: LOG_PRIMASK usually comes 
from sys/syslog.h and we set it to "7" if it is not defined. Previously 
both masks were the same. Since this is no longer the case, I wouldn't

-#define	APLOG_LEVELMASK	7	/* mask off the level value */
+#define APLOG_LEVELMASK ((LOG_PRIMASK << 1) + 1)  /* mask off the level 
value */

but instead keep our mask at a fixed value:

+#define APLOG_LEVELMASK 15  /* mask off the level value */

and then when using syslog

-        syslog(level_and_mask, "%s", errstr);
+        syslog(level_and_mask < APLOG_DEBUG ? level_and_mask : APLOG_DEBUG,
+               "%s", errstr);

instead

+        syslog(level_and_mask < LOG_PRIMASK ? level_and_mask : APLOG_DEBUG,
+               "%s", errstr);


0005-introduce-per-module-log-levels.patch
------------------------------------------

ap_get_module_loglevel() and ap_set_module_loglevel() get the module 
index as an argument, but the doc says "get at *their own* 
module-specific loglevel". It seems you can get/set the levels for all 
modules, not only your own. But well... If you change the wording, 
there's an anlogous comment in 
0009-add-loglevel-to-request_rec-and-conn_rec.patch.

Identation in the line: "Setting LogLevel for all modules to %s", arg);
(the line is also contained in 
0010-Introcduce-per-dir-loglevel-configuration.patch, but unchanged there)

Twice outdated code comments in log_error_core: "* above the server log 
level".


0013-Adjust-loglevels-to-log-less-at-levels-INFO-and-DEBU.patch
---------------------------------------------------------------

Indentation: "proxy: HTTP: canonicalising URL %s", url);
Indentation: loglevel = APLOG_INFO;

Indentation in the "new if" block in server/protocol.c

Indentation in modules/ssl/ssl_engine_kernel.c whenever ap_log_error is 
replaced by ap_log_cerror (argument indentation)


0017-associate-CONNECT-errors-with-the-request.patch
----------------------------------------------------

Indentation in modules/proxy/mod_proxy.h whenever ap_log_error is 
replaced by ap_log_rerror (argument indentation)


Additional remarks
------------------

We need to fix and add some docs

   - removed directives for mod_ssl, mod_rewrite and mod_logio
   - new syntax for LogLevel
   - merging and inheritance of LogLevels between servers and modules

Later possible improvement: should we add the module_name to the log 
line? Since you already know the index, associating the name would be 
easy. The question is, whether the info is important enough to add to 
the log line.

Regards,

Rainer

Mime
View raw message