You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "William A. Rowe, Jr." <wr...@rowe-clan.net> on 2007/08/27 10:28:09 UTC
Re: svn commit: r569947 - /httpd/httpd/branches/2.2.x/STATUS
Many thanks to Ruediger for reviewing 2.0 and 2.2 so far, and to both
Jim and Jeff for their reviews of current/2.2 modern flavors. I could
use a set of eyeballs on the final log.c patch for 2.2, and the patch
set for our old 'n crusty 2.0.
I'm especially interested if any Win32 folks want to take a peek at my
suggested mpm_winnt change set before I commit it to 2.0. Holler if
you are.
There is a patch over at apr-1.x trunk waiting for comments so I can
call 1.2 and 0.9 baked. Essentially, I've given up; Ryan and I had
attempted to make Win32 inheritance behave as Unix fork() inheritance,
and it just made zero sense and the end of the day.
If you study mpm_winnt, we don't even open our listeners and mutexes
as inherited; we start the process (suspended), duplicate the desired
handles into the child process, and then send the child's handle id's
down to it via the stdin channel. Sounds like unix domain sockets, eh?
So the model didn't work, and for NT I propose to stop inheriting the
handles other than stdin/out/err. Leave only the currently dup2()'ed
handle as inherited, and temporarily uninherit even those, when they
have been overridden. Why leave them inherited at all? For good old
classic apps that still rely on classic inheritance.
Since we can't do this in Win9x, there is a trivial patch that makes
the problem of holding open the old error log go away there, too.
--- server/log.c (revision 569931)
+++ server/log.c (working copy)
@@ -265,6 +265,10 @@
apr_proc_t *procnew;
apr_file_t *errfile;
+ if (dummy_stderr)
+ if ((rc = apr_file_open_stderr(&errfile, p)) == APR_SUCCESS)
+ apr_file_inherit_unset(errfile);
+
if (((rc = apr_procattr_create(&procattr, p)) == APR_SUCCESS)
&& ((rc = apr_procattr_cmdtype_set(procattr,
APR_SHELLCMD_ENV)) == APR_SUCCESS)
Essentially, before we play with proc_create, we tell apr that we don't want
stderr ever inherited by any process, ever again. This is a noop on NT, and
just forces stderr to be closed on fork before exec on unix (before, I trust,
stdout replaces the stderr.) So on unix, it's also effectively a no-op.
Something to ponder - I'm really not that motivated to apply this fix just
for Win9x, but if people holler before we tag httpd again, I'll stand behind
the patch. I don't know if it has any impact on the other implementations
of apr_proc_create().
Bill
wrowe@apache.org wrote:
> Author: wrowe
> Date: Sun Aug 26 18:24:43 2007
> New Revision: 569947
>
> URL: http://svn.apache.org/viewvc?rev=569947&view=rev
> Log:
> Some final suggestions for 2.2.6
>
> Modified:
> httpd/httpd/branches/2.2.x/STATUS
>
> Modified: httpd/httpd/branches/2.2.x/STATUS
> URL: http://svn.apache.org/viewvc/httpd/httpd/branches/2.2.x/STATUS?rev=569947&r1=569946&r2=569947&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.2.x/STATUS (original)
> +++ httpd/httpd/branches/2.2.x/STATUS Sun Aug 26 18:24:43 2007
> @@ -215,6 +215,25 @@
> http://svn.apache.org/viewvc?view=rev&revision=569622
> +1: niq, rpluem
>
> + * log core: ensure we use a special pool for stderr logging, so that
> + the stderr channel remains valid from the time plog is destroyed,
> + until the time the open_logs hook is called again. [William Rowe]
> + http://svn.apache.org/viewvc?view=rev&revision=569535
> + Backported to 2.2;
> + http://people.apache.org/~wrowe/r569535-backport-2.2.patch
> + +1: wrowe
> +
> + * mod_ssl: Version reporting update; displays 'compiled against'
> + Apache and build-time SSL Library versions at loglevel [info],
> + while reporting the run-time SSL Library version in the server
> + info tags. Helps to identify a mod_ssl built against one flavor
> + of OpenSSL but running against another (also adds SSL-C version
> + number reporting.) [William Rowe]
> + http://svn.apache.org/viewvc?view=rev&revision=520701
> + Backported to 2.2;
> + http://people.apache.org/~wrowe/r520701-backport-2.2.patch
> + +1: wrowe
> +
Re: svn commit: r569947 - /httpd/httpd/branches/2.2.x/STATUS
Posted by Tom Donovan <do...@bellatlantic.net>.
William A. Rowe, Jr. wrote:
> Many thanks to Ruediger for reviewing 2.0 and 2.2 so far, and to both
> Jim and Jeff for their reviews of current/2.2 modern flavors. I could
> use a set of eyeballs on the final log.c patch for 2.2, and the patch
> set for our old 'n crusty 2.0.
>
> I'm especially interested if any Win32 folks want to take a peek at my
> suggested mpm_winnt change set before I commit it to 2.0. Holler if
> you are.
>
re: mpm_winnt change 569541 for 2.2 -
This seems to cause "unable to replace stderr with error_log" CRIT
errors at startup.
No problem when started with -X, it happens only when there is a *real*
child process (which sure doesn't make debugging it any easier!)
I see this on w2k and XP. Maybe the child tries to set up stderr before
getting all the info from the parent?
-tom-
debug error.log from (Aug 27) 2.2.x tree running on win2k
====================================================
[Mon Aug 27 12:43:50 2007] [notice] Apache/2.2.6-dev (Win32) configured
-- resuming normal operations
[Mon Aug 27 12:43:50 2007] [notice] Server built: Aug 27 2007 12:39:13
[Mon Aug 27 12:43:50 2007] [notice] Parent: Created child process 2616
[Mon Aug 27 12:43:50 2007] [debug] mpm_winnt.c(487): Parent: Sent the
scoreboard to the child
[Mon Aug 27 12:43:50 2007] [crit] (OS 6)The handle is invalid. : unable
to replace stderr with error_log
[Mon Aug 27 12:43:50 2007] [crit] (2)No such file or directory: unable
to replace stderr with /dev/null
[Mon Aug 27 12:43:50 2007] [crit] (OS 6)The handle is invalid. : unable
to replace stderr with error_log
[Mon Aug 27 12:43:50 2007] [crit] (2)No such file or directory: unable
to replace stderr with /dev/null
[Mon Aug 27 12:43:50 2007] [notice] Child 2616: Child process is running
[Mon Aug 27 12:43:50 2007] [info] Parent: Duplicating socket 904 and
sending it to child process 2616
[Mon Aug 27 12:43:50 2007] [debug] mpm_winnt.c(408): Child 2616:
Retrieved our scoreboard from the parent.
[Mon Aug 27 12:43:50 2007] [debug] mpm_winnt.c(605): Parent: Sent 1
listeners to child 2616
[Mon Aug 27 12:43:50 2007] [debug] mpm_winnt.c(564): Child 2616:
retrieved 1 listeners from parent
[Mon Aug 27 12:43:50 2007] [notice] Child 2616: Acquired the start mutex.
[Mon Aug 27 12:43:50 2007] [notice] Child 2616: Starting 50 worker threads.
[Mon Aug 27 12:43:50 2007] [notice] Child 2616: Starting thread to
listen on port 80.
[
Re: svn commit: r569947 - /httpd/httpd/branches/2.2.x/STATUS
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Plüm wrote:
>
> I wouldn't say that it is a no-op on Unix. Some logger programs might
> expect an open stderr, even if this points to /dev/null. So I am not in
> favour of this patch. Besides I understood that we no longer support
> Win9x. So why making an exception here?
> IMHO if things do not work correctly on Win9x, bad luck.
I don't disagree. I raised it to point out why I deliberately neglected it.
I went over your other observations, thank you for pointing out the issues
with the patch. New one incoming.
Bill
Re: svn commit: r569947 - /httpd/httpd/branches/2.2.x/STATUS
Posted by Plüm,
Rüdiger,
VF-Group <ru...@vodafone.com>.
> -----Ursprüngliche Nachricht-----
> Von: William A. Rowe, Jr.
> Gesendet: Montag, 27. August 2007 10:28
> An: dev@httpd.apache.org
> Betreff: Re: svn commit: r569947 - /httpd/httpd/branches/2.2.x/STATUS
>
>
> So the model didn't work, and for NT I propose to stop inheriting the
> handles other than stdin/out/err. Leave only the currently dup2()'ed
> handle as inherited, and temporarily uninherit even those, when they
> have been overridden. Why leave them inherited at all? For good old
> classic apps that still rely on classic inheritance.
>
> Since we can't do this in Win9x, there is a trivial patch that makes
> the problem of holding open the old error log go away there, too.
>
> --- server/log.c (revision 569931)
> +++ server/log.c (working copy)
> @@ -265,6 +265,10 @@
> apr_proc_t *procnew;
> apr_file_t *errfile;
>
> + if (dummy_stderr)
> + if ((rc = apr_file_open_stderr(&errfile, p)) == APR_SUCCESS)
> + apr_file_inherit_unset(errfile);
> +
> if (((rc = apr_procattr_create(&procattr, p)) == APR_SUCCESS)
> && ((rc = apr_procattr_cmdtype_set(procattr,
>
> APR_SHELLCMD_ENV)) == APR_SUCCESS)
>
> Essentially, before we play with proc_create, we tell apr
> that we don't want
> stderr ever inherited by any process, ever again. This is a
> noop on NT, and
> just forces stderr to be closed on fork before exec on unix
> (before, I trust,
> stdout replaces the stderr.) So on unix, it's also
> effectively a no-op.
I wouldn't say that it is a no-op on Unix. Some logger programs might
expect an open stderr, even if this points to /dev/null. So I am not in
favour of this patch. Besides I understood that we no longer support
Win9x. So why making an exception here?
IMHO if things do not work correctly on Win9x, bad luck.
>
>
> wrowe@apache.org wrote:
> > Author: wrowe
> > Date: Sun Aug 26 18:24:43 2007
> > New Revision: 569947
> >
> > URL: http://svn.apache.org/viewvc?rev=569947&view=rev
> > Log:
> > Some final suggestions for 2.2.6
> >
> > Modified:
> > httpd/httpd/branches/2.2.x/STATUS
> >
> > Modified: httpd/httpd/branches/2.2.x/STATUS
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/branches/2.2.x/STATUS
?rev=569947&r1=569946&r2=569947&view=diff
> ==============================================================================
> --- httpd/httpd/branches/2.2.x/STATUS (original)
> +++ httpd/httpd/branches/2.2.x/STATUS Sun Aug 26 18:24:43 2007
> @@ -215,6 +215,25 @@
> http://svn.apache.org/viewvc?view=rev&revision=569622
> +1: niq, rpluem
>
> + * log core: ensure we use a special pool for stderr logging, so that
> + the stderr channel remains valid from the time plog is destroyed,
> + until the time the open_logs hook is called again. [William Rowe]
> + http://svn.apache.org/viewvc?view=rev&revision=569535
> + Backported to 2.2;
> + http://people.apache.org/~wrowe/r569535-backport-2.2.patch
> + +1: wrowe
Excerpt from http://people.apache.org/~wrowe/r569535-backport-2.2.patch
@@ -365,13 +409,19 @@
/* replace stderr with this new log */
apr_file_flush(s_main->error_log);
if ((rc = apr_file_open_stderr(&errfile, p)) == APR_SUCCESS) {
This line no longer makes sense. We do not use errfile. So why doing apr_file_open_stderr
into errfile?
- rc = apr_file_dup2(errfile, s_main->error_log, p);
+ rc = apr_file_dup2(stderr_log, s_main->error_log, p);
This is the wrong pool. It needs to be stderr_p instead of p.
}
if (rc != APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_CRIT, rc, s_main,
"unable to replace stderr with error_log");
}
else {
+ /* We are done with stderr_pool, close it, killing
+ * the previous generation's stderr logger
+ */
+ if (stderr_pool)
+ apr_pool_destroy(stderr_pool);
+ stderr_pool = stderr_p;
replace_stderr = 0;
}
}
Regards
Rüdiger