You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by rb...@apache.org on 2001/04/18 23:06:07 UTC
cvs commit: httpd-2.0/modules/loggers mod_log_config.c
rbb 01/04/18 14:06:07
Modified: . CHANGES
modules/http http_core.c
modules/loggers mod_log_config.c
Log:
Allow modules to specify their own logging format specifier. Basically,
mod_log_config has registered an optional function, that other modules
can use to specify a function to be called. This is analogous to the way
that mod_include works. This also allows http to do the connection
logging itself, without exposing HTTP specific pieces to other modules.
Revision Changes Path
1.177 +9 -1 httpd-2.0/CHANGES
Index: CHANGES
===================================================================
RCS file: /home/cvs/httpd-2.0/CHANGES,v
retrieving revision 1.176
retrieving revision 1.177
diff -u -d -b -w -u -r1.176 -r1.177
--- CHANGES 2001/04/16 21:16:52 1.176
+++ CHANGES 2001/04/18 21:06:05 1.177
@@ -1,4 +1,12 @@
-Changes with Apache 2.0.17-dev
+Changes with Apache 2.0.18-dev
+
+ *) Allow modules to specify their own logging tags. This basically
+ allows a module to tell mod_log_config that when %x is encountered
+ a specific function should be called. Currently, x can be any single
+ character. It may be more useful to make this a string at some point.
+ [Ryan Bloom]
+
+Changes with Apache 2.0.17
*) If a higher-level filter handles the the byterange aspects of a
request, then the byterange filter should not try to redo the
1.272 +31 -0 httpd-2.0/modules/http/http_core.c
Index: http_core.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/http/http_core.c,v
retrieving revision 1.271
retrieving revision 1.272
diff -u -d -b -w -u -r1.271 -r1.272
--- 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"
static const char *set_keep_alive_timeout(cmd_parms *cmd, void *dummy,
const char *arg)
@@ -442,8 +444,37 @@
return OK;
}
+static const char *log_connection_status(request_rec *r, char *a)
+{
+ ap_http_conn_rec *hconn = ap_get_module_config(r->connection->conn_config,
+ &http_module);
+ if (r->connection->aborted)
+ return "X";
+
+ if ((r->connection->keepalive) &&
+ ((r->server->keep_alive_max - hconn->keepalives) > 0)) {
+ return "+";
+ }
+
+ return "-";
+}
+
+static void http_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_pfn_register = APR_RETRIEVE_OPTIONAL_FN(ap_register_log_handler);
+
+ if (log_pfn_register) {
+ log_pfn_register(p, "c", log_connection_status, 0);
+ }
+}
+
static void register_hooks(apr_pool_t *p)
{
+ static const char *const pred[] = { "mod_log_config.c", NULL };
+
+ ap_hook_pre_config(http_pre_config, pred, NULL, APR_HOOK_MIDDLE);
ap_hook_pre_connection(ap_pre_http_connection,NULL,NULL,
APR_HOOK_REALLY_LAST);
ap_hook_process_connection(ap_process_http_connection,NULL,NULL,
1.54 +60 -129 httpd-2.0/modules/loggers/mod_log_config.c
Index: mod_log_config.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/loggers/mod_log_config.c,v
retrieving revision 1.53
retrieving revision 1.54
diff -u -d -b -w -u -r1.53 -r1.54
--- mod_log_config.c 2001/04/18 03:53:33 1.53
+++ mod_log_config.c 2001/04/18 21:06:07 1.54
@@ -181,11 +181,14 @@
#include "apr_strings.h"
#include "apr_lib.h"
+#include "apr_hash.h"
+#include "apr_optional.h"
#define APR_WANT_STRFUNC
#include "apr_want.h"
#include "ap_config.h"
+#include "mod_log_config.h"
#include "httpd.h"
#include "http_config.h"
#include "http_core.h" /* For REMOTE_NAME */
@@ -206,6 +209,7 @@
static int xfer_flags = (APR_WRITE | APR_APPEND | APR_CREATE);
static apr_fileperms_t xfer_perms = APR_OS_DEFAULT;
+static apr_hash_t *log_hash;
/* POSIX.1 defines PIPE_BUF as the maximum number of bytes that is
* guaranteed to be atomic when writing a pipe. And PIPE_BUF >= 512
@@ -271,10 +275,8 @@
* Note that many of these could have ap_sprintfs replaced with static buffers.
*/
-typedef const char *(*item_key_func) (request_rec *, char *);
-
typedef struct {
- item_key_func func;
+ ap_log_handler_fn_t *func;
char *arg;
int condition_sense;
int want_orig;
@@ -526,133 +528,12 @@
{
return apr_psprintf(r->pool, "%ld", (long) getpid());
}
-static const char *log_connection_status(request_rec *r, char *a)
-{
-#ifdef AP_HTTP_ENABLED
- ap_http_conn_rec *hconn = ap_get_module_config(r->connection->conn_config,
- &http_module);
-#endif
- if (r->connection->aborted)
- return "X";
-
-#ifdef AP_HTTP_ENABLED
- if ((r->connection->keepalive) &&
- ((r->server->keep_alive_max - hconn->keepalives) > 0)) {
- return "+";
- }
-#endif
- return "-";
-}
/*****************************************************************
*
* Parsing the log format string
*/
-static struct log_item_list {
- char ch;
- item_key_func func;
- int want_orig_default;
-} log_item_keys[] = {
-
- {
- 'h', log_remote_host, 0
- },
- {
- 'a', log_remote_address, 0
- },
- {
- 'A', log_local_address, 0
- },
- {
- 'l', log_remote_logname, 0
- },
- {
- 'u', log_remote_user, 0
- },
- {
- 't', log_request_time, 0
- },
- {
- 'T', log_request_duration, 1
- },
- {
- 'r', log_request_line, 1
- },
- {
- 'f', log_request_file, 0
- },
- {
- 'U', log_request_uri, 1
- },
- {
- 's', log_status, 1
- },
- {
- 'b', clf_log_bytes_sent, 0
- },
- {
- 'B', log_bytes_sent, 0
- },
- {
- 'i', log_header_in, 0
- },
- {
- 'o', log_header_out, 0
- },
- {
- 'n', log_note, 0
- },
- {
- 'e', log_env_var, 0
- },
- {
- 'V', log_server_name, 0
- },
- {
- 'v', log_virtual_host, 0
- },
- {
- 'p', log_server_port, 0
- },
- {
- 'P', log_child_pid, 0
- },
- {
- 'H', log_request_protocol, 0
- },
- {
- 'm', log_request_method, 0
- },
- {
- 'q', log_request_query, 0
- },
- {
- 'c', log_connection_status, 0
- },
- {
- 'C', log_cookie, 0
- },
- {
- 'D', log_request_duration_microseconds, 1
- },
- {
- '\0'
- }
-};
-
-static struct log_item_list *find_log_func(char k)
-{
- int i;
-
- for (i = 0; log_item_keys[i].ch; ++i)
- if (k == log_item_keys[i].ch) {
- return &log_item_keys[i];
- }
-
- return NULL;
-}
-
static char *parse_log_misc_string(apr_pool_t *p, log_format_item *it,
const char **sa)
{
@@ -718,6 +599,7 @@
static char *parse_log_item(apr_pool_t *p, log_format_item *it, const char **sa)
{
const char *s = *sa;
+ ap_log_handler *handler;
if (*s != '%') {
return parse_log_misc_string(p, it, sa);
@@ -731,7 +613,6 @@
while (*s) {
int i;
- struct log_item_list *l;
switch (*s) {
case '!':
@@ -779,8 +660,8 @@
break;
default:
- l = find_log_func(*s++);
- if (!l) {
+ handler = (ap_log_handler *)apr_hash_get(log_hash, s++, 1);
+ if (!handler) {
char dummy[2];
dummy[0] = s[-1];
@@ -788,9 +669,9 @@
return apr_pstrcat(p, "Unrecognized LogFormat directive %",
dummy, NULL);
}
- it->func = l->func;
+ it->func = handler->func;
if (it->want_orig == -1) {
- it->want_orig = l->want_orig_default;
+ it->want_orig = handler->want_orig_default;
}
*sa = s;
return NULL;
@@ -1260,11 +1141,61 @@
#endif
}
+static void ap_register_log_handler(apr_pool_t *p, char *tag,
+ ap_log_handler_fn_t *handler, int def)
+{
+ ap_log_handler *log_struct = apr_palloc(p, sizeof(*log_struct));
+ log_struct->func = handler;
+ log_struct->want_orig_default = def;
+
+ apr_hash_set(log_hash, tag, 1, (const void *)log_struct);
+}
+
+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) {
+ log_pfn_register(p, "h", log_remote_host, 0);
+ log_pfn_register(p, "a", log_remote_address, 0 );
+ log_pfn_register(p, "A", log_local_address, 0 );
+ log_pfn_register(p, "l", log_remote_logname, 0);
+ log_pfn_register(p, "u", log_remote_user, 0);
+ log_pfn_register(p, "t", log_request_time, 0);
+ log_pfn_register(p, "f", log_request_file, 0);
+ log_pfn_register(p, "b", clf_log_bytes_sent, 0);
+ log_pfn_register(p, "B", log_bytes_sent, 0);
+ log_pfn_register(p, "i", log_header_in, 0);
+ log_pfn_register(p, "o", log_header_out, 0);
+ log_pfn_register(p, "n", log_note, 0);
+ log_pfn_register(p, "e", log_env_var, 0);
+ log_pfn_register(p, "V", log_server_name, 0);
+ log_pfn_register(p, "v", log_virtual_host, 0);
+ log_pfn_register(p, "p", log_server_port, 0);
+ log_pfn_register(p, "P", log_child_pid, 0);
+ log_pfn_register(p, "H", log_request_protocol, 0);
+ log_pfn_register(p, "m", log_request_method, 0);
+ log_pfn_register(p, "q", log_request_query, 0);
+ log_pfn_register(p, "C", log_cookie, 0);
+ log_pfn_register(p, "r", log_request_line, 1);
+ log_pfn_register(p, "D", log_request_duration_microseconds, 1);
+ log_pfn_register(p, "T", log_request_duration, 1);
+ log_pfn_register(p, "U", log_request_uri, 1);
+ log_pfn_register(p, "s", log_status, 1);
+ }
+}
+
static void register_hooks(apr_pool_t *p)
{
+ ap_hook_pre_config(log_pre_config,NULL,NULL,APR_HOOK_REALLY_FIRST);
ap_hook_child_init(init_child,NULL,NULL,APR_HOOK_MIDDLE);
ap_hook_open_logs(init_config_log,NULL,NULL,APR_HOOK_MIDDLE);
ap_hook_log_transaction(multi_log_transaction,NULL,NULL,APR_HOOK_MIDDLE);
+
+ APR_REGISTER_OPTIONAL_FN(ap_register_log_handler);
}
module AP_MODULE_DECLARE_DATA log_config_module =
Re: cvs commit: httpd-2.0/modules/loggers mod_log_config.c
Posted by Bill Stoddard <bi...@wstoddard.com>.
> On Fri, 20 Apr 2001, Bill Stoddard wrote:
>
> > Debugging another problem related to this patch got me to thinking....
> >
> > <vent>
> > Is this patch pedantic? I mean, what is wrong with letting mod_log_config handle the %c
directive
> > just as it always has? We are trying to file every single little nit in its tight little spot
at
> > the expense of maintainability. This code base is getting seriously complex. Stuff like this
makes
> > it even worse.
> > </vent>
>
> This patch is important, because it means that if I want to log something,
> I don't have to modify mod_log_config. Think of cookie logging. Why do
> we enable cookie logging if mod_usertrack isn't in the server? I want to
> be able to extend logging without modifying another module.
>
> yes, this may be a bit more complex, but it puts the onus of logging a
> feature on the code that implements the feature. I would much rather do
> that, then to put the onus on a completely unrelated logging module.
>
I very much like the idea of optional functions. I was specifically questioning moving the %c
handling from mod_log_config to the core. Gotta get active writing some docs cause some of this
stuff is seriously complicated. :-)
Bill
Re: cvs commit: httpd-2.0/modules/loggers mod_log_config.c
Posted by rb...@covalent.net.
On Fri, 20 Apr 2001, Bill Stoddard wrote:
> Debugging another problem related to this patch got me to thinking....
>
> <vent>
> Is this patch pedantic? I mean, what is wrong with letting mod_log_config handle the %c directive
> just as it always has? We are trying to file every single little nit in its tight little spot at
> the expense of maintainability. This code base is getting seriously complex. Stuff like this makes
> it even worse.
> </vent>
This patch is important, because it means that if I want to log something,
I don't have to modify mod_log_config. Think of cookie logging. Why do
we enable cookie logging if mod_usertrack isn't in the server? I want to
be able to extend logging without modifying another module.
yes, this may be a bit more complex, but it puts the onus of logging a
feature on the code that implements the feature. I would much rather do
that, then to put the onus on a completely unrelated logging module.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: httpd-2.0/modules/loggers mod_log_config.c
Posted by Bill Stoddard <st...@raleigh.ibm.com>.
Debugging another problem related to this patch got me to thinking....
<vent>
Is this patch pedantic? I mean, what is wrong with letting mod_log_config handle the %c directive
just as it always has? We are trying to file every single little nit in its tight little spot at
the expense of maintainability. This code base is getting seriously complex. Stuff like this makes
it even worse.
</vent>
Bill
> rbb 01/04/18 14:06:07
>
> Modified: . CHANGES
> modules/http http_core.c
> modules/loggers mod_log_config.c
> Log:
> Allow modules to specify their own logging format specifier. Basically,
> mod_log_config has registered an optional function, that other modules
> can use to specify a function to be called. This is analogous to the way
> that mod_include works. This also allows http to do the connection
> logging itself, without exposing HTTP specific pieces to other modules.
>
Re: cvs commit: httpd-2.0/modules/loggers mod_log_config.c
Posted by rb...@covalent.net.
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
-------------------------------------------------------------------------------
Re: cvs commit: httpd-2.0/modules/loggers mod_log_config.c
Posted by Greg Stein <gs...@lyra.org>.
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.
>...
> --- mod_log_config.c 2001/04/18 03:53:33 1.53
> +++ mod_log_config.c 2001/04/18 21:06:07 1.54
>...
> +static void ap_register_log_handler(apr_pool_t *p, char *tag,
> + ap_log_handler_fn_t *handler, int def)
> +{
> + ap_log_handler *log_struct = apr_palloc(p, sizeof(*log_struct));
> + log_struct->func = handler;
> + log_struct->want_orig_default = def;
> +
> + apr_hash_set(log_hash, tag, 1, (const void *)log_struct);
> +}
> +
> +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?
> 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).
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)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/loggers mod_log_config.c
Posted by rb...@covalent.net.
Yep, done now. Thanks.
Ryan
On Wed, 18 Apr 2001, Greg Stein wrote:
> On Wed, Apr 18, 2001 at 09:06:07PM -0000, rbb@apache.org wrote:
> > rbb 01/04/18 14:06:07
> >
> > Modified: . CHANGES
> > modules/http http_core.c
> > modules/loggers mod_log_config.c
>
> Hrm. I think you may have forgot to add mod_log_config.h ?
>
> Cheers,
> -g
>
> --
> Greg Stein, http://www.lyra.org/
>
>
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: httpd-2.0/modules/loggers mod_log_config.c
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Apr 18, 2001 at 09:06:07PM -0000, rbb@apache.org wrote:
> rbb 01/04/18 14:06:07
>
> Modified: . CHANGES
> modules/http http_core.c
> modules/loggers mod_log_config.c
Hrm. I think you may have forgot to add mod_log_config.h ?
Cheers,
-g
--
Greg Stein, http://www.lyra.org/