On 11/07/2013 11:11 AM, Joe Orton wrote:
On Thu, Oct 17, 2013 at 12:33:50PM +0000, Plüm, Rüdiger, Vodafone Group wrote:
Hmm. This points out another issue when using an error log provider for the
main server log:
We lose everything that the server or other programs like CGI-scripts write to
the stderr FD as it
is simply written to /dev/null. Don't we need to have a separate process in
this case that
like a piped logger reads from the reading end of the "stderr pipe" and writes
it
via ap_server_conf->errorlog_provider->writer to the log?
Jumping in here...
Actually there should be few cases where modules write to stderr, no?
I'm not sure we should even consider "writes to stderr get logged" a
valid part of the module API. "If you're not using ap_log_*error,
you're out of luck" is not totally unreasonable.
mod_cgi does intercept stderr and logs it properly via ap_log_rerror,
and mod_cgid does stderr logging differently anyway.
One case which is common is the use of apr_procattr_child_errfn_set() to
write exec() errors to stderr after forking a child. But perhaps we
could and should provide a single common implementation of that which
does something more useful.
With "ErrorLog syslog" in current 2.x (and 1.3?), stderr is /dev/null
already, so the status quo is that we lose stuff, though that is kind of
ugly.
Reading this I think I could commit my httpd-trunk-stderr-provider.patch
from older email in this thread. It does not fix stderr logging, but it
fixes httpd-trunk problem where messages with NULL server_rec* are not
logged.
I'm attaching better version of that patch here (it fixes potential
crash pointed out by Rüdiger when both logf and error_log_provider are
NULL).
If anyone is against it or has better idea how to fix logging of
messages with NULL server_rec* when log providers are used, tell me.
Regards, Joe
Regards,
Jan Kaluza
Index: server/log.c
===================================================================
--- server/log.c (revision 1539566)
+++ server/log.c (working copy)
@@ -437,9 +437,12 @@
#define NULL_DEVICE "/dev/null"
#endif
- if (replace_stderr && freopen(NULL_DEVICE, "w", stderr) == NULL) {
- ap_log_error(APLOG_MARK, APLOG_CRIT, errno, s_main, APLOGNO(00093)
- "unable to replace stderr with %s", NULL_DEVICE);
+ if (replace_stderr) {
+ if (freopen(NULL_DEVICE, "w", stderr) == NULL) {
+ ap_log_error(APLOG_MARK, APLOG_CRIT, errno, s_main, APLOGNO(00093)
+ "unable to replace stderr with %s", NULL_DEVICE);
+ }
+ stderr_log = NULL;
}
for (virt = s_main->next; virt; virt = virt->next) {
@@ -1034,6 +1037,8 @@
const request_rec *rmain = NULL;
core_server_config *sconf = NULL;
ap_errorlog_info info;
+ ap_errorlog_provider *errorlog_provider = NULL;
+ void *errorlog_provider_handle = NULL;
/* do we need to log once-per-req or once-per-conn info? */
int log_conn_info = 0, log_req_info = 0;
@@ -1060,6 +1065,10 @@
#endif
logf = stderr_log;
+ if (!logf && ap_server_conf && ap_server_conf->errorlog_provider) {
+ errorlog_provider = ap_server_conf->errorlog_provider;
+ errorlog_provider_handle = ap_server_conf->errorlog_provider_handle;
+ }
}
else {
int configured_level = r ? ap_get_request_module_loglevel(r, module_index) :
@@ -1078,6 +1087,9 @@
logf = s->error_log;
}
+ errorlog_provider = s->errorlog_provider;
+ errorlog_provider_handle = s->errorlog_provider_handle;
+
/* the faked server_rec from mod_cgid does not have s->module_config */
if (s->module_config) {
sconf = ap_get_core_module_config(s->module_config);
@@ -1106,6 +1118,14 @@
}
}
+ if (!logf && !errorlog_provider) {
+ /* There is no file to send the log message to (or it is
+ * redirected to /dev/null and therefore any formating done below
+ * would be lost anyway) and there is no log provider available, so
+ * we just return here. */
+ return;
+ }
+
info.s = s;
info.c = c;
info.pool = pool;
@@ -1191,7 +1211,7 @@
continue;
}
- if (logf || (s->errorlog_provider->flags &
+ if (logf || (errorlog_provider->flags &
AP_ERRORLOG_PROVIDER_ADD_EOL_STR)) {
/* Truncate for the terminator (as apr_snprintf does) */
if (len > MAX_STRING_LEN - sizeof(APR_EOL_STR)) {
@@ -1205,8 +1225,8 @@
write_logline(errstr, len, logf, level_and_mask);
}
else {
- s->errorlog_provider->writer(&info, s->errorlog_provider_handle,
- errstr, len);
+ errorlog_provider->writer(&info, errorlog_provider_handle,
+ errstr, len);
}
if (done) {
