httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: [Patch] Reliable error logs for trunk
Date Sat, 17 Jan 2009 17:02:48 GMT


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

Mime
View raw message