httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: svn commit: r735516 - /httpd/httpd/trunk/server/log.c
Date Wed, 13 May 2009 20:38:19 GMT
-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?


rjung@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))
> -
> 
> 
> 
> 


Mime
View raw message