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 Sun, 18 Jan 2009 15:37:22 GMT


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


Mime
View raw message