httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Plüm, Rüdiger, VF-Group" <ruediger.pl...@vodafone.com>
Subject RE: Per-module / per-dir loglevel configuration version 4
Date Fri, 04 Jun 2010 11:40:42 GMT
 

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


Mime
View raw message