You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by je...@apache.org on 2001/09/21 18:14:50 UTC
cvs commit: apr/threadproc/unix proc.c
jerenkrantz 01/09/21 09:14:50
Modified: . CHANGES
threadproc/unix proc.c
Log:
Simplify apr_proc_wait_all_procs and consolidate apr_proc_wait.
(I had a similar version in my tree. Kevin's wins out because of the
WIF macros. Are there any platforms that don't have this? The Solaris
man page seems to indicate that they must be called, so it seems correct.
Please check on your favorite platform.)
Submitted by: Kevin Pilch-Bisson <ke...@pilch-bisson.net>
Reviewed by: Justin Erenkrantz
Revision Changes Path
1.159 +5 -0 apr/CHANGES
Index: CHANGES
===================================================================
RCS file: /home/cvs/apr/CHANGES,v
retrieving revision 1.158
retrieving revision 1.159
diff -u -r1.158 -r1.159
--- CHANGES 2001/09/19 20:08:05 1.158
+++ CHANGES 2001/09/21 16:14:50 1.159
@@ -1,5 +1,10 @@
Changes with APR b1
+ *) Make the unix version of apr_proc_wait_all_procs a simple wrapper
+ around apr_proc_wait, and which extracts the exit code from the
+ status returned by waitpid.
+ [Kevin Pilch-Bisson <ke...@pilch-bisson.net>]
+
*) Add process locking API to APR. [Aaron Bannert <aa...@clove.org>]
*) Add condition variables for Windows. [Ryan Bloom]
1.49 +21 -24 apr/threadproc/unix/proc.c
Index: proc.c
===================================================================
RCS file: /home/cvs/apr/threadproc/unix/proc.c,v
retrieving revision 1.48
retrieving revision 1.49
diff -u -r1.48 -r1.49
--- proc.c 2001/09/20 08:59:31 1.48
+++ proc.c 2001/09/21 16:14:50 1.49
@@ -361,22 +361,13 @@
return APR_SUCCESS;
}
-APR_DECLARE(apr_status_t) apr_proc_wait_all_procs(apr_proc_t *proc, apr_wait_t *status,
- apr_wait_how_e waithow, apr_pool_t *p)
+APR_DECLARE(apr_status_t) apr_proc_wait_all_procs(apr_proc_t *proc,
+ apr_wait_t *status,
+ apr_wait_how_e waithow,
+ apr_pool_t *p)
{
- int waitpid_options = WUNTRACED;
-
- if (waithow != APR_WAIT) {
- waitpid_options |= WNOHANG;
- }
-
- if ((proc->pid = waitpid(-1, status, waitpid_options)) > 0) {
- return APR_CHILD_DONE;
- }
- else if (proc->pid == 0) {
- return APR_CHILD_NOTDONE;
- }
- return errno;
+ proc->pid = -1;
+ return apr_proc_wait(proc, status, waithow);
}
APR_DECLARE(apr_status_t) apr_proc_wait(apr_proc_t *proc,
@@ -384,18 +375,24 @@
apr_wait_how_e waithow)
{
pid_t pstatus;
+ int waitpid_options = WUNTRACED;
+ int exit_int;
- if (waithow == APR_WAIT) {
- if ((pstatus = waitpid(proc->pid, exitcode, WUNTRACED)) > 0) {
- return APR_CHILD_DONE;
+ if (waithow != APR_WAIT) {
+ waitpid_options |= WNOHANG;
+ }
+
+ if ((pstatus = waitpid(proc->pid, &exit_int, waitpid_options)) > 0) {
+ if (proc->pid == -1) {
+ proc->pid = pstatus;
}
- else if (pstatus == 0) {
- return APR_CHILD_NOTDONE;
+ if (WIFEXITED(exit_int)) {
+ if (exitcode != NULL) {
+ *exitcode = WEXITSTATUS(exit_int);
+ }
+ return APR_CHILD_DONE;
}
- return errno;
- }
- if ((pstatus = waitpid(proc->pid, exitcode, WUNTRACED | WNOHANG)) > 0) {
- return APR_CHILD_DONE;
+ return APR_CHILD_NOTDONE;
}
else if (pstatus == 0) {
return APR_CHILD_NOTDONE;
Re: [PATCH] Re: cvs commit: apr/threadproc/unix proc.c
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Tue, Oct 16, 2001 at 08:41:35AM -0400, Jeff Trawick wrote:
> Ryan Bloom <rb...@covalent.net> writes:
>
> > On Monday 15 October 2001 10:38 am, Jeff Trawick wrote:
> >
> > I seriously doubt that we want to extend the apr_proc_t type anymore. Having
> > this type be complete has caused Windows to have to jump through hoops to
> > keep track of processes correctly, and adding more to it is a
> > mistake IMO.
>
> (rest of Ryan's comments omitted)
>
> I'll reimplement according to your suggestions and post again.
Ryan's solution seems fine to me. We're not usually too concerned with programs
exiting due to a signal, since we are just running diff, patch or a small
script. Sorry to have overlooked that when I made the patch before. Thanks for
noticing it Jeff.
> --
> Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
> http://www.geocities.com/SiliconValley/Park/9289/
> Born in Roswell... married an alien...
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson http://www.pilch-bisson.net
"Historically speaking, the presences of wheels in Unix
has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: [PATCH] Re: cvs commit: apr/threadproc/unix proc.c
Posted by Jeff Trawick <tr...@attglobal.net>.
Ryan Bloom <rb...@covalent.net> writes:
> On Monday 15 October 2001 10:38 am, Jeff Trawick wrote:
>
> I seriously doubt that we want to extend the apr_proc_t type anymore. Having
> this type be complete has caused Windows to have to jump through hoops to
> keep track of processes correctly, and adding more to it is a
> mistake IMO.
(rest of Ryan's comments omitted)
I'll reimplement according to your suggestions and post again.
--
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Re: cvs commit: apr/threadproc/unix proc.c
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 18 October 2001 12:15 pm, Jeff Trawick wrote:
> Ryan Bloom <rb...@covalent.net> writes:
> > I am missing something here. This patch requires us to use the
> > non-portable W* macros in Apache. That is wrong.
>
> This patch does not force an APR app to use any W* macros other than
> WCOREDUMP(), and most apps don't care about that.
As I read the last patch that you posted, all of the W* functions are still
being used in ap_process_child status.
>
> > This patch does not help APR to be
> > more portable, rather it allows programs that use APR to continue to use
> > non-portable constructs to solve the problem. I would fully expect that
> > in order for this to be done correctly, we will need to modify
> > ap_process_child_status in mpm_common.c to not use the W* functions. If
> > we do anything else, then we have not helped APR to be portable.
>
> We're trying to make an APR app more portable. I certainly plead
> guilty to allowing an APR app to use a non-portable construct when the
> app considers it necessary. I hardly invented the concept. It is an
> important feature of APR in various areas.
I am saying that I don't believe it is necessary here, and if it is, then this
implementation is not the correct one. Every other place that APR exposes
some native information, it is done by querying a structure. This patch exposes
a non-portable value as a part of the main-line function. No program can
ever use the native_exit_code variable and expect to be portable. If we are
going to introduce something like that, it should be returned by a function.
> What this patch does, if I may offer my version of
> motherhood-and-apple-pie, is increase the ability of an APR app to do
> meaningful stuff portably when compared with current CVS. It allows
> an APR app to do non-portable stuff if it so chooses by making
> available the native status code.
But the native status code is not what people would expect it to be. The native
status code should be what the program returned when it exited. It should not
be that code after it was munged by the OS.
> As far as apr_wait_or_timeout() and apr_process_child_status(), please
> see my my previous patch for examples on how to reduce the amount of
> non-portable constructs in Apache. I just don't want to muck with
> that at the moment. Unlike in my previous patch, I did not choose to
> use the portable view of exit status whenever possible because I want
> to get APR fixed appropriately first (while allowing all MPMs to work
> correctly).
I am very much against changing the API of an APR function temporarily.
Returning the native exit code is not the correct solution to this problem,
because that code can't really be used in a portable app. If the idea is to
go to three variables in the function, and then move back to two, then just
go straight to two. If the idea is to stick with three forever, then -1.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] Re: cvs commit: apr/threadproc/unix proc.c
Posted by Jeff Trawick <tr...@attglobal.net>.
Ryan Bloom <rb...@covalent.net> writes:
> I am missing something here. This patch requires us to use the non-portable
> W* macros in Apache. That is wrong.
This patch does not force an APR app to use any W* macros other than
WCOREDUMP(), and most apps don't care about that.
> This patch does not help APR to be
> more portable, rather it allows programs that use APR to continue to use
> non-portable constructs to solve the problem. I would fully expect that in
> order for this to be done correctly, we will need to modify
> ap_process_child_status in mpm_common.c to not use the W* functions. If we
> do anything else, then we have not helped APR to be portable.
We're trying to make an APR app more portable. I certainly plead
guilty to allowing an APR app to use a non-portable construct when the
app considers it necessary. I hardly invented the concept. It is an
important feature of APR in various areas.
What this patch does, if I may offer my version of
motherhood-and-apple-pie, is increase the ability of an APR app to do
meaningful stuff portably when compared with current CVS. It allows
an APR app to do non-portable stuff if it so chooses by making
available the native status code.
As far as apr_wait_or_timeout() and apr_process_child_status(), please
see my my previous patch for examples on how to reduce the amount of
non-portable constructs in Apache. I just don't want to muck with
that at the moment. Unlike in my previous patch, I did not choose to
use the portable view of exit status whenever possible because I want
to get APR fixed appropriately first (while allowing all MPMs to work
correctly).
--
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Re: cvs commit: apr/threadproc/unix proc.c
Posted by Ryan Bloom <rb...@covalent.net>.
On Thursday 18 October 2001 10:44 am, Jeff Trawick wrote:
> Here is yet another patch. When compared with the previous patch,
> this one adds the native status to the parameter lists of
> apr_proc_wait() and apr_proc_wait_all_procs(). Fewer MPM changes are
> necessary.
>
> missing: fix the doc in apr_thread_proc.h
> roll the changes into apr/threadproc/foo/proc.c, foo != unix
>
> untested, but there isn't much to screw up
>
> The interfaces are the important thing to look at.
I am missing something here. This patch requires us to use the non-portable
W* macros in Apache. That is wrong. This patch does not help APR to be
more portable, rather it allows programs that use APR to continue to use
non-portable constructs to solve the problem. I would fully expect that in
order for this to be done correctly, we will need to modify
ap_process_child_status in mpm_common.c to not use the W* functions. If we
do anything else, then we have not helped APR to be portable.
Ryan
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
Re: [PATCH] Re: cvs commit: apr/threadproc/unix proc.c
Posted by Greg Ames <gr...@remulak.net>.
Jeff Trawick wrote:
> I'm happy with the APR-ized notion of how-did-the-process-exit and
> what-is-the-signal-or-exit-code but throwing away the native status is
> a real problem.
[...]
> missing from patch:
> doc in header files, other mpms, apr/threadproc/foo, where foo !=
> unix, testing
>
> Index: include/mpm_common.h
I put this patch on, and AFAICT Apache is back to normal with respect to
child exit status.
Prior to doing the make install, I captured doc on another really
annoying phenomenon. Our httpd-std.conf contains "Group #-1". This
does not work on my Linux box. If I try to use it with the current
HEAD, we loop creating child processes which quickly exit with a fatal
error. The parent doesn't realize it, and just keeps forking doomed
children forever.
After applying Jeff's patch, the parent sees the fatal error and quickly
bails out. Then I restarted the server with a good config and forced a
seg fault in mod_info's handler. I see a log message, and the
scoreboard gets cleaned up properly.
I'm going to put this on daedalus and verify that it works there. If
so, I plan to put this into production this evening when the load dies
down. I'd like to see a more complete version of this patch or
something equivalent committed soon.
Greg
------------------------------------------------------------------------------------------------
results when using "Group #-1" on Linux, with httpd build from the
curent HEAD
Children are generating fatal errors which are not recognized by the
parent.
[gregames@gandalf /ap2.org]$ hps
27133 1 140 do_sel S pts/1 00:00:00 httpd
27276 27133 044 do_exi Z pts/1 00:00:00 httpd <defunct>
27277 27133 044 do_exi Z pts/1 00:00:00 httpd <defunct>
27278 27133 044 do_exi Z pts/1 00:00:00 httpd <defunct>
27279 27133 044 do_exi Z pts/1 00:00:00 httpd <defunct>
27280 27133 044 do_exi Z pts/1 00:00:00 httpd <defunct>
27281 27133 044 do_exi Z pts/1 00:00:00 httpd <defunct>
27282 27133 044 do_exi Z pts/1 00:00:00 httpd <defunct>
27283 27133 044 do_exi Z pts/1 00:00:00 httpd <defunct>
[gregames@gandalf /ap2.org]$ hps
27133 1 140 do_sel S pts/1 00:00:00 httpd
27309 27133 044 do_exi Z pts/1 00:00:00 httpd <defunct>
[gregames@gandalf /ap2.org]$ hps
27133 1 140 do_sel S pts/1 00:00:00 httpd
27337 27133 044 do_exi Z pts/1 00:00:00 httpd <defunct>
27338 27133 044 do_exi Z pts/1 00:00:00 httpd <defunct>
27339 27133 044 do_exi Z pts/1 00:00:00 httpd <defunct>
27340 27133 044 do_exi Z pts/1 00:00:00 httpd <defunct>
[gregames@gandalf /ap2.org]$ hps
27133 1 140 do_sel S pts/1 00:00:00 httpd
[gregames@gandalf /ap2.org]$ tail logs/error_log
[Thu Oct 18 11:33:03 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
[Thu Oct 18 11:33:03 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
[Thu Oct 18 11:33:03 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
[Thu Oct 18 11:33:03 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
[Thu Oct 18 11:33:03 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
[Thu Oct 18 11:33:03 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
[Thu Oct 18 11:33:03 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
[Thu Oct 18 11:33:03 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
[Thu Oct 18 11:33:05 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
this continues until you kill the parent.
applied Jeff's latest patch, tried the same config:
[Thu Oct 18 11:33:05 2001] [notice] caught SIGTERM, shutting down
[Thu Oct 18 12:23:56 2001] [notice] Apache/2.0.27-dev (Unix) configured
-- resuming normal operations
[Thu Oct 18 12:23:56 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
[Thu Oct 18 12:23:56 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
[Thu Oct 18 12:23:56 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
[Thu Oct 18 12:23:56 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
[Thu Oct 18 12:23:56 2001] [alert] (22)Invalid argument: setgid: unable
to set group id to Group 4294967295
[Thu Oct 18 12:23:57 2001] [alert] Child 29966 returned a Fatal error...
Apache is exiting!
hooray!
started the same build, with a config file containing a usable Group,
then forced a seg fault in mod_info's handler:
[Thu Oct 18 12:45:19 2001] [notice] child pid 673 exit signal
Segmentation fault (11)
yay!
checked the scoreboard with server-status. The slot where pid 673 would
have been contains a "." - open slot with no current process.
Re: [PATCH] Re: cvs commit: apr/threadproc/unix proc.c
Posted by Jeff Trawick <tr...@attglobal.net>.
Justin Erenkrantz <je...@ebuilt.com> writes:
> I think WIFSTOPPED needs to be checked (do all platforms have this?
> Linux does). But, I'm not sure that should return APR_CHILD_DONE or
> NOTDONE - since the process may resume later on. This gets hairy.
> Also, is returning APR_EGENERAL the right thing to do when we don't
> recognize the exithow? -- justin
We no longer pass WUNTRACED to waitpid(), so we no longer have to deal
with WIFSTOPPED().
As far as APR_EGENERAL, make up something else :) I dunno either.
--
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Re: cvs commit: apr/threadproc/unix proc.c
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Thu, Oct 18, 2001 at 01:44:08PM -0400, Jeff Trawick wrote:
> Here is yet another patch. When compared with the previous patch,
> this one adds the native status to the parameter lists of
> apr_proc_wait() and apr_proc_wait_all_procs(). Fewer MPM changes are
> necessary.
>
> missing: fix the doc in apr_thread_proc.h
> roll the changes into apr/threadproc/foo/proc.c, foo != unix
>
> untested, but there isn't much to screw up
>
> The interfaces are the important thing to look at.
Yeah. Looks good except for one caveat (see below).
> if (WIFEXITED(exit_int)) {
> - if (exitcode != NULL) {
> - *exitcode = WEXITSTATUS(exit_int);
> - }
> - return APR_CHILD_DONE;
> + *exithow = APR_CHILD_NORMAL_EXIT;
> + *exitcode_or_signal = WEXITSTATUS(exit_int);
> + }
> + else if (WIFSIGNALED(exit_int)) {
> + *exithow = APR_CHILD_SIGNAL_EXIT;
> + *exitcode_or_signal = WTERMSIG(exit_int);
> + }
> + else {
> + /* unexpected condition */
> + return APR_EGENERAL;
> }
> - return APR_CHILD_NOTDONE;
> + return APR_CHILD_DONE;
> }
I think WIFSTOPPED needs to be checked (do all platforms have this?
Linux does). But, I'm not sure that should return APR_CHILD_DONE or
NOTDONE - since the process may resume later on. This gets hairy.
Also, is returning APR_EGENERAL the right thing to do when we don't
recognize the exithow? -- justin
Re: [PATCH] Re: cvs commit: apr/threadproc/unix proc.c
Posted by Jeff Trawick <tr...@attglobal.net>.
Here is yet another patch. When compared with the previous patch,
this one adds the native status to the parameter lists of
apr_proc_wait() and apr_proc_wait_all_procs(). Fewer MPM changes are
necessary.
missing: fix the doc in apr_thread_proc.h
roll the changes into apr/threadproc/foo/proc.c, foo != unix
untested, but there isn't much to screw up
The interfaces are the important thing to look at.
Index: server/mpm_common.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm_common.c,v
retrieving revision 1.69
diff -u -r1.69 mpm_common.c
--- server/mpm_common.c 2001/09/21 14:29:33 1.69
+++ server/mpm_common.c 2001/10/18 17:31:01
@@ -124,7 +124,7 @@
continue;
proc.pid = pid;
- waitret = apr_proc_wait(&proc, NULL, APR_NOWAIT);
+ waitret = apr_proc_wait(&proc, NULL, NULL, NULL, APR_NOWAIT);
if (waitret != APR_CHILD_NOTDONE) {
MPM_NOTE_CHILD_KILLED(i);
continue;
@@ -204,7 +204,7 @@
if (wait_or_timeout_counter == INTERVAL_OF_WRITABLE_PROBES) {
wait_or_timeout_counter = 0;
}
- rv = apr_proc_wait_all_procs(ret, status, APR_NOWAIT, p);
+ rv = apr_proc_wait_all_procs(ret, NULL, NULL, status, APR_NOWAIT, p);
if (APR_STATUS_IS_EINTR(rv)) {
ret->pid = -1;
return;
@@ -213,6 +213,7 @@
return;
}
#ifdef NEED_WAITPID
+ /* wtf... ancient cruft? */
if ((ret = reap_children(status)) > 0) {
return;
}
Index: srclib/apr/include/apr_thread_proc.h
===================================================================
RCS file: /home/cvs/apr/include/apr_thread_proc.h,v
retrieving revision 1.75
diff -u -r1.75 apr_thread_proc.h
--- srclib/apr/include/apr_thread_proc.h 2001/09/20 08:59:30 1.75
+++ srclib/apr/include/apr_thread_proc.h 2001/10/18 17:31:04
@@ -81,6 +81,7 @@
typedef enum {APR_SHELLCMD, APR_PROGRAM} apr_cmdtype_e;
typedef enum {APR_WAIT, APR_NOWAIT} apr_wait_how_e;
+typedef enum {APR_CHILD_NORMAL_EXIT, APR_CHILD_SIGNAL_EXIT} apr_exit_how_e;
#define APR_NO_PIPE 0
#define APR_FULL_BLOCK 1
@@ -481,11 +482,18 @@
/**
* Wait for a child process to die
* @param proc The process handle that corresponds to the desired child process
- * @param exitcode The returned exit status of the child, if a child process
- * dies. On platforms that don't support obtaining this
- * information, the exitcode parameter will be returned as
- * APR_ENOTIMPL. This parameter may be NULL if the exit code
- * is not needed.
+ * @parameter exithow How the child exited. One of:
+ * <PRE>
+ * APR_CHILD_NORMAL_EXIT -- child exited normally
+ * APR_CHILD_SIGNAL_EXIT -- child exited due to an uncaught signal
+ * </PRE>
+ * This parameter may be NULL if the reason for exiting is not needed.
+ * @param exitcode_or_signal The returned exit status of the child, if a child
+ * process died normally, or the signal number if the child
+ * process died due to an uncaught signal. On platforms that
+ * don't support obtaining this information, the exitcode
+ * parameter will be returned as APR_ENOTIMPL. This parameter
+ * may be NULL if the exit code is not needed.
* @param waithow How should we wait. One of:
* <PRE>
* APR_WAIT -- block until the child process dies.
@@ -499,7 +507,9 @@
* </PRE>
*/
APR_DECLARE(apr_status_t) apr_proc_wait(apr_proc_t *proc,
- apr_wait_t *exitcode,
+ apr_exit_how_e *exithow,
+ int *exitcode_or_signal,
+ apr_wait_t *native_status,
apr_wait_how_e waithow);
/**
@@ -507,9 +517,6 @@
* about that child.
* @param proc Pointer to NULL on entry, will be filled out with child's
* information
- * @param status The returned exit status of the child, if a child process dies
- * On platforms that don't support obtaining this information,
- * the status parameter will be returned as APR_ENOTIMPL.
* @param waithow How should we wait. One of:
* <PRE>
* APR_WAIT -- block until the child process dies.
@@ -519,9 +526,11 @@
* @param p Pool to allocate child information out of.
*/
APR_DECLARE(apr_status_t) apr_proc_wait_all_procs(apr_proc_t *proc,
- apr_wait_t *status,
- apr_wait_how_e waithow,
- apr_pool_t *p);
+ apr_exit_how_e *exithow,
+ int *exitcode_or_signal,
+ apr_wait_t *native_status,
+ apr_wait_how_e waithow,
+ apr_pool_t *p);
/**
* Detach the process from the controlling terminal.
Index: srclib/apr/memory/unix/apr_pools.c
===================================================================
RCS file: /home/cvs/apr/memory/unix/apr_pools.c,v
retrieving revision 1.113
diff -u -r1.113 apr_pools.c
--- srclib/apr/memory/unix/apr_pools.c 2001/09/28 15:22:35 1.113
+++ srclib/apr/memory/unix/apr_pools.c 2001/10/18 17:31:09
@@ -1505,7 +1505,7 @@
#ifndef NEED_WAITPID
/* Pick up all defunct processes */
for (p = procs; p; p = p->next) {
- if (apr_proc_wait(p->pid, NULL, APR_NOWAIT) != APR_CHILD_NOTDONE) {
+ if (apr_proc_wait(p->pid, NULL, NULL, NULL, APR_NOWAIT) != APR_CHILD_NOTDONE) {
p->kill_how = kill_never;
}
}
@@ -1550,7 +1550,7 @@
/* Now wait for all the signaled processes to die */
for (p = procs; p; p = p->next) {
if (p->kill_how != kill_never) {
- (void) apr_proc_wait(p->pid, NULL, APR_WAIT);
+ (void) apr_proc_wait(p->pid, NULL, NULL, NULL, APR_WAIT);
}
}
#ifdef WIN32
Index: srclib/apr/threadproc/unix/proc.c
===================================================================
RCS file: /home/cvs/apr/threadproc/unix/proc.c,v
retrieving revision 1.49
diff -u -r1.49 proc.c
--- srclib/apr/threadproc/unix/proc.c 2001/09/21 16:14:50 1.49
+++ srclib/apr/threadproc/unix/proc.c 2001/10/18 17:31:12
@@ -362,37 +362,57 @@
}
APR_DECLARE(apr_status_t) apr_proc_wait_all_procs(apr_proc_t *proc,
- apr_wait_t *status,
+ apr_exit_how_e *exithow,
+ int *exitcode_or_signal,
+ apr_wait_t *native_status,
apr_wait_how_e waithow,
apr_pool_t *p)
{
proc->pid = -1;
- return apr_proc_wait(proc, status, waithow);
+ return apr_proc_wait(proc, exithow, exitcode_or_signal, native_status,
+ waithow);
}
APR_DECLARE(apr_status_t) apr_proc_wait(apr_proc_t *proc,
- apr_wait_t *exitcode,
+ apr_exit_how_e *exithow,
+ int *exitcode_or_signal,
+ apr_wait_t *native_status,
apr_wait_how_e waithow)
{
pid_t pstatus;
- int waitpid_options = WUNTRACED;
- int exit_int;
+ int waitpid_options = 0;
+ int exit_int, ignored_exitcode_or_signal;
+ apr_exit_how_e ignored_exithow;
+ apr_wait_t ignored_native_status;
+ /* exithow and exitcode_or_signal and ignored_native_status are optional */
+ if (!exithow)
+ exithow = &ignored_exithow;
+ if (!exitcode_or_signal)
+ exitcode_or_signal = &ignored_exitcode_or_signal;
+ if (!native_status)
+ native_status = &ignored_native_status;
+
if (waithow != APR_WAIT) {
waitpid_options |= WNOHANG;
}
if ((pstatus = waitpid(proc->pid, &exit_int, waitpid_options)) > 0) {
- if (proc->pid == -1) {
- proc->pid = pstatus;
- }
+ proc->pid = pstatus;
+ *native_status = exit_int;
if (WIFEXITED(exit_int)) {
- if (exitcode != NULL) {
- *exitcode = WEXITSTATUS(exit_int);
- }
- return APR_CHILD_DONE;
+ *exithow = APR_CHILD_NORMAL_EXIT;
+ *exitcode_or_signal = WEXITSTATUS(exit_int);
+ }
+ else if (WIFSIGNALED(exit_int)) {
+ *exithow = APR_CHILD_SIGNAL_EXIT;
+ *exitcode_or_signal = WTERMSIG(exit_int);
+ }
+ else {
+ /* unexpected condition */
+ return APR_EGENERAL;
}
- return APR_CHILD_NOTDONE;
+ return APR_CHILD_DONE;
}
else if (pstatus == 0) {
return APR_CHILD_NOTDONE;
--
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Re: cvs commit: apr/threadproc/unix proc.c
Posted by Jeff Trawick <tr...@attglobal.net>.
Ryan Bloom <rb...@covalent.net> writes:
> I would also get rid of the native exit code.
This is a real pain. Unfortunately, I only "felt" it and didn't have
any concrete examples until I got mostly done with a new patch :)
Consider the logic in Apache to see if a core dump occurred. This
isn't a portable concept. It seems best for APR not to get in-between
the OS and the app. Otherwise, we may find there are other bits in
the OS status which are of critical importance. All of this adds more
clumsiness to the APR interface if we try to APR-ize all the info.
I'm happy with the APR-ized notion of how-did-the-process-exit and
what-is-the-signal-or-exit-code but throwing away the native status is
a real problem.
And while at first I was fine with conveying the two pieces of
information in the parameter list to apr_proc_wait() and
apr_proc_wait_all_procs(), if the native status is provided the
parameter list gets a little more unwieldy.
So I have a new patch which I think does what you want (give or take
the names of identifiers), but it does not completely restore the info
we used to be able to get from apr_proc_wait().
Comments from anyone about what to change are most welcome. Simply
adding the native status to apr_proc_wait*() parmlist is okay with me,
but like I say it seems unwieldy.
missing from patch:
doc in header files, other mpms, apr/threadproc/foo, where foo !=
unix, testing
Index: include/mpm_common.h
===================================================================
RCS file: /home/cvs/httpd-2.0/include/mpm_common.h,v
retrieving revision 1.28
diff -u -r1.28 mpm_common.h
--- include/mpm_common.h 2001/08/14 12:30:49 1.28
+++ include/mpm_common.h 2001/10/16 17:57:09
@@ -125,7 +125,8 @@
* @param p The pool to allocate out of
*/
#ifdef AP_MPM_WANT_WAIT_OR_TIMEOUT
-void ap_wait_or_timeout(apr_wait_t *status, apr_proc_t *ret, apr_pool_t *p);
+void ap_wait_or_timeout(apr_exit_how_e *exithow, int *exitcode_or_signal,
+ apr_proc_t *ret, apr_pool_t *p);
#endif
/**
@@ -135,7 +136,8 @@
* @param status The status returned from ap_wait_or_timeout
*/
#ifdef AP_MPM_WANT_PROCESS_CHILD_STATUS
-void ap_process_child_status(apr_proc_t *pid, apr_wait_t status);
+void ap_process_child_status(apr_proc_t *pid, apr_exit_how_e exithow,
+ int exitcode_or_signal);
#endif
#if defined(TCP_NODELAY) && !defined(MPE) && !defined(TPF)
Index: server/mpm_common.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm_common.c,v
retrieving revision 1.69
diff -u -r1.69 mpm_common.c
--- server/mpm_common.c 2001/09/21 14:29:33 1.69
+++ server/mpm_common.c 2001/10/16 17:57:18
@@ -124,7 +124,7 @@
continue;
proc.pid = pid;
- waitret = apr_proc_wait(&proc, NULL, APR_NOWAIT);
+ waitret = apr_proc_wait(&proc, NULL, NULL, APR_NOWAIT);
if (waitret != APR_CHILD_NOTDONE) {
MPM_NOTE_CHILD_KILLED(i);
continue;
@@ -196,7 +196,8 @@
#endif
static int wait_or_timeout_counter;
-void ap_wait_or_timeout(apr_wait_t *status, apr_proc_t *ret, apr_pool_t *p)
+void ap_wait_or_timeout(apr_exit_how_e *exithow, int *exitcode_or_signal,
+ apr_proc_t *ret, apr_pool_t *p)
{
apr_status_t rv;
@@ -204,7 +205,7 @@
if (wait_or_timeout_counter == INTERVAL_OF_WRITABLE_PROBES) {
wait_or_timeout_counter = 0;
}
- rv = apr_proc_wait_all_procs(ret, status, APR_NOWAIT, p);
+ rv = apr_proc_wait_all_procs(ret, exithow, exitcode_or_signal, APR_NOWAIT, p);
if (APR_STATUS_IS_EINTR(rv)) {
ret->pid = -1;
return;
@@ -213,6 +214,7 @@
return;
}
#ifdef NEED_WAITPID
+ /* wtf... ancient cruft? */
if ((ret = reap_children(status)) > 0) {
return;
}
@@ -224,16 +226,16 @@
#endif /* AP_MPM_WANT_WAIT_OR_TIMEOUT */
#ifdef AP_MPM_WANT_PROCESS_CHILD_STATUS
-void ap_process_child_status(apr_proc_t *pid, apr_wait_t status)
+void ap_process_child_status(apr_proc_t *pid, apr_exit_how_e exithow,
+ int exitcode_or_signal)
{
- int signum = WTERMSIG(status);
- const char *sigdesc = apr_signal_get_description(signum);
+ const char *sigdesc;
/* Child died... if it died due to a fatal error,
* we should simply bail out.
*/
- if ((WIFEXITED(status)) &&
- WEXITSTATUS(status) == APEXIT_CHILDFATAL) {
+ if (exithow == APR_CHILD_NORMAL_EXIT &&
+ exitcode_or_signal == APEXIT_CHILDFATAL) {
ap_log_error(APLOG_MARK, APLOG_ALERT|APLOG_NOERRNO, 0, ap_server_conf,
"Child %ld returned a Fatal error..." APR_EOL_STR
"Apache is exiting!",
@@ -241,22 +243,23 @@
exit(APEXIT_CHILDFATAL);
}
- if (WIFSIGNALED(status)) {
- switch (signum) {
+ if (exithow == APR_CHILD_SIGNAL_EXIT) {
+ switch (exitcode_or_signal) {
case SIGTERM:
case SIGHUP:
case AP_SIG_GRACEFUL:
case SIGKILL:
break;
default:
-#ifdef WCOREDUMP
+ sigdesc = apr_signal_get_description(exitcode_or_signal);
+#ifdef WCOREDUMP_SHOWSTOPPER
if (WCOREDUMP(status)) {
ap_log_error(APLOG_MARK, APLOG_NOERRNO|APLOG_NOTICE,
0, ap_server_conf,
"child pid %ld exit signal %s (%d), "
"possible coredump in %s",
- (long)pid->pid, sigdesc, signum,
- ap_coredump_dir);
+ (long)pid->pid, sigdesc,
+ exitcode_or_signal, ap_coredump_dir);
}
else
#endif
@@ -264,7 +267,7 @@
ap_log_error(APLOG_MARK, APLOG_NOERRNO|APLOG_NOTICE,
0, ap_server_conf,
"child pid %ld exit signal %s (%d)",
- (long)pid->pid, sigdesc, signum);
+ (long)pid->pid, sigdesc, exitcode_or_signal);
}
}
}
Index: server/mpm/prefork/prefork.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm/prefork/prefork.c,v
retrieving revision 1.203
diff -u -r1.203 prefork.c
--- server/mpm/prefork/prefork.c 2001/10/11 16:28:23 1.203
+++ server/mpm/prefork/prefork.c 2001/10/16 17:57:25
@@ -1175,18 +1171,19 @@
while (!restart_pending && !shutdown_pending) {
int child_slot;
- apr_wait_t status;
+ apr_exit_how_e exithow;
+ int exitcode_or_signal;
/* this is a memory leak, but I'll fix it later. */
apr_proc_t pid;
- ap_wait_or_timeout(&status, &pid, pconf);
+ ap_wait_or_timeout(&exithow, &exitcode_or_signal, &pid, pconf);
/* XXX: if it takes longer than 1 second for all our children
* to start up and get into IDLE state then we may spawn an
* extra child
*/
if (pid.pid != -1) {
- ap_process_child_status(&pid, status);
+ ap_process_child_status(&pid, exithow, exitcode_or_signal);
/* non-fatal death... note that it's gone in the scoreboard. */
ap_sync_scoreboard_image();
child_slot = find_child_by_pid(&pid);
@@ -1203,7 +1200,7 @@
}
#if APR_HAS_OTHER_CHILD
}
- else if (apr_proc_other_child_read(&pid, status) == 0) {
+ else if (apr_proc_other_child_read(&pid, -1 /* status */) == 0) {
/* handled */
#endif
}
Index: srclib/apr/include/apr_thread_proc.h
===================================================================
RCS file: /home/cvs/apr/include/apr_thread_proc.h,v
retrieving revision 1.75
diff -u -r1.75 apr_thread_proc.h
--- srclib/apr/include/apr_thread_proc.h 2001/09/20 08:59:30 1.75
+++ srclib/apr/include/apr_thread_proc.h 2001/10/16 17:57:48
@@ -81,6 +81,7 @@
typedef enum {APR_SHELLCMD, APR_PROGRAM} apr_cmdtype_e;
typedef enum {APR_WAIT, APR_NOWAIT} apr_wait_how_e;
+typedef enum {APR_CHILD_NORMAL_EXIT, APR_CHILD_SIGNAL_EXIT} apr_exit_how_e;
#define APR_NO_PIPE 0
#define APR_FULL_BLOCK 1
@@ -481,11 +482,18 @@
/**
* Wait for a child process to die
* @param proc The process handle that corresponds to the desired child process
- * @param exitcode The returned exit status of the child, if a child process
- * dies. On platforms that don't support obtaining this
- * information, the exitcode parameter will be returned as
- * APR_ENOTIMPL. This parameter may be NULL if the exit code
- * is not needed.
+ * @parameter exithow How the child exited. One of:
+ * <PRE>
+ * APR_CHILD_NORMAL_EXIT -- child exited normally
+ * APR_CHILD_SIGNAL_EXIT -- child exited due to an uncaught signal
+ * </PRE>
+ * This parameter may be NULL if the reason for exiting is not needed.
+ * @param exitcode_or_signal The returned exit status of the child, if a child
+ * process died normally, or the signal number if the child
+ * process died due to an uncaught signal. On platforms that
+ * don't support obtaining this information, the exitcode
+ * parameter will be returned as APR_ENOTIMPL. This parameter
+ * may be NULL if the exit code is not needed.
* @param waithow How should we wait. One of:
* <PRE>
* APR_WAIT -- block until the child process dies.
@@ -499,7 +507,8 @@
* </PRE>
*/
APR_DECLARE(apr_status_t) apr_proc_wait(apr_proc_t *proc,
- apr_wait_t *exitcode,
+ apr_exit_how_e *exithow,
+ int *exitcode_or_signal,
apr_wait_how_e waithow);
/**
@@ -507,9 +516,6 @@
* about that child.
* @param proc Pointer to NULL on entry, will be filled out with child's
* information
- * @param status The returned exit status of the child, if a child process dies
- * On platforms that don't support obtaining this information,
- * the status parameter will be returned as APR_ENOTIMPL.
* @param waithow How should we wait. One of:
* <PRE>
* APR_WAIT -- block until the child process dies.
@@ -519,9 +525,10 @@
* @param p Pool to allocate child information out of.
*/
APR_DECLARE(apr_status_t) apr_proc_wait_all_procs(apr_proc_t *proc,
- apr_wait_t *status,
- apr_wait_how_e waithow,
- apr_pool_t *p);
+ apr_exit_how_e *exithow,
+ int *exitcode_or_signal,
+ apr_wait_how_e waithow,
+ apr_pool_t *p);
/**
* Detach the process from the controlling terminal.
Index: srclib/apr/memory/unix/apr_pools.c
===================================================================
RCS file: /home/cvs/apr/memory/unix/apr_pools.c,v
retrieving revision 1.113
diff -u -r1.113 apr_pools.c
--- srclib/apr/memory/unix/apr_pools.c 2001/09/28 15:22:35 1.113
+++ srclib/apr/memory/unix/apr_pools.c 2001/10/16 17:57:57
@@ -1505,7 +1505,7 @@
#ifndef NEED_WAITPID
/* Pick up all defunct processes */
for (p = procs; p; p = p->next) {
- if (apr_proc_wait(p->pid, NULL, APR_NOWAIT) != APR_CHILD_NOTDONE) {
+ if (apr_proc_wait(p->pid, NULL, NULL, APR_NOWAIT) != APR_CHILD_NOTDONE) {
p->kill_how = kill_never;
}
}
@@ -1550,7 +1550,7 @@
/* Now wait for all the signaled processes to die */
for (p = procs; p; p = p->next) {
if (p->kill_how != kill_never) {
- (void) apr_proc_wait(p->pid, NULL, APR_WAIT);
+ (void) apr_proc_wait(p->pid, NULL, NULL, APR_WAIT);
}
}
#ifdef WIN32
Index: srclib/apr/threadproc/unix/proc.c
===================================================================
RCS file: /home/cvs/apr/threadproc/unix/proc.c,v
retrieving revision 1.49
diff -u -r1.49 proc.c
--- srclib/apr/threadproc/unix/proc.c 2001/09/21 16:14:50 1.49
+++ srclib/apr/threadproc/unix/proc.c 2001/10/16 17:58:01
@@ -362,37 +362,50 @@
}
APR_DECLARE(apr_status_t) apr_proc_wait_all_procs(apr_proc_t *proc,
- apr_wait_t *status,
+ apr_exit_how_e *exithow,
+ int *exitcode_or_signal,
apr_wait_how_e waithow,
apr_pool_t *p)
{
proc->pid = -1;
- return apr_proc_wait(proc, status, waithow);
+ return apr_proc_wait(proc, exithow, exitcode_or_signal, waithow);
}
APR_DECLARE(apr_status_t) apr_proc_wait(apr_proc_t *proc,
- apr_wait_t *exitcode,
+ apr_exit_how_e *exithow,
+ int *exitcode_or_signal,
apr_wait_how_e waithow)
{
pid_t pstatus;
- int waitpid_options = WUNTRACED;
- int exit_int;
+ int waitpid_options = 0;
+ int exit_int, ignored_exitcode_or_signal;
+ apr_exit_how_e ignored_exithow;
+ /* exithow and exitcode_or_signal are optional */
+ if (!exithow)
+ exithow = &ignored_exithow;
+ if (!exitcode_or_signal)
+ exitcode_or_signal = &ignored_exitcode_or_signal;
+
if (waithow != APR_WAIT) {
waitpid_options |= WNOHANG;
}
if ((pstatus = waitpid(proc->pid, &exit_int, waitpid_options)) > 0) {
- if (proc->pid == -1) {
- proc->pid = pstatus;
- }
+ proc->pid = pstatus;
if (WIFEXITED(exit_int)) {
- if (exitcode != NULL) {
- *exitcode = WEXITSTATUS(exit_int);
- }
- return APR_CHILD_DONE;
+ *exithow = APR_CHILD_NORMAL_EXIT;
+ *exitcode_or_signal = WEXITSTATUS(exit_int);
+ }
+ else if (WIFSIGNALED(exit_int)) {
+ *exithow = APR_CHILD_SIGNAL_EXIT;
+ *exitcode_or_signal = WTERMSIG(exit_int);
+ }
+ else {
+ /* unexpected condition */
+ return APR_EGENERAL;
}
- return APR_CHILD_NOTDONE;
+ return APR_CHILD_DONE;
}
else if (pstatus == 0) {
return APR_CHILD_NOTDONE;
--
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Re: cvs commit: apr/threadproc/unix proc.c
Posted by Ryan Bloom <rb...@covalent.net>.
On Monday 15 October 2001 10:38 am, Jeff Trawick wrote:
I seriously doubt that we want to extend the apr_proc_t type anymore. Having
this type be complete has caused Windows to have to jump through hoops to
keep track of processes correctly, and adding more to it is a mistake IMO.
Also, I would completely ignore stopped processes, because I have a feeling
that they aren't portable at all. Finally, I would do this with just another argument
to apr_proc_wait_*. All we need is a simple enum that tells us how the process
died, either normally or because of a signal.
I would also get rid of the native exit code. In general, APR doesn't expose
information like this to the user. There are two locations that is not true, error
codes, and processes. Error codes are done this way, because we have
created macros to compare the codes. Processes are a mistake, and they
really should have remained incomplete types.
Opening up a third location where we expose the native result to the uesr is
just making programs less portable. From everything that I have read in this
thread, we are looking for two pieces of information.
1) Did the process die normally
2) What was the exit code or signal
Adding another int * to the argument list allows us to return both pieces
of information cleanly and easily.
Ryan
> This patch implements (for Unix, at least) what I mentioned in a
> previous post, namely, to make available all information about a
> terminated child but to classify it so that a portable app doesn't
> have to play games with WIFEXITED() et al.
>
> As I mentioned before, I don't know if we're doing the right thing on
> Unix w.r.t. capturing information about processes which are stopped.
______________________________________________________________
Ryan Bloom rbb@apache.org
Covalent Technologies rbb@covalent.net
--------------------------------------------------------------
[PATCH] Re: cvs commit: apr/threadproc/unix proc.c
Posted by Jeff Trawick <tr...@attglobal.net>.
This patch implements (for Unix, at least) what I mentioned in a
previous post, namely, to make available all information about a
terminated child but to classify it so that a portable app doesn't
have to play games with WIFEXITED() et al.
As I mentioned before, I don't know if we're doing the right thing on
Unix w.r.t. capturing information about processes which are stopped.
Index: server/mpm_common.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm_common.c,v
retrieving revision 1.69
diff -u -r1.69 mpm_common.c
--- server/mpm_common.c 2001/09/21 14:29:33 1.69
+++ server/mpm_common.c 2001/10/15 17:24:42
@@ -124,7 +124,7 @@
continue;
proc.pid = pid;
- waitret = apr_proc_wait(&proc, NULL, APR_NOWAIT);
+ waitret = apr_proc_wait(&proc, APR_NOWAIT);
if (waitret != APR_CHILD_NOTDONE) {
MPM_NOTE_CHILD_KILLED(i);
continue;
@@ -204,12 +204,13 @@
if (wait_or_timeout_counter == INTERVAL_OF_WRITABLE_PROBES) {
wait_or_timeout_counter = 0;
}
- rv = apr_proc_wait_all_procs(ret, status, APR_NOWAIT, p);
+ rv = apr_proc_wait_all_procs(ret, APR_NOWAIT, p);
if (APR_STATUS_IS_EINTR(rv)) {
ret->pid = -1;
return;
}
if (APR_STATUS_IS_CHILD_DONE(rv)) {
+ *status = ret->native_exit_status;
return;
}
#ifdef NEED_WAITPID
Index: srclib/apr/include/apr_thread_proc.h
===================================================================
RCS file: /home/cvs/apr/include/apr_thread_proc.h,v
retrieving revision 1.75
diff -u -r1.75 apr_thread_proc.h
--- srclib/apr/include/apr_thread_proc.h 2001/09/20 08:59:30 1.75
+++ srclib/apr/include/apr_thread_proc.h 2001/10/15 17:25:09
@@ -81,6 +81,7 @@
typedef enum {APR_SHELLCMD, APR_PROGRAM} apr_cmdtype_e;
typedef enum {APR_WAIT, APR_NOWAIT} apr_wait_how_e;
+typedef enum {APR_CHILD_NORMAL_EXIT, APR_CHILD_SIGNAL_EXIT, APR_CHILD_STOPPED} apr_exit_how_e;
#define APR_NO_PIPE 0
#define APR_FULL_BLOCK 1
@@ -124,6 +125,17 @@
apr_file_t *out;
/** Parent's side of pipe to child's stdouterr */
apr_file_t *err;
+ /** How did the child exit? (normally, via uncaught signal, etc.) */
+ apr_exit_how_e exit_how;
+ /** What did the OS return for the child exit status? */
+ apr_wait_t native_exit_status;
+ /** What is the exit code (for normal exit) or terminating signal? */
+ union {
+ int exit_code;
+#ifdef WIFSIGNALED
+ int signal;
+#endif
+ } exit_status;
#ifdef WIN32
/** Must retain the handle as any clone may not have the
* the same permissions
@@ -481,11 +493,6 @@
/**
* Wait for a child process to die
* @param proc The process handle that corresponds to the desired child process
- * @param exitcode The returned exit status of the child, if a child process
- * dies. On platforms that don't support obtaining this
- * information, the exitcode parameter will be returned as
- * APR_ENOTIMPL. This parameter may be NULL if the exit code
- * is not needed.
* @param waithow How should we wait. One of:
* <PRE>
* APR_WAIT -- block until the child process dies.
@@ -499,7 +506,6 @@
* </PRE>
*/
APR_DECLARE(apr_status_t) apr_proc_wait(apr_proc_t *proc,
- apr_wait_t *exitcode,
apr_wait_how_e waithow);
/**
@@ -507,9 +513,6 @@
* about that child.
* @param proc Pointer to NULL on entry, will be filled out with child's
* information
- * @param status The returned exit status of the child, if a child process dies
- * On platforms that don't support obtaining this information,
- * the status parameter will be returned as APR_ENOTIMPL.
* @param waithow How should we wait. One of:
* <PRE>
* APR_WAIT -- block until the child process dies.
@@ -519,9 +522,8 @@
* @param p Pool to allocate child information out of.
*/
APR_DECLARE(apr_status_t) apr_proc_wait_all_procs(apr_proc_t *proc,
- apr_wait_t *status,
- apr_wait_how_e waithow,
- apr_pool_t *p);
+ apr_wait_how_e waithow,
+ apr_pool_t *p);
/**
* Detach the process from the controlling terminal.
Index: srclib/apr/memory/unix/apr_pools.c
===================================================================
RCS file: /home/cvs/apr/memory/unix/apr_pools.c,v
retrieving revision 1.113
diff -u -r1.113 apr_pools.c
--- srclib/apr/memory/unix/apr_pools.c 2001/09/28 15:22:35 1.113
+++ srclib/apr/memory/unix/apr_pools.c 2001/10/15 17:25:17
@@ -1505,7 +1505,7 @@
#ifndef NEED_WAITPID
/* Pick up all defunct processes */
for (p = procs; p; p = p->next) {
- if (apr_proc_wait(p->pid, NULL, APR_NOWAIT) != APR_CHILD_NOTDONE) {
+ if (apr_proc_wait(p->pid, APR_NOWAIT) != APR_CHILD_NOTDONE) {
p->kill_how = kill_never;
}
}
@@ -1550,7 +1550,7 @@
/* Now wait for all the signaled processes to die */
for (p = procs; p; p = p->next) {
if (p->kill_how != kill_never) {
- (void) apr_proc_wait(p->pid, NULL, APR_WAIT);
+ (void) apr_proc_wait(p->pid, APR_WAIT);
}
}
#ifdef WIN32
Index: srclib/apr/threadproc/unix/proc.c
===================================================================
RCS file: /home/cvs/apr/threadproc/unix/proc.c,v
retrieving revision 1.49
diff -u -r1.49 proc.c
--- srclib/apr/threadproc/unix/proc.c 2001/09/21 16:14:50 1.49
+++ srclib/apr/threadproc/unix/proc.c 2001/10/15 17:25:23
@@ -362,16 +362,14 @@
}
APR_DECLARE(apr_status_t) apr_proc_wait_all_procs(apr_proc_t *proc,
- apr_wait_t *status,
apr_wait_how_e waithow,
apr_pool_t *p)
{
proc->pid = -1;
- return apr_proc_wait(proc, status, waithow);
+ return apr_proc_wait(proc, waithow);
}
APR_DECLARE(apr_status_t) apr_proc_wait(apr_proc_t *proc,
- apr_wait_t *exitcode,
apr_wait_how_e waithow)
{
pid_t pstatus;
@@ -383,16 +381,21 @@
}
if ((pstatus = waitpid(proc->pid, &exit_int, waitpid_options)) > 0) {
- if (proc->pid == -1) {
- proc->pid = pstatus;
- }
+ proc->pid = pstatus;
+ proc->native_exit_status = exit_int;
if (WIFEXITED(exit_int)) {
- if (exitcode != NULL) {
- *exitcode = WEXITSTATUS(exit_int);
- }
- return APR_CHILD_DONE;
+ proc->exit_how = APR_CHILD_NORMAL_EXIT;
+ proc->exit_status.exit_code = WEXITSTATUS(exit_int);
+ }
+ else if (WIFSIGNALED(exit_int)) {
+ proc->exit_how = APR_CHILD_SIGNAL_EXIT;
+ proc->exit_status.signal = WTERMSIG(exit_int);
+ }
+ else if (WIFSTOPPED(exit_int)) {
+ proc->exit_how = APR_CHILD_STOPPED;
+ proc->exit_status.signal = WSTOPSIG(exit_int);
}
- return APR_CHILD_NOTDONE;
+ return APR_CHILD_DONE;
}
else if (pstatus == 0) {
return APR_CHILD_NOTDONE;
--
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: cvs commit: apr/threadproc/unix proc.c
Posted by Jeff Trawick <tr...@attglobal.net>.
Kevin Pilch-Bisson <ke...@pilch-bisson.net> writes:
> WIFEXITED and WEXITSTATUS are not portable. Applications may need more detail
> than simply whether it exited due to a signal or not. For example, subversion
> has a pre-commit hook which prevents a commit if it returns non-zero. We need
> to be able to determine the exact return value. It is also useful when we run
> programs such as diff and patch, which return diagnostics via the exit code.
I think it was understood that they weren't portable (since apr_wait_t
was exposed). I understand why you want a nice congealed exit code,
but you can't have such a thing without telling the user what you got
(well, since a segfault currently reports APR_CHILD_NOT_DONE I guess
you have that, sort-of).
I wish I knew the history of these APR interfaces, but I don't. Why
do we bother to define apr_wait_t and thus expose these details to the
APR user, for example?
It would seem to me that more fields will be needed in the apr_proc_t
and we might as well get rid of the apr_wait_t parameter, since you
can't look at that portably without knowing how the process exited.
New fields in apr_proc_t, filled in by apr_proc_wait() and
apr_proc_wait_all_procs():
1) apr_wait_t native_exit_status (on Unix, this is the complete encoded
status)
2) some_enum exit_type (normal-termination, uncaught-signal,
stop-signal, whatever else)
3) union {
int signal; (a terminating signal or stopping signal)
int exit_code;
} exit_status;
I included mention of stopping signals here. I must confess that I
don't know why we care about stopped processes.
--
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: cvs commit: apr/threadproc/unix proc.c
Posted by Kevin Pilch-Bisson <ke...@pilch-bisson.net>.
On Sat, Oct 13, 2001 at 02:06:30PM -0400, Jeff Trawick wrote:
> Justin Erenkrantz <je...@ebuilt.com> writes:
>
> > On Fri, Oct 12, 2001 at 05:44:38PM -0400, Jeff Trawick wrote:
> > > no!!!!!!! Because of this, we're returning APR_CHILD_NOTDONE when a
> > > child exits due to a signal (like SIGSEGV)... thus Apache isn't able
> > > to see that the segfault happened and the log message is broken.
> > >
> > > The old code had this correct. If waitpid() returns >0, a child has
> > > finished. The WIF...() macros are just to find out how it finished
> > > (it chose to exit -- WIFEXITED, it got a signal -- WIFSIGNALED()).
> >
> > Um, okay. Is this acceptable? -- justin
> >
> > Index: threadproc/unix/proc.c
> > ===================================================================
> > RCS file: /home/cvs/apr/threadproc/unix/proc.c,v
> > retrieving revision 1.49
> > diff -u -r1.49 proc.c
> > --- threadproc/unix/proc.c 2001/09/21 16:14:50 1.49
> > +++ threadproc/unix/proc.c 2001/10/13 15:00:27
> > @@ -390,9 +390,8 @@
> > if (exitcode != NULL) {
> > *exitcode = WEXITSTATUS(exit_int);
> > }
> > - return APR_CHILD_DONE;
> > }
> > - return APR_CHILD_NOTDONE;
> > + return APR_CHILD_DONE;
> > }
> > else if (pstatus == 0) {
> > return APR_CHILD_NOTDONE;
>
> The handling of the return code looks right, but as far as I can
> tell we're still missing the ability for the caller of
> apr_proc_wait_all_procs() (and thus the caller of
> ap_wait_or_timeout()) to know how the child exited, since exitcode is
> only updated if the child chose to exit.
>
> I don't think apr_wait_t pretends to be some sort of portable
> the-process-chose-to-exit-with-this-return-code value, so it seems
> fine to feed back the raw OS status as we did before, whether the
> process exited normally or due to a signal.
As the one who proposed this patch, I thought I should jump in here.
WIFEXITED and WEXITSTATUS are not portable. Applications may need more detail
than simply whether it exited due to a signal or not. For example, subversion
has a pre-commit hook which prevents a commit if it returns non-zero. We need
to be able to determine the exact return value. It is also useful when we run
programs such as diff and patch, which return diagnostics via the exit code.
Maybe this isn't the right way to go about it, but I think there does need to
be a way of determining what the process chose to exit with.
>
> --
> Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
> http://www.geocities.com/SiliconValley/Park/9289/
> Born in Roswell... married an alien...
--
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Kevin Pilch-Bisson http://www.pilch-bisson.net
"Historically speaking, the presences of wheels in Unix
has never precluded their reinvention." - Larry Wall
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Re: cvs commit: apr/threadproc/unix proc.c
Posted by Jeff Trawick <tr...@attglobal.net>.
Justin Erenkrantz <je...@ebuilt.com> writes:
> On Fri, Oct 12, 2001 at 05:44:38PM -0400, Jeff Trawick wrote:
> > no!!!!!!! Because of this, we're returning APR_CHILD_NOTDONE when a
> > child exits due to a signal (like SIGSEGV)... thus Apache isn't able
> > to see that the segfault happened and the log message is broken.
> >
> > The old code had this correct. If waitpid() returns >0, a child has
> > finished. The WIF...() macros are just to find out how it finished
> > (it chose to exit -- WIFEXITED, it got a signal -- WIFSIGNALED()).
>
> Um, okay. Is this acceptable? -- justin
>
> Index: threadproc/unix/proc.c
> ===================================================================
> RCS file: /home/cvs/apr/threadproc/unix/proc.c,v
> retrieving revision 1.49
> diff -u -r1.49 proc.c
> --- threadproc/unix/proc.c 2001/09/21 16:14:50 1.49
> +++ threadproc/unix/proc.c 2001/10/13 15:00:27
> @@ -390,9 +390,8 @@
> if (exitcode != NULL) {
> *exitcode = WEXITSTATUS(exit_int);
> }
> - return APR_CHILD_DONE;
> }
> - return APR_CHILD_NOTDONE;
> + return APR_CHILD_DONE;
> }
> else if (pstatus == 0) {
> return APR_CHILD_NOTDONE;
The handling of the return code looks right, but as far as I can
tell we're still missing the ability for the caller of
apr_proc_wait_all_procs() (and thus the caller of
ap_wait_or_timeout()) to know how the child exited, since exitcode is
only updated if the child chose to exit.
I don't think apr_wait_t pretends to be some sort of portable
the-process-chose-to-exit-with-this-return-code value, so it seems
fine to feed back the raw OS status as we did before, whether the
process exited normally or due to a signal.
--
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: cvs commit: apr/threadproc/unix proc.c
Posted by Justin Erenkrantz <je...@ebuilt.com>.
On Fri, Oct 12, 2001 at 05:44:38PM -0400, Jeff Trawick wrote:
> no!!!!!!! Because of this, we're returning APR_CHILD_NOTDONE when a
> child exits due to a signal (like SIGSEGV)... thus Apache isn't able
> to see that the segfault happened and the log message is broken.
>
> The old code had this correct. If waitpid() returns >0, a child has
> finished. The WIF...() macros are just to find out how it finished
> (it chose to exit -- WIFEXITED, it got a signal -- WIFSIGNALED()).
Um, okay. Is this acceptable? -- justin
Index: threadproc/unix/proc.c
===================================================================
RCS file: /home/cvs/apr/threadproc/unix/proc.c,v
retrieving revision 1.49
diff -u -r1.49 proc.c
--- threadproc/unix/proc.c 2001/09/21 16:14:50 1.49
+++ threadproc/unix/proc.c 2001/10/13 15:00:27
@@ -390,9 +390,8 @@
if (exitcode != NULL) {
*exitcode = WEXITSTATUS(exit_int);
}
- return APR_CHILD_DONE;
}
- return APR_CHILD_NOTDONE;
+ return APR_CHILD_DONE;
}
else if (pstatus == 0) {
return APR_CHILD_NOTDONE;
Re: cvs commit: apr/threadproc/unix proc.c
Posted by Jeff Trawick <tr...@attglobal.net>.
jerenkrantz@apache.org writes:
> jerenkrantz 01/09/21 09:14:50
>
> Modified: . CHANGES
> threadproc/unix proc.c
> Log:
> Simplify apr_proc_wait_all_procs and consolidate apr_proc_wait.
>
> Index: proc.c
> diff -u -r1.48 -r1.49
> --- proc.c 2001/09/20 08:59:31 1.48
> +++ proc.c 2001/09/21 16:14:50 1.49
...
> APR_DECLARE(apr_status_t) apr_proc_wait(apr_proc_t *proc,
> @@ -384,18 +375,24 @@
> apr_wait_how_e waithow)
> {
> pid_t pstatus;
> + int waitpid_options = WUNTRACED;
> + int exit_int;
>
> - if (waithow == APR_WAIT) {
> - if ((pstatus = waitpid(proc->pid, exitcode, WUNTRACED)) > 0) {
> - return APR_CHILD_DONE;
> + if (waithow != APR_WAIT) {
> + waitpid_options |= WNOHANG;
> + }
> +
> + if ((pstatus = waitpid(proc->pid, &exit_int, waitpid_options)) > 0) {
> + if (proc->pid == -1) {
> + proc->pid = pstatus;
> }
> - else if (pstatus == 0) {
> - return APR_CHILD_NOTDONE;
> + if (WIFEXITED(exit_int)) {
> + if (exitcode != NULL) {
> + *exitcode = WEXITSTATUS(exit_int);
> + }
no!!!!!!! Because of this, we're returning APR_CHILD_NOTDONE when a
child exits due to a signal (like SIGSEGV)... thus Apache isn't able
to see that the segfault happened and the log message is broken.
The old code had this correct. If waitpid() returns >0, a child has
finished. The WIF...() macros are just to find out how it finished
(it chose to exit -- WIFEXITED, it got a signal -- WIFSIGNALED()).
--
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: cvs commit: apr/threadproc/unix proc.c
Posted by Jeff Trawick <tr...@attglobal.net>.
jerenkrantz@apache.org writes:
> jerenkrantz 01/09/21 09:14:50
>
> Modified: . CHANGES
> threadproc/unix proc.c
> Log:
> Simplify apr_proc_wait_all_procs and consolidate apr_proc_wait.
>
> Index: proc.c
> diff -u -r1.48 -r1.49
> --- proc.c 2001/09/20 08:59:31 1.48
> +++ proc.c 2001/09/21 16:14:50 1.49
...
> APR_DECLARE(apr_status_t) apr_proc_wait(apr_proc_t *proc,
> @@ -384,18 +375,24 @@
> apr_wait_how_e waithow)
> {
> pid_t pstatus;
> + int waitpid_options = WUNTRACED;
> + int exit_int;
>
> - if (waithow == APR_WAIT) {
> - if ((pstatus = waitpid(proc->pid, exitcode, WUNTRACED)) > 0) {
> - return APR_CHILD_DONE;
> + if (waithow != APR_WAIT) {
> + waitpid_options |= WNOHANG;
> + }
> +
> + if ((pstatus = waitpid(proc->pid, &exit_int, waitpid_options)) > 0) {
> + if (proc->pid == -1) {
> + proc->pid = pstatus;
> }
> - else if (pstatus == 0) {
> - return APR_CHILD_NOTDONE;
> + if (WIFEXITED(exit_int)) {
> + if (exitcode != NULL) {
> + *exitcode = WEXITSTATUS(exit_int);
> + }
no!!!!!!! Because of this, we're returning APR_CHILD_NOTDONE when a
child exits due to a signal (like SIGSEGV)... thus Apache isn't able
to see that the segfault happened and the log message is broken.
The old code had this correct. If waitpid() returns >0, a child has
finished. The WIF...() macros are just to find out how it finished
(it chose to exit -- WIFEXITED, it got a signal -- WIFSIGNALED()).
--
Jeff Trawick | trawick@attglobal.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...