You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by wr...@apache.org on 2007/08/22 01:28:32 UTC
svn commit: r568326 - /httpd/httpd/trunk/server/log.c
Author: wrowe
Date: Tue Aug 21 16:28:32 2007
New Revision: 568326
URL: http://svn.apache.org/viewvc?rev=568326&view=rev
Log:
Refactor r452431, because this should not be fatal to starting
the server if it's horribly broken. The alternative of returing
'rc' in this case would be to open /dev/null as the error stream
for this generation of the server; and even more useless result.
Also the parent-file was never necessary; we can pass only the
child handle using apr_procattr_child_err_set() and have no extra
file to clean up afterwards.
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=568326&r1=568325&r2=568326&view=diff
==============================================================================
--- httpd/httpd/trunk/server/log.c (original)
+++ httpd/httpd/trunk/server/log.c Tue Aug 21 16:28:32 2007
@@ -250,18 +250,20 @@
APR_NO_PIPE,
APR_NO_PIPE)) == APR_SUCCESS)
&& ((rc = apr_procattr_error_check_set(procattr, 1)) == APR_SUCCESS)
- && ((rc = apr_procattr_child_errfn_set(procattr, log_child_errfn)) == APR_SUCCESS)
- && (!dummy_stderr
- || ((rc = apr_file_open_stdout(&errfile, p)) == APR_SUCCESS
- && (rc = apr_procattr_child_err_set(procattr,
- errfile,
- errfile)) == APR_SUCCESS))) {
+ && ((rc = apr_procattr_child_errfn_set(procattr, log_child_errfn))
+ == APR_SUCCESS)) {
char **args;
const char *pname;
apr_tokenize_to_argv(progname, &args, p);
pname = apr_pstrdup(p, args[0]);
procnew = (apr_proc_t *)apr_pcalloc(p, sizeof(*procnew));
+
+ if (dummy_stderr) {
+ if ((rc = apr_file_open_stdout(&errfile, p)) == APR_SUCCESS)
+ rc = apr_procattr_child_err_set(procattr, errfile, NULL);
+ }
+
rc = apr_proc_create(procnew, pname, (const char * const *)args,
NULL, procattr, p);
@@ -271,13 +273,6 @@
/* read handle to pipe not kept open, so no need to call
* close_handle_in_child()
*/
- }
-
- /* apr_procattr_child_err_set dups errfile twice: close the
- * remaining "parent-side" copy (apr_proc_create closes the
- * other). */
- if (dummy_stderr) {
- apr_file_close(procnew->err);
}
}
Re: svn commit: r568326 - /httpd/httpd/trunk/server/log.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 08/23/2007 10:32 PM, William A. Rowe, Jr. wrote:
> Ruediger Pluem wrote:
>>> As I got to thinking about this, when the situation is this fatal, why kill
>>> an otherwise perfectly healthy server? Worst case, we have some piped loggers
>>> which hang around longer than desired. It's a situation which I wouldn't
>>> want to bring down a production server for.
>> Ok, so the situation, that you cannot get the stdout handle, is very unlikely to
>> happen after you fixed the windows stdout problem, correct?
>> So if we deal with hanging piped loggers something else got terribly wrong.
>
> Yes, but it could be as simple as someone's module closing stdout
> inadvertently. That shouldn't kill the server, would you agree?
Yes, but I guess an error message to stderr could be helpful to detect this
provided that we a valid stderr :-).
Regards
Rüdiger
Re: svn commit: r568326 - /httpd/httpd/trunk/server/log.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Ruediger Pluem wrote:
>
>> The original patch was dying on win32 as-a-service, because apr_file_open_stdout
>> fails without a stdout handle.
>
> Ok, just for a non windows guy to understand: If httpd runs as a service we usually
> have no stdout handle and thus apr_file_open_stdout fails, correct?
Yup. (No stdin/stderr either although we worked around those, already).
>> Now, I fixed blessing win32 services with a stdout handle /Device/null.
>> That much is now healthy.
>
> Fixed in r568446, correct?
Right.
>> As I got to thinking about this, when the situation is this fatal, why kill
>> an otherwise perfectly healthy server? Worst case, we have some piped loggers
>> which hang around longer than desired. It's a situation which I wouldn't
>> want to bring down a production server for.
>
> Ok, so the situation, that you cannot get the stdout handle, is very unlikely to
> happen after you fixed the windows stdout problem, correct?
> So if we deal with hanging piped loggers something else got terribly wrong.
Yes, but it could be as simple as someone's module closing stdout
inadvertently. That shouldn't kill the server, would you agree?
Bill
Re: svn commit: r568326 - /httpd/httpd/trunk/server/log.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 08/23/2007 09:36 PM, William A. Rowe, Jr. wrote:
> Ruediger Pluem wrote:
>> On 08/22/2007 01:28 AM, wrowe@apache.org wrote:
>>> Author: wrowe
>>> Date: Tue Aug 21 16:28:32 2007
>>> New Revision: 568326
>>>
>>> URL: http://svn.apache.org/viewvc?rev=568326&view=rev
>>> Log:
>>> Refactor r452431, because this should not be fatal to starting
>>> the server if it's horribly broken. The alternative of returing
>>> 'rc' in this case would be to open /dev/null as the error stream
>>> for this generation of the server; and even more useless result.
>> Where do you see that /dev/null will be used as the error stream in this case?
>> As far as I can see if log_child returns something different than APR_SUCCESS
>> the whole open_logs hook fails and causes the process to die.
>
> The original patch was dying on win32 as-a-service, because apr_file_open_stdout
> fails without a stdout handle.
Ok, just for a non windows guy to understand: If httpd runs as a service we usually
have no stdout handle and thus apr_file_open_stdout fails, correct?
>
> Now, I fixed blessing win32 services with a stdout handle /Device/null.
> That much is now healthy.
Fixed in r568446, correct?
>
> As I got to thinking about this, when the situation is this fatal, why kill
> an otherwise perfectly healthy server? Worst case, we have some piped loggers
> which hang around longer than desired. It's a situation which I wouldn't
> want to bring down a production server for.
Ok, so the situation, that you cannot get the stdout handle, is very unlikely to
happen after you fixed the windows stdout problem, correct?
So if we deal with hanging piped loggers something else got terribly wrong.
Regards
Rüdiger
Re: svn commit: r568326 - /httpd/httpd/trunk/server/log.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Ruediger Pluem wrote:
>
> On 08/22/2007 01:28 AM, wrowe@apache.org wrote:
>> Author: wrowe
>> Date: Tue Aug 21 16:28:32 2007
>> New Revision: 568326
>>
>> URL: http://svn.apache.org/viewvc?rev=568326&view=rev
>> Log:
>> Refactor r452431, because this should not be fatal to starting
>> the server if it's horribly broken. The alternative of returing
>> 'rc' in this case would be to open /dev/null as the error stream
>> for this generation of the server; and even more useless result.
>
> Where do you see that /dev/null will be used as the error stream in this case?
> As far as I can see if log_child returns something different than APR_SUCCESS
> the whole open_logs hook fails and causes the process to die.
The original patch was dying on win32 as-a-service, because apr_file_open_stdout
fails without a stdout handle.
Now, I fixed blessing win32 services with a stdout handle /Device/null.
That much is now healthy.
As I got to thinking about this, when the situation is this fatal, why kill
an otherwise perfectly healthy server? Worst case, we have some piped loggers
which hang around longer than desired. It's a situation which I wouldn't
want to bring down a production server for.
Unfortunately, the perl-framework runs windows httpd as-a-console, and also
doesn't test piped logging, so this was never noticed when you and Joe had
originally offered up the solution. Joy :)
Bill
Re: svn commit: r568326 - /httpd/httpd/trunk/server/log.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 08/22/2007 01:28 AM, wrowe@apache.org wrote:
> Author: wrowe
> Date: Tue Aug 21 16:28:32 2007
> New Revision: 568326
>
> URL: http://svn.apache.org/viewvc?rev=568326&view=rev
> Log:
> Refactor r452431, because this should not be fatal to starting
> the server if it's horribly broken. The alternative of returing
> 'rc' in this case would be to open /dev/null as the error stream
> for this generation of the server; and even more useless result.
Where do you see that /dev/null will be used as the error stream in this case?
As far as I can see if log_child returns something different than APR_SUCCESS
the whole open_logs hook fails and causes the process to die.
Regards
Rüdiger