On 18.01.2009 16:37, Ruediger Pluem wrote:
On 01/18/2009 03:52 PM, Rainer Jung wrote:
Hi Rüdiger,
first thanks for reviewing.
On 17.01.2009 18:02, Ruediger Pluem wrote:
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)
@@ -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.
It *is* confusing.
Step 1:
-------
open_error_log() calls
piped_log_open() calls
apr_file_pipe_create_ex()
We get 2 FDs, say FD9 (read) and FD10 (write).
Step 2:
-------
piped_log_open() calls
piped_log_spawn() calls
apr_procattr_child_in_set()
We have 2 additional FDs, say FD11 and FD12, FD11 pointing to the same
end of the pipe as FD9 and FD12 as FD10.
Step 3:
-------
piped_log_spawn() calls
apr_proc_create()
This closes FD11.
Step 4:
-------
piped_log_spawn() calls
apr_file_close(procnew->in)
This closes FD12.
Now we have a separate process and the parent only has 2 additional FDs,
which can be used to communicate to the external process and to recreate
it.
The next steps do only happen for the main error logger:
Step 5:
-------
open_error_log() calls
apr_file_dup2(stderr_log, s_main->error_log, stderr_p)
s_main->error_log contains FD10, so the call lets FD2 (stderr_log) point
to the same pipe as FD10. FD9 and FD10 remain unchanged.
Step 6:
-------
open_error_log() calls
apr_file_close(s_main->error_log)
The call to apr_file_close happens in ap_open_logs not in open_error_log
and ap_open_logs is IMHO not called by piped_log_maintenance.
Yes, sorry, it is ap_open_logs.
But nevertheless:
During startup or after restart we go through ap_open_logs() and thus
after startup or restart, the write side of the original pl is closed
and the pipe is written to via the duped FD in stderr.
More precisely ap_open_logs() calls open_error_log(), which returns
ap_piped_log_write_fd(pl) in s->error_log, and after return
ap_open_logs() calls apr_file_close(s_main->error_log) is s==s_main.
When killing the logger, piped_log_maintenance() gets called. It can now
not simply use pl, because the write side has been closed before in
ap_open_logs().
I tried to keep the startup behaviour during restart, so I dup stderr to
the write side in the maintenance function, spawn and close the write
side again.
FD10 gets closed, we only have FD2 and FD9 remaining,
ap_piped_log_write_fd(pl) is gone now!
So if we want to keep this behaviour, we need to recreate the write side
of the pipe from stderr in maintenance before spawning again (in the
main error logger case), and close it after spawning.
I tried using FD2 directly as the write side without duping it, but that
doesn't work.
Hm. F10 should still be open and untouched.
Just to make sure we are talking about the same. This gets only closed
for the main error logger. For all other error loggers and the access
loggers, there's no such complication.
Regards,
Rainer
