-1. There's really no excuse to abstract an abstraction, when you could have simply added 'int special_stderr;' to the piped_log struct.
There are plenty of unnecessary changes here to accomplish what your patch set out to do. Let's stick with KISS, this isn't Java code :) You are also patching many different things in this patch; error message formatting, etc etc. Commit such as individual patches so that the significant changes are not lost in the noise. Please revert the introduction of a _wrapper struct and let's simply fix the piped_log structure? rj...@apache.org wrote: > Author: rjung > Date: Sun Jan 18 10:19:45 2009 > New Revision: 735516 > > URL: http://svn.apache.org/viewvc?rev=735516&view=rev > Log: > Piped error loggers should use the reliable pipes, > i.e. they should be automatically restarted when they die > similar to what happens for access loggers. > > Patch makes error loggers use the same code path as > access loggers. > > Side effect: patch adds ap_server_root_relative() to the error > logger path before spawning. > > Reviewed by R. Pluem. > > Patch needs to be tested on Windows. > > Modified: > httpd/httpd/trunk/server/log.c > > Modified: httpd/httpd/trunk/server/log.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/server/log.c?rev=735516&r1=735515&r2=735516&view=diff > ============================================================================== > --- httpd/httpd/trunk/server/log.c (original) > +++ httpd/httpd/trunk/server/log.c Sun Jan 18 10:19:45 2009 > @@ -141,6 +141,14 @@ > > static apr_file_t *stderr_log = NULL; > > +/* wrap a piped_log* with additional info > + * about special stderr treatment used for main > + * error logger */ > +typedef struct piped_log_wrapper { > + piped_log *pl; > + int special_stderr; > +} piped_log_wrapper; > + > /* track pipe handles to close in child process */ > typedef struct read_handle_t { > struct read_handle_t *next; > @@ -257,6 +265,7 @@ > "%s", description); > } > > +#ifndef AP_HAVE_RELIABLE_PIPED_LOGS > /* Create a child process running PROGNAME with a pipe connected to > * the childs stdin. The write-end of the pipe will be placed in > * *FPIN on successful return. If dummy_stderr is non-zero, the > @@ -310,6 +319,11 @@ > > return rc; > } > +#endif > + > +/* forward declaration */ > +static piped_log* piped_log_open(apr_pool_t *p, const char *program, > + int special_stderr); > > /* Open the error log for the given server_rec. If IS_MAIN is > * non-zero, s is the main server. */ > @@ -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; > } > - > - s->error_log = dummy; > + s->error_log = ap_piped_log_write_fd(pl); > } > > #ifdef HAVE_SYSLOG > @@ -361,7 +383,7 @@ > fname = ap_server_root_relative(p, s->error_fname); > if (!fname) { > ap_log_error(APLOG_MARK, APLOG_STARTUP, APR_EBADPATH, NULL, > - "%s: Invalid error log path %s.", > + "%s: Invalid error log path '%s'.", > ap_server_argv0, s->error_fname); > return DONE; > } > @@ -369,7 +391,7 @@ > APR_APPEND | APR_WRITE | APR_CREATE | > APR_LARGEFILE, > APR_OS_DEFAULT, p)) != APR_SUCCESS) { > ap_log_error(APLOG_MARK, APLOG_STARTUP, rc, NULL, > - "%s: could not open error log file %s.", > + "%s: could not open error log file '%s'.", > ap_server_argv0, fname); > return DONE; > } > @@ -406,7 +428,7 @@ > */ > apr_pool_create(&stderr_p, apr_pool_parent_get(p)); > apr_pool_tag(stderr_p, "stderr_pool"); > - > + > if (open_error_log(s_main, 1, stderr_p) != OK) { > return DONE; > } > @@ -414,7 +436,7 @@ > replace_stderr = 1; > if (s_main->error_log) { > apr_status_t rv; > - > + > /* Replace existing stderr with new log. */ > apr_file_flush(s_main->error_log); > rv = apr_file_dup2(stderr_log, s_main->error_log, stderr_p); > @@ -868,16 +890,23 @@ > > /* piped log support */ > > +AP_DECLARE(piped_log *) ap_open_piped_log(apr_pool_t *p, const char *program) > +{ > + return piped_log_open(p, program, 0); > +} > + > #ifdef AP_HAVE_RELIABLE_PIPED_LOGS > /* forward declaration */ > static void piped_log_maintenance(int reason, void *data, apr_wait_t status); > > /* Spawn the piped logger process pl->program. */ > -static apr_status_t piped_log_spawn(piped_log *pl) > +static apr_status_t piped_log_spawn(piped_log_wrapper *plw) > { > apr_procattr_t *procattr; > apr_proc_t *procnew = NULL; > + apr_file_t *errfile; > apr_status_t status; > + piped_log *pl = plw->pl; > > if (((status = apr_procattr_create(&procattr, pl->p)) != APR_SUCCESS) || > ((status = apr_procattr_cmdtype_set(procattr, > @@ -886,6 +915,14 @@ > ap_piped_log_read_fd(pl), > ap_piped_log_write_fd(pl))) > != APR_SUCCESS) || > + /* If special_stderr is non-zero, the stderr for the child will > + * be the same as the stdout of the parent. Otherwise the child > + * will inherit the stderr from the parent. */ > + (plw->special_stderr && (status = apr_file_open_stdout(&errfile, > pl->p)) > + != APR_SUCCESS) || > + (plw->special_stderr && (status = > apr_procattr_child_err_set(procattr, > + > errfile, NULL)) > + != APR_SUCCESS) || > ((status = apr_procattr_child_errfn_set(procattr, log_child_errfn)) > != APR_SUCCESS) || > ((status = apr_procattr_error_check_set(procattr, 1)) != > APR_SUCCESS)) { > @@ -895,6 +932,7 @@ > "piped_log_spawn: unable to setup child process '%s': > %s", > pl->program, apr_strerror(status, buf, sizeof(buf))); > } > + > else { > char **args; > const char *pname; > @@ -912,7 +950,7 @@ > * avoid a leak. */ > apr_file_close(procnew->in); > procnew->in = NULL; > - apr_proc_other_child_register(procnew, piped_log_maintenance, pl, > + apr_proc_other_child_register(procnew, piped_log_maintenance, > plw, > ap_piped_log_write_fd(pl), pl->p); > close_handle_in_child(pl->p, ap_piped_log_read_fd(pl)); > } > @@ -931,7 +969,8 @@ > > static void piped_log_maintenance(int reason, void *data, apr_wait_t status) > { > - piped_log *pl = data; > + piped_log_wrapper *plw = data; > + piped_log *pl = plw->pl; > apr_status_t stats; > int mpm_state; > > @@ -941,7 +980,7 @@ > pl->pid = NULL; /* in case we don't get it going again, this > * tells other logic not to try to kill it > */ > - apr_proc_other_child_unregister(pl); > + apr_proc_other_child_unregister(plw); > stats = ap_mpm_query(AP_MPMQ_MPM_STATE, &mpm_state); > if (stats != APR_SUCCESS) { > ap_log_error(APLOG_MARK, APLOG_STARTUP, 0, NULL, > @@ -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)) { > + > + 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; > @@ -1003,14 +1055,19 @@ > } > > > -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; > + piped_log_wrapper *plw; > > pl = apr_palloc(p, sizeof (*pl)); > pl->p = p; > pl->program = apr_pstrdup(p, program); > pl->pid = NULL; > + plw = apr_palloc(p, sizeof (*plw)); > + plw->special_stderr = special_stderr; > + plw->pl = pl; > if (apr_file_pipe_create_ex(&ap_piped_log_read_fd(pl), > &ap_piped_log_write_fd(pl), > APR_FULL_BLOCK, p) != APR_SUCCESS) { > @@ -1018,7 +1075,7 @@ > } > apr_pool_cleanup_register(p, pl, piped_log_cleanup, > piped_log_cleanup_for_exec); > - if (piped_log_spawn(pl) != APR_SUCCESS) { > + if (piped_log_spawn(plw) != APR_SUCCESS) { > apr_pool_cleanup_kill(p, pl, piped_log_cleanup); > apr_file_close(ap_piped_log_read_fd(pl)); > apr_file_close(ap_piped_log_write_fd(pl)); > @@ -1037,16 +1094,18 @@ > 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; > } > > @@ -1072,4 +1131,3 @@ > const request_rec *r, apr_pool_t *pool, > const char *errstr), (file, line, level, > status, s, r, pool, errstr)) > - > > > >
