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.
>
> 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.
Regards
Rüdiger
