Re: Per-module / per-dir loglevel configuration version 4
On Wed, Jun 02, 2010 at 10:42:44PM +0200, Stefan Fritsch wrote: The patch is at http://people.apache.org/~sf/per-module-loglevel-v4/ , This looks good to me. Kudos for taking on such a task. It's kind of hard to review the individual patches with fixes-on-fixes separated out, or the big patch which jumbles everything up. The use of an extra per-file static global pointer to the module index seems awkward at first but turns into a neat hack by the end of the story. I very much like the new trace levels, and junking all the module-specific logging stuff in mod_rewrite/ssl. Some very minor nitpicking: Can you rename struct logcfg to something a bit more namespace-safe, with an ap_ prefix at least? struct ap_log_config, ap_logconf? In ap_reset_module_loglevels(), is there any reason not to simplify: +for (i = 0; i total_modules + DYNAMIC_MODULE_LIMIT; i++) +l-module_levels[i] = val; to: memset(l-module_levels, val, total_modules + DYNAMIC_MODULE_LIMIT); ? That's all I have... thanks and nice work! Regards, Joe
RE: Per-module / per-dir loglevel configuration version 4
-Original Message- From: Joe Orton Sent: Freitag, 4. Juni 2010 13:29 To: dev@httpd.apache.org Subject: Re: Per-module / per-dir loglevel configuration version 4 On Wed, Jun 02, 2010 at 10:42:44PM +0200, Stefan Fritsch wrote: The patch is at http://people.apache.org/~sf/per-module-loglevel-v4/ , This looks good to me. Kudos for taking on such a task. It's kind of hard to review the individual patches with fixes-on-fixes separated out, or the big patch which jumbles everything up. The use of an extra per-file static global pointer to the module index seems awkward at first but turns into a neat hack by the end of the story. I very much like the new trace levels, and junking all the module-specific logging stuff in mod_rewrite/ssl. Some very minor nitpicking: Can you rename struct logcfg to something a bit more namespace-safe, with an ap_ prefix at least? struct ap_log_config, ap_logconf? In ap_reset_module_loglevels(), is there any reason not to simplify: +for (i = 0; i total_modules + DYNAMIC_MODULE_LIMIT; i++) +l-module_levels[i] = val; to: memset(l-module_levels, val, total_modules + DYNAMIC_MODULE_LIMIT); Hm, module_levels is int[] and memset works byte wise. So IMHO we would only touch the first total_modules + DYNAMIC_MODULE_LIMIT bytes of module_levels whereas module_levels is sizeof(int) * (total_modules + DYNAMIC_MODULE_LIMIT) large. Furthermore the values set will be IMHO wrong as each byte is filled with the same value and sizeof(int) bytes will be intepreted as an int later. Regards Rüdiger
Re: Per-module / per-dir loglevel configuration version 4
On Fri, Jun 04, 2010 at 01:40:42PM +0200, Plüm, Rüdiger, VF-Group wrote: memset(l-module_levels, val, total_modules + DYNAMIC_MODULE_LIMIT); Hm, module_levels is int[] and memset works byte wise. Doh. Sorry, yes, ignore me there.
Re: Per-module / per-dir loglevel configuration version 4
On Friday 04 June 2010, William A. Rowe Jr. wrote: This seems to be a very good solution, and the fact that their are no constructor-time calls to initialize this should avoid any platform quirks. My only question is; are we assured to have the same module_index reassigned on each config/reload phase? The reason I'm worried is that this code won't refresh the index, but there may be platforms that don't actually do a module unload/reload/reinit heap of aplog_module_index to NULL. Since we are only initializing a pointer into the module struct, it really does not matter if the module_index changes or not. If the module is not reloaded, the address of the module struct stays the same. If the module is reloaded, the pointer is reinitialized. A module just must not use a module_struct for logging which is in a different module/shared object. This could lead to segfaults if the depended upon module is reloaded (if the runtime linker does not prevent unloading in this case anyway). But only using your own module_index for logging does not sound like a limitation to me.
Re: Per-module / per-dir loglevel configuration version 4
On 6/4/2010 4:47 PM, Stefan Fritsch wrote: Since we are only initializing a pointer into the module struct, it really does not matter if the module_index changes or not. If the module is not reloaded, the address of the module struct stays the same. If the module is reloaded, the pointer is reinitialized. Then this all seems complete; +1 to commit, thanks again for all your diligence in making this improvement painless to module developers!
Re: Per-module / per-dir loglevel configuration version 4
On Wednesday 02 June 2010, 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:
APLOG_USE_MODULE(foo)
expands to
extern module AP_MODULE_DECLARE_DATA foo_module;
static int * const aplog_module_index =
(foo_module.module_index)
and
AP_DECLARE_MODULE(foo)
expands to
APLOG_USE_MODULE(foo);
module AP_MODULE_DECLARE_DATA foo_module
So, a single-file module only needs to do:
AP_DECLARE_MODULE(foo) =
{
STANDARD20_MODULE_STUFF,
...
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/ ,
I have added two additional patches (filenames 10*) to fix a segfault.
In addition, I now have this in http_log.h:
#define APLOG_NO_MODULE -1
static int * const aplog_module_index;
#define APLOG_MODULE_INDEX \
(aplog_module_index ? *aplog_module_index : APLOG_NO_MODULE)
This means, if some source file does not initialize aplog_module_index
with APLOG_USE_MODULE, logging will simply default to the global
loglevel. This works because static pointers are initialized to NULL
if no explicit initialization is given.
I think that's a nice solution. Modules from 2.2.x continue to work
without changes. But in order to benefit from per-module loglevel
configuration, they have to use the new macros.
Cheers,
Stefan
Re: Per-module / per-dir loglevel configuration version 4
On Thu, Jun 3, 2010 at 12:49 PM, Stefan Fritsch s...@sfritsch.de wrote:
On Wednesday 02 June 2010, 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:
APLOG_USE_MODULE(foo)
expands to
extern module AP_MODULE_DECLARE_DATA foo_module;
static int * const aplog_module_index =
(foo_module.module_index)
and
AP_DECLARE_MODULE(foo)
expands to
APLOG_USE_MODULE(foo);
module AP_MODULE_DECLARE_DATA foo_module
So, a single-file module only needs to do:
AP_DECLARE_MODULE(foo) =
{
STANDARD20_MODULE_STUFF,
...
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/http://people.apache.org/%7Esf/per-module-loglevel-v4/,
I have added two additional patches (filenames 10*) to fix a segfault.
In addition, I now have this in http_log.h:
#define APLOG_NO_MODULE -1
static int * const aplog_module_index;
#define APLOG_MODULE_INDEX \
(aplog_module_index ? *aplog_module_index : APLOG_NO_MODULE)
This means, if some source file does not initialize aplog_module_index
with APLOG_USE_MODULE, logging will simply default to the global
loglevel. This works because static pointers are initialized to NULL
if no explicit initialization is given.
I think that's a nice solution. Modules from 2.2.x continue to work
without changes. But in order to benefit from per-module loglevel
configuration, they have to use the new macros.
sounds very reasonable to me
Re: Per-module / per-dir loglevel configuration version 4
On 03.06.2010 20:09, Jeff Trawick wrote: On Thu, Jun 3, 2010 at 12:49 PM, Stefan Fritsch s...@sfritsch.de wrote: This means, if some source file does not initialize aplog_module_index with APLOG_USE_MODULE, logging will simply default to the global loglevel. This works because static pointers are initialized to NULL if no explicit initialization is given. I think that's a nice solution. Modules from 2.2.x continue to work without changes. But in order to benefit from per-module loglevel configuration, they have to use the new macros. sounds very reasonable to me +1 Regards Rüdiger
Re: Per-module / per-dir loglevel configuration version 4
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 -#defineAPLOG_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
Re: Per-module / per-dir loglevel configuration version 4
On 6/3/2010 4:49 PM, Rainer Jung wrote: Indentation in modules/proxy/mod_proxy.h whenever ap_log_error is replaced by ap_log_rerror (argument indentation) Can we start pre-committing some of these pieces which are simply bug fixes, e.g. correcting the loglevel scope? It will save on review of the 'big picture' patch to get the small stuff out of the way, and into svn. 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. The construct mod_foo: The foo module broke is just sooo common it would seem like a waste not to take advantage of the newfound intellegence in the error log, and reduce all this verbosity in the source code.
Re: Per-module / per-dir loglevel configuration version 4
On Thursday 03 June 2010, Rainer Jung wrote: 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. Thank you very much for the comments. They look very reasonable and I will integrate them in the next round of patches. Or should I create a branch in svn? But I think git is easier to use when tracking the changes in trunk. Additional remarks -- We need to fix and add some docs - removed directives for mod_ssl, mod_rewrite and mod_logio - new syntax for LogLevel I have started updating the docs today. Except for mod_ssl, these two points are done. I have put it as 2000_docs.patch into the directory above. - merging and inheritance of LogLevels between servers and modules This and the handling of per-dir, per-conn, and per-request loglevels is still missing. Also, there is infrastructure to use the loglevel from the conn_req, but no way to set it, yet. Maybe a LogLevelIfIP directive? 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. It would be very useful when tuning the loglevel, but by default it may be too much noise. There are quite a few items in the same category (PID/TID, microseconds, ...) so a ErrorLogFlags or ErrorLogFormat directive may be a good idea. But these changes are not going to change the API/ABI, so IMHO they are less urgent. Cheers, Stefan
Re: Per-module / per-dir loglevel configuration version 4
On Friday 04 June 2010, William A. Rowe Jr. wrote: On 6/3/2010 4:49 PM, Rainer Jung wrote: Indentation in modules/proxy/mod_proxy.h whenever ap_log_error is replaced by ap_log_rerror (argument indentation) Can we start pre-committing some of these pieces which are simply bug fixes, e.g. correcting the loglevel scope? It will save on review of the 'big picture' patch to get the small stuff out of the way, and into svn. Sure. I have commited 0003-Introduce-SSLLOG_MARK-in-preparation-to-redefine-APL.patch 0004-move-find-module-logic-into-separate-function.patch 0012-Remove-loglevel-entry-in-core_dir_config-which-has-b.patch 0017-associate-CONNECT-errors-with-the-request.patch which are either pure bug fixes or pretty trivial. I will create a new patch series without these soon, hopefully tomorrow. 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. The construct mod_foo: The foo module broke is just sooo common it would seem like a waste not to take advantage of the newfound intellegence in the error log, and reduce all this verbosity in the source code. Cheers, Stefan
Re: Per-module / per-dir loglevel configuration version 4
On Thu, Jun 3, 2010 at 7:27 PM, Stefan Fritsch s...@sfritsch.de wrote: which are either pure bug fixes or pretty trivial. I will create a new patch series without these soon, hopefully tomorrow. Appreciate your commitment to getting this in! -- Eric Covener cove...@gmail.com
Re: Per-module / per-dir loglevel configuration version 4
On Thu, Jun 3, 2010 at 7:51 PM, Eric Covener cove...@gmail.com wrote: On Thu, Jun 3, 2010 at 7:27 PM, Stefan Fritsch s...@sfritsch.de wrote: which are either pure bug fixes or pretty trivial. I will create a new patch series without these soon, hopefully tomorrow. Appreciate your commitment to getting this in! +1
Re: Per-module / per-dir loglevel configuration version 4
On 6/3/2010 7:10 PM, Jeff Trawick wrote: On Thu, Jun 3, 2010 at 7:51 PM, Eric Covener cove...@gmail.com mailto:cove...@gmail.com wrote: On Thu, Jun 3, 2010 at 7:27 PM, Stefan Fritsch s...@sfritsch.de mailto:s...@sfritsch.de wrote: which are either pure bug fixes or pretty trivial. I will create a new patch series without these soon, hopefully tomorrow. Appreciate your commitment to getting this in! +1 +1! - these improvements are going to go a long ways to let 'debug' logging become usable, instead of the current blight caused by mod_ssl and other modules guilty of the low S/N ratio.
Re: Per-module / per-dir loglevel configuration version 4
On 6/3/2010 11:49 AM, Stefan Fritsch wrote: I have added two additional patches (filenames 10*) to fix a segfault. In addition, I now have this in http_log.h: #define APLOG_NO_MODULE -1 static int * const aplog_module_index; #define APLOG_MODULE_INDEX \ (aplog_module_index ? *aplog_module_index : APLOG_NO_MODULE) This means, if some source file does not initialize aplog_module_index with APLOG_USE_MODULE, logging will simply default to the global loglevel. This works because static pointers are initialized to NULL if no explicit initialization is given. I think that's a nice solution. Modules from 2.2.x continue to work without changes. But in order to benefit from per-module loglevel configuration, they have to use the new macros. This seems to be a very good solution, and the fact that their are no constructor-time calls to initialize this should avoid any platform quirks. My only question is; are we assured to have the same module_index reassigned on each config/reload phase? The reason I'm worried is that this code won't refresh the index, but there may be platforms that don't actually do a module unload/reload/reinit heap of aplog_module_index to NULL.
