httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From <...@covalent.net>
Subject Re: cvs commit: httpd-2.0/modules/loggers mod_log_config.c
Date Wed, 18 Apr 2001 22:35:49 GMT
On Wed, 18 Apr 2001, Greg Stein wrote:

> On Wed, Apr 18, 2001 at 09:06:07PM -0000, rbb@apache.org wrote:
> >...
> >   --- http_core.c	2001/04/18 03:53:30	1.271
> >   +++ http_core.c	2001/04/18 21:06:06	1.272
> >   @@ -59,6 +59,7 @@
> >    #include "apr_strings.h"
> >    #include "apr_thread_proc.h"    /* for RLIMIT stuff */
> >    #include "apr_lib.h"
> >   +#include "apr_optional.h"
> >
> >    #define APR_WANT_STRFUNC
> >    #include "apr_want.h"
> >   @@ -77,6 +78,7 @@
> >    #include "scoreboard.h"
> >
> >    #include "mod_core.h"
> >   +#include "../loggers/mod_log_config.h"
>
> I'd say that modules/loggers/ should be added to the INCLUDES path so we
> don't need this relative path. mod_dav and mod_http (core) does this. Maybe
> for mod_include, too?
>
> Remember to install mod_log_config.h, too.

Yeah, I just followed what was there already.  We need to figure out how
we are going to do this stuff.

> >   +static void log_pre_config(apr_pool_t *p, apr_pool_t *plog, apr_pool_t *ptemp)
> >   +{
> >   +    static APR_OPTIONAL_FN_TYPE(ap_register_log_handler) *log_pfn_register;
> >   +
> >   +    log_hash = apr_hash_make(p);
> >   +    log_pfn_register = APR_RETRIEVE_OPTIONAL_FN(ap_register_log_handler);
> >   +
> >   +    if (log_pfn_register) {
>
> Why this monkey work? Why aren't you just calling ap_register_log_handler
> directly? There isn't any reason to do it via the OPTIONAL_FN stuff since
> the function is *in* this file.
>
> For clarity/maintainability, can't we just call directly?

Same reason we do it this way in mod_include.  This shows people how to do
it themselves, and it is a REALLY fast way to see when things break.  We
should always be eating our own dog food, otherwise we are asking to miss
problems.

> >    static void register_hooks(apr_pool_t *p)
> >    {
> >   +    ap_hook_pre_config(log_pre_config,NULL,NULL,APR_HOOK_REALLY_FIRST);
>
> I'd suggest just using APR_HOOK_MIDDLE. People that are using this module
> should be using the predecessor stuff, as you did in http_core.
>
> Any time we see "REALLY_FIRST", we should raise a red flag. That generally
> means something is going wrong (that we aren't using the hooks facilities as
> they were originally intended).

We can do that.  This was a part of debugging the problem with the hook
ordering, and I forgot about it.

> Anyways... great patch. It hides that ap_http_conn_rec properly. Now... can
> we make that structure private? :-)
>
> (personally, I'd hope to see mod_core.h (mod_http) totally private)

I agree, it should be totally private.  Feel free to make ap_http_conn_rec
private, or just wait until we can make the whole file private.

Ryan



_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------


Mime
View raw message