On 01/16/2009 12:21 AM, Rainer Jung wrote:
> > This difference goes back to pre 1.3 httpd. > > The patch at > > http://people.apache.org/~rjung/patches/reliable_error_log.patch Just some quick comments below Index: server/log.c =================================================================== --- server/log.c (revision 734457) +++ server/log.c (working copy) @@ -319,20 +333,28 @@ int rc; if (*s->error_fname == '|') { - apr_file_t *dummy = NULL; + piped_log *pl; + fname = ap_server_root_relative(p, s->error_fname + 1); + if (!fname) { + ap_log_error(APLOG_MARK, APLOG_STARTUP, APR_EBADPATH, NULL, + "%s: Invalid error log path '%s'.", + ap_server_argv0, s->error_fname); + return NULL; + } + /* Spawn a new child logger. If this is the main server_rec, * the new child must use a dummy stderr since the current * stderr might be a pipe to the old logger. Otherwise, the * child inherits the parents stderr. */ - rc = log_child(p, s->error_fname + 1, &dummy, is_main); - if (rc != APR_SUCCESS) { - ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL, - "Couldn't start ErrorLog process"); + pl = piped_log_open(p, fname, is_main); + if (pl == NULL) { + ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL, + "%s: Couldn't start ErrorLog process '%s'.", + ap_server_argv0, fname); return DONE; Why not keeping the old way if #ifndef AP_HAVE_RELIABLE_PIPED_LOGS? } - - s->error_log = dummy; + s->error_log = ap_piped_log_write_fd(pl); } #ifdef HAVE_SYSLOG @@ -953,13 +992,26 @@ ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL, "piped log program '%s' failed unexpectedly", pl->program); - if ((stats = piped_log_spawn(pl)) != APR_SUCCESS) { + + /* If special_stderr is non-zero, we need to get the write + * end of the pipe back from stderr before spawning. */ + if ((plw->special_stderr && (stats = apr_file_dup(&ap_piped_log_write_fd(pl), + stderr_log, pl->p)) + != APR_SUCCESS) || + ((stats = piped_log_spawn(plw)) != APR_SUCCESS) || + /* If special_stderr is non-zero, we need to close the write + * end of the pipe after spawning, because we write to it via + * stderr. */ + (plw->special_stderr && (stats = apr_file_close(ap_piped_log_write_fd(pl))) + != APR_SUCCESS)) { I know that this is always a difficult and confusing part, so maybe it just confused me again, but why do we need to do the above? ap_piped_log_write_fd(pl) should have the correct fd already because in ap_open_logs we just duplicate the write side of the pipe to stderr and in all other cases (not main server error log) ap_piped_log_write_fd is just fine anyways. + + char buf[120]; + /* what can we do? This could be the error log we're having * problems opening up... */ - char buf[120]; ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL, "piped_log_maintenance: unable to respawn '%s': %s", - pl->program, apr_strerror(stats, buf, sizeof(buf))); + pl->program, apr_strerror(stats, buf, sizeof(buf))); } } break; @@ -1037,21 +1094,25 @@ return APR_SUCCESS; } -AP_DECLARE(piped_log *) ap_open_piped_log(apr_pool_t *p, const char *program) +static piped_log* piped_log_open(apr_pool_t *p, const char *program, + int special_stderr) { piped_log *pl; apr_file_t *dummy = NULL; int rc; - rc = log_child(p, program, &dummy, 0); + rc = log_child(p, program, &dummy, special_stderr); if (rc != APR_SUCCESS) { ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL, - "Couldn't start piped log process"); + "Couldn't start piped log process for %s", + program == NULL ? "'NULL'" : "'" program "'"); return NULL; } pl = apr_palloc(p, sizeof (*pl)); pl->p = p; + pl->program = (program == NULL) ? NULL : apr_pstrdup(p, program); + pl->pid = NULL; Why is this needed now? Or was this just missed previously and is not really related to the reliable pipe usage of the error log? ap_piped_log_read_fd(pl) = NULL; ap_piped_log_write_fd(pl) = dummy; apr_pool_cleanup_register(p, pl, piped_log_cleanup, piped_log_cleanup); > > should fix this. I tested it with access and error logs, with the main > server and virtual hosts and with restarts and logger killing. > > The problem is the delicate handling of file descriptors in the main > error log case. > > Should I apply this to trunk? I guess soon because this make discussion easier. > > My tests are only on Solaris. I will also test on Linux, but it would be > nice if someone could test on Windows too. > > Some comments on the patch: > > 1) The piped_log_maintenance() function used as a callback when the > logger dies needs one additional data, namely whether the died logger > was the main error log. We could add this to the piped_log structure, This is the part I don't get :-). Why do we need to know this? Regards Rüdiger
