You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by gs...@apache.org on 2002/01/28 22:58:16 UTC
cvs commit: apr/threadproc/win32 proc.c
gstein 02/01/28 13:58:16
Modified: include apr_thread_proc.h
threadproc/beos proc.c
threadproc/netware proc.c
threadproc/os2 proc.c
threadproc/unix proc.c
threadproc/win32 proc.c
Log:
Add a couple new command types to process creation:
APR_PROGRAM_ENV: start the program using the caller's environment
APR_PROGRAM_PATH: search PATH for the program, use caller's env
(the normal APR_PROGRAM isolates the env and does not use PATH)
The BeOS, OS/2, and Win32 implementations are incomplete. These two
new forms just default back to APR_PROGRAM for now. (although BeOS
doesn't even distinguish between APR_SHELLCMD and APR_PROGRAM!)
On Unix and Netware, these map into execv() and execvp() calls.
Also clarified some doc for the enums in apr_thread_proc.h a bit.
Revision Changes Path
1.79 +25 -8 apr/include/apr_thread_proc.h
Index: apr_thread_proc.h
===================================================================
RCS file: /home/cvs/apr/include/apr_thread_proc.h,v
retrieving revision 1.78
retrieving revision 1.79
diff -u -r1.78 -r1.79
--- apr_thread_proc.h 27 Dec 2001 17:02:59 -0000 1.78
+++ apr_thread_proc.h 28 Jan 2002 21:58:15 -0000 1.79
@@ -71,7 +71,7 @@
#endif /* __cplusplus */
/**
* @file apr_thread_proc.h
- * @brief APR Thread Library
+ * @brief APR Thread and Process Library
*/
/**
* @defgroup APR_Thread Thread Library
@@ -79,15 +79,28 @@
* @{
*/
-typedef enum {APR_SHELLCMD, APR_PROGRAM} apr_cmdtype_e;
-typedef enum {APR_WAIT, APR_NOWAIT} apr_wait_how_e;
+typedef enum {
+ APR_SHELLCMD, /**< use the shell to invoke the program */
+ APR_PROGRAM, /**< invoke the program directly, no copied env */
+ APR_PROGRAM_ENV, /**< invoke the program, replicating our environment */
+ APR_PROGRAM_PATH /**< find program on PATH, use our environment */
+} apr_cmdtype_e;
+
+typedef enum {
+ APR_WAIT, /**< wait for the specified process to finish */
+ APR_NOWAIT /**< do not wait -- just see if it has finished */
+} apr_wait_how_e;
+
/* I am specifically calling out the values so that the macros below make
* more sense. Yes, I know I don't need to, but I am hoping this makes what
* I am doing more clear. If you want to add more reasons to exit, continue
* to use bitmasks.
*/
-typedef enum {APR_PROC_EXIT = 1, APR_PROC_SIGNAL = 2,
- APR_PROC_SIGNAL_CORE = 4} apr_exit_why_e;
+typedef enum {
+ APR_PROC_EXIT = 1, /**< process exited normally */
+ APR_PROC_SIGNAL = 2, /**< process exited due to a signal */
+ APR_PROC_SIGNAL_CORE = 4 /**< process exited and dumped a core file */
+} apr_exit_why_e;
#define APR_PROC_CHECK_EXIT(x) (x & APR_PROC_EXIT)
#define APR_PROC_CHECK_SIGNALED(x) (x & APR_PROC_SIGNAL)
@@ -428,8 +441,10 @@
* @param attr The procattr we care about.
* @param cmd The type of command. One of:
* <PRE>
- * APR_SHELLCMD -- Shell script
- * APR_PROGRAM -- Executable program (default)
+ * APR_SHELLCMD -- Shell script
+ * APR_PROGRAM -- Executable program (default)
+ * APR_PROGRAM_ENV -- Executable program, copy environment
+ * APR_PROGRAM_PATH -- Executable program on PATH, copy env
* </PRE>
*/
APR_DECLARE(apr_status_t) apr_procattr_cmdtype_set(apr_procattr_t *attr,
@@ -477,7 +492,9 @@
* @param const_args the arguments to pass to the new program. The first
* one should be the program name.
* @param env The new environment table for the new process. This
- * should be a list of NULL-terminated strings.
+ * should be a list of NULL-terminated strings. This argument
+ * is ignored for APR_PROGRAM_ENV and APR_PROGRAM_PATH types
+ * of commands.
* @param attr the procattr we should use to determine how to create the new
* process
* @param cont The pool to use.
1.44 +2 -0 apr/threadproc/beos/proc.c
Index: proc.c
===================================================================
RCS file: /home/cvs/apr/threadproc/beos/proc.c,v
retrieving revision 1.43
retrieving revision 1.44
diff -u -r1.43 -r1.44
--- proc.c 14 Dec 2001 01:12:25 -0000 1.43
+++ proc.c 28 Jan 2002 21:58:15 -0000 1.44
@@ -249,6 +249,8 @@
}
newargs[nargs] = NULL;
+ /* ### we should be looking at attr->cmdtype in here... */
+
newproc = load_image(nargs, (const char**)newargs, (const char**)env);
/* load_image copies the data so now we can free it... */
1.5 +14 -1 apr/threadproc/netware/proc.c
Index: proc.c
===================================================================
RCS file: /home/cvs/apr/threadproc/netware/proc.c,v
retrieving revision 1.4
retrieving revision 1.5
diff -u -r1.4 -r1.5
--- proc.c 8 Jan 2002 21:00:37 -0000 1.4
+++ proc.c 28 Jan 2002 21:58:15 -0000 1.5
@@ -349,11 +349,24 @@
}
execve(SHELL_PATH, (char * const *) newargs, (char * const *)env);
}
- else {
+ else if (attr->cmdtype == APR_PROGRAM) {
if (attr->detached) {
apr_proc_detach();
}
execve(progname, (char * const *)args, (char * const *)env);
+ }
+ else if (attr->cmdtype == APR_PROGRAM_ENV) {
+ if (attr->detached) {
+ apr_proc_detach();
+ }
+ execv(progname, (char * const *)args);
+ }
+ else {
+ /* APR_PROGRAM_PATH */
+ if (attr->detached) {
+ apr_proc_detach();
+ }
+ execvp(progname, (char * const *)args);
}
exit(-1); /* if we get here, there is a problem, so exit with an */
/* error code. */
1.48 +2 -0 apr/threadproc/os2/proc.c
Index: proc.c
===================================================================
RCS file: /home/cvs/apr/threadproc/os2/proc.c,v
retrieving revision 1.47
retrieving revision 1.48
diff -u -r1.47 -r1.48
--- proc.c 26 Oct 2001 02:31:04 -0000 1.47
+++ proc.c 28 Jan 2002 21:58:15 -0000 1.48
@@ -345,6 +345,8 @@
if (extension == NULL || strchr(extension, '/') || strchr(extension, '\\'))
extension = "";
+ /* ### how to handle APR_PROGRAM_ENV and APR_PROGRAM_PATH? */
+
if (attr->cmdtype == APR_SHELLCMD || strcasecmp(extension, ".cmd") == 0) {
strcpy(interpreter, "#!" SHELL_PATH);
extra_arg = "/C";
1.55 +14 -1 apr/threadproc/unix/proc.c
Index: proc.c
===================================================================
RCS file: /home/cvs/apr/threadproc/unix/proc.c,v
retrieving revision 1.54
retrieving revision 1.55
diff -u -r1.54 -r1.55
--- proc.c 25 Jan 2002 02:21:43 -0000 1.54
+++ proc.c 28 Jan 2002 21:58:15 -0000 1.55
@@ -366,11 +366,24 @@
}
execve(SHELL_PATH, (char * const *) newargs, (char * const *)env);
}
- else {
+ else if (attr->cmdtype == APR_PROGRAM) {
if (attr->detached) {
apr_proc_detach();
}
execve(progname, (char * const *)args, (char * const *)env);
+ }
+ else if (attr->cmdtype == APR_PROGRAM_ENV) {
+ if (attr->detached) {
+ apr_proc_detach();
+ }
+ execv(progname, (char * const *)args);
+ }
+ else {
+ /* APR_PROGRAM_PATH */
+ if (attr->detached) {
+ apr_proc_detach();
+ }
+ execvp(progname, (char * const *)args);
}
exit(-1); /* if we get here, there is a problem, so exit with an */
/* error code. */
1.62 +2 -0 apr/threadproc/win32/proc.c
Index: proc.c
===================================================================
RCS file: /home/cvs/apr/threadproc/win32/proc.c,v
retrieving revision 1.61
retrieving revision 1.62
diff -u -r1.61 -r1.62
--- proc.c 28 Jan 2002 15:56:08 -0000 1.61
+++ proc.c 28 Jan 2002 21:58:16 -0000 1.62
@@ -343,6 +343,8 @@
i++;
}
+ /* ### how to handle APR_PROGRAM_ENV and APR_PROGRAM_PATH? */
+
if (attr->cmdtype == APR_SHELLCMD) {
char *shellcmd = getenv("COMSPEC");
if (!shellcmd)
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by Karl Fogel <kf...@newton.ch.collab.net>.
And he's off!...
> gstein 02/01/28 13:58:16
> Modified: include apr_thread_proc.h
> threadproc/beos proc.c
> threadproc/netware proc.c
> threadproc/os2 proc.c
> threadproc/unix proc.c
> threadproc/win32 proc.c
> Log:
> Add a couple new command types to process creation:
>
> APR_PROGRAM_ENV: start the program using the caller's environment
> APR_PROGRAM_PATH: search PATH for the program, use caller's env
>
> (the normal APR_PROGRAM isolates the env and does not use PATH)
>
> The BeOS, OS/2, and Win32 implementations are incomplete. These two
> new forms just default back to APR_PROGRAM for now. (although BeOS
> doesn't even distinguish between APR_SHELLCMD and APR_PROGRAM!)
>
> On Unix and Netware, these map into execv() and execvp() calls.
>
> Also clarified some doc for the enums in apr_thread_proc.h a bit.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by Greg Stein <gs...@lyra.org>.
On Tue, Jan 29, 2002 at 08:34:43PM +1000, Brian Havard wrote:
>...
> Well, other platforms CAN provide it and I'd guess you could do it on unix
> too if you really tried (do the path search yourself perhaps?). The
> non-unix platforms sometimes have to jump through hoops to implement
> behaviour that is standard on unix (usually because the API was designed by
> a unixhead :) so why shouldn't the reverse be true sometimes?
That would be fine, but add it as a cmdtype, rather than determination based
on what you happen to pass as 'env' to apr_proc_create(). By using a
cmdtype, then a platform can bail at cmdtype_set() time (like Netware does).
Returning ENOTIMPL from apr_proc_create() would be bad -- it doesn't signify
that it was a *combination* of args that is not implemented.
Much easier to return something from cmdtype_set() to indicate that a
particular type is not supported on the platform.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Jan 28, 2002 at 11:35:20PM -0600, William A. Rowe, Jr. wrote:
> From: "Greg Stein" <gs...@lyra.org>
> Sent: Monday, January 28, 2002 9:50 PM
>...
> > But it would also open up a combination that isn't possible --
> > APR_PROGRAM_PATH with env==[...]. In words that is, "search for the program
> > on PATH, and here is the environment to use." That combination isn't
> > available on Unix, at least.
>
> Can we say APR_ENOTIMPL? Seriously, Win32 always searches the program path
> if the executable (1st arg to CreateProcess) is NULL. It executes a very
> specific program if the 1st arg is given. This part of your new schema is
> great goodness.
See my note to Brian, re: using a cmdtype rather than at proc_create() time.
> But If I'm Not Mistaken [Read: I'm probably mistaken], exec() is essentially
> a fork() -> load process -> run sequence [please correct me.] There is nothing
> keeping us from replacing the environment after we've done the fork(), is there?
The process is: fork() to duplicate the process (into a parent and child),
then the child process replaces itself by using exec(). The child then
starts running.
The environment is (typically) readonly, so it cannot be replaced after the
exec() occur.
>...
> > The enumerated type gives us the ability to specify the exact combinations
> > that are allowed to callers.
>
> Ugh. It is not exactly quaint, but I begin to see some value here. Still,
> this just makes more sense as flags, if you ask me [who was asking :-?] EVEN
> if we treat it as flags, but only allow specific combinations. At least we
> can test them by bit to find specific logic rules.
I prefer the enumerations, but a set of flags would be acceptable. As long
as it is cmdtype_set() that returns the ENOTIMPL. proc_create() would simply
ignore/use the 'env' parameter as defined for the command type.
(it could also be acceptable to return an error if env is provided but not
being used; "error" also meaning an abort() is possible :-)
And in retrospect, I'd probably use APR_PROGRAM_INH_ENV or something to
indicate inheritence. APR_PROGRAM_ENV might indicate "use the environment
provided" which is opposite of the actual semantic :-)
>...
> I'm still with Brian, I think this is overkill. I also agree with you, we
> need to pay attention to the docs :) But the only think you can argue was
> passing NULL for env was undefined. Its use-definition wasn't a bad one.
Not overkill, but to prevent illegal combinations. And detecting those
combinations at cmdtype_set type (rather than basing on 'env' at create
time) is still valid.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Greg Stein" <gs...@lyra.org>
Sent: Monday, January 28, 2002 9:50 PM
> On Mon, Jan 28, 2002 at 08:52:03PM -0600, William A. Rowe, Jr. wrote:
> > From: "Brian Havard" <br...@kheldar.apana.org.au>
> > Sent: Monday, January 28, 2002 9:33 PM
> > > On 28 Jan 2002 21:58:16 -0000, gstein@apache.org wrote:
> >...
> > > > Add a couple new command types to process creation:
> > > >
> > > > APR_PROGRAM_ENV: start the program using the caller's environment
> > > > APR_PROGRAM_PATH: search PATH for the program, use caller's env
> >...
> > > Wouldn't it be simpler to just have env==NULL imply using the parent's
> > > environment? That is in fact already the case in the OS/2 implementation
> > > (and Win32 by the looks of it).
> >
> > + 1 ... this patch [appeared] overly complex in the first place.
> > With an env value passes that array of envvars. Without simply passes
> > the current environment.
>
> It wouldn't really be simpler. If somebody accidentally passes NULL for the
> env and it copies the environment down... oops. Security issue. In Apache,
> we've taken some pains to limit the environment passed down to children.
Yes, and we are explicit, we pass a char**
> But it would also open up a combination that isn't possible --
> APR_PROGRAM_PATH with env==[...]. In words that is, "search for the program
> on PATH, and here is the environment to use." That combination isn't
> available on Unix, at least.
Can we say APR_ENOTIMPL? Seriously, Win32 always searches the program path
if the executable (1st arg to CreateProcess) is NULL. It executes a very
specific program if the 1st arg is given. This part of your new schema is
great goodness.
But If I'm Not Mistaken [Read: I'm probably mistaken], exec() is essentially
a fork() -> load process -> run sequence [please correct me.] There is nothing
keeping us from replacing the environment after we've done the fork(), is there?
Yes - we all presume that the prior v.s. new PATH value is indeterminate.
> The enumerated type gives us the ability to specify the exact combinations
> that are allowed to callers.
Ugh. It is not exactly quaint, but I begin to see some value here. Still,
this just makes more sense as flags, if you ask me [who was asking :-?] EVEN
if we treat it as flags, but only allow specific combinations. At least we
can test them by bit to find specific logic rules.
> When I went in there and saw some sort of interpretation of env==NULL, I was
> pretty surprised confused. That behavior certainly was not documented in the
> header file. Having a little automagic copying of the environment isn't
> necessarily a good thing in the apache world. IMO, that env==NULL behavior
> should be axed and the inherit of the env should be made explicit.
If you don't pass env, you don't get env. What's confusing about that?
Since it's a double-indirection, even an _EMPTY_ env would be a pointer to
a NULL pointer. Not difficult to parse.
I'm still with Brian, I think this is overkill. I also agree with you, we
need to pay attention to the docs :) But the only think you can argue was
passing NULL for env was undefined. Its use-definition wasn't a bad one.
Bill
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by Brian Havard <br...@kheldar.apana.org.au>.
On Mon, 28 Jan 2002 19:50:19 -0800, Greg Stein wrote:
>On Mon, Jan 28, 2002 at 08:52:03PM -0600, William A. Rowe, Jr. wrote:
>> From: "Brian Havard" <br...@kheldar.apana.org.au>
>> Sent: Monday, January 28, 2002 9:33 PM
>> > On 28 Jan 2002 21:58:16 -0000, gstein@apache.org wrote:
>>...
>> > > Add a couple new command types to process creation:
>> > >
>> > > APR_PROGRAM_ENV: start the program using the caller's environment
>> > > APR_PROGRAM_PATH: search PATH for the program, use caller's env
>>...
>> > Wouldn't it be simpler to just have env==NULL imply using the parent's
>> > environment? That is in fact already the case in the OS/2 implementation
>> > (and Win32 by the looks of it).
>>
>> + 1 ... this patch [appeared] overly complex in the first place.
>> With an env value passes that array of envvars. Without simply passes
>> the current environment.
>
>It wouldn't really be simpler. If somebody accidentally passes NULL for the
>env and it copies the environment down... oops. Security issue. In Apache,
>we've taken some pains to limit the environment passed down to children.
>
>But it would also open up a combination that isn't possible --
>APR_PROGRAM_PATH with env==[...]. In words that is, "search for the program
>on PATH, and here is the environment to use." That combination isn't
>available on Unix, at least.
Well, other platforms CAN provide it and I'd guess you could do it on unix
too if you really tried (do the path search yourself perhaps?). The
non-unix platforms sometimes have to jump through hoops to implement
behaviour that is standard on unix (usually because the API was designed by
a unixhead :) so why shouldn't the reverse be true sometimes?
--
______________________________________________________________________________
| Brian Havard | "He is not the messiah! |
| brianh@kheldar.apana.org.au | He's a very naughty boy!" - Life of Brian |
------------------------------------------------------------------------------
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Jan 28, 2002 at 08:52:03PM -0600, William A. Rowe, Jr. wrote:
> From: "Brian Havard" <br...@kheldar.apana.org.au>
> Sent: Monday, January 28, 2002 9:33 PM
> > On 28 Jan 2002 21:58:16 -0000, gstein@apache.org wrote:
>...
> > > Add a couple new command types to process creation:
> > >
> > > APR_PROGRAM_ENV: start the program using the caller's environment
> > > APR_PROGRAM_PATH: search PATH for the program, use caller's env
>...
> > Wouldn't it be simpler to just have env==NULL imply using the parent's
> > environment? That is in fact already the case in the OS/2 implementation
> > (and Win32 by the looks of it).
>
> + 1 ... this patch [appeared] overly complex in the first place.
> With an env value passes that array of envvars. Without simply passes
> the current environment.
It wouldn't really be simpler. If somebody accidentally passes NULL for the
env and it copies the environment down... oops. Security issue. In Apache,
we've taken some pains to limit the environment passed down to children.
But it would also open up a combination that isn't possible --
APR_PROGRAM_PATH with env==[...]. In words that is, "search for the program
on PATH, and here is the environment to use." That combination isn't
available on Unix, at least.
The enumerated type gives us the ability to specify the exact combinations
that are allowed to callers.
When I went in there and saw some sort of interpretation of env==NULL, I was
pretty surprised confused. That behavior certainly was not documented in the
header file. Having a little automagic copying of the environment isn't
necessarily a good thing in the apache world. IMO, that env==NULL behavior
should be axed and the inherit of the env should be made explicit.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by "William A. Rowe, Jr." <wr...@covalent.net>.
From: "Brian Havard" <br...@kheldar.apana.org.au>
Sent: Monday, January 28, 2002 9:33 PM
> On 28 Jan 2002 21:58:16 -0000, gstein@apache.org wrote:
>
> >gstein 02/01/28 13:58:16
> >
> > Modified: include apr_thread_proc.h
> > threadproc/beos proc.c
> > threadproc/netware proc.c
> > threadproc/os2 proc.c
> > threadproc/unix proc.c
> > threadproc/win32 proc.c
> > Log:
> > Add a couple new command types to process creation:
> >
> > APR_PROGRAM_ENV: start the program using the caller's environment
> > APR_PROGRAM_PATH: search PATH for the program, use caller's env
> >
> > (the normal APR_PROGRAM isolates the env and does not use PATH)
> >
> > The BeOS, OS/2, and Win32 implementations are incomplete. These two
> > new forms just default back to APR_PROGRAM for now. (although BeOS
> > doesn't even distinguish between APR_SHELLCMD and APR_PROGRAM!)
> >
> > On Unix and Netware, these map into execv() and execvp() calls.
>
> Wouldn't it be simpler to just have env==NULL imply using the parent's
> environment? That is in fact already the case in the OS/2 implementation
> (and Win32 by the looks of it).
+ 1 ... this patch [appeared] overly complex in the first place.
With an env value passes that array of envvars. Without simply passes
the current environment.
Bill
Re: cvs commit: apr/threadproc/win32 proc.c
Posted by Brian Havard <br...@kheldar.apana.org.au>.
On 28 Jan 2002 21:58:16 -0000, gstein@apache.org wrote:
>gstein 02/01/28 13:58:16
>
> Modified: include apr_thread_proc.h
> threadproc/beos proc.c
> threadproc/netware proc.c
> threadproc/os2 proc.c
> threadproc/unix proc.c
> threadproc/win32 proc.c
> Log:
> Add a couple new command types to process creation:
>
> APR_PROGRAM_ENV: start the program using the caller's environment
> APR_PROGRAM_PATH: search PATH for the program, use caller's env
>
> (the normal APR_PROGRAM isolates the env and does not use PATH)
>
> The BeOS, OS/2, and Win32 implementations are incomplete. These two
> new forms just default back to APR_PROGRAM for now. (although BeOS
> doesn't even distinguish between APR_SHELLCMD and APR_PROGRAM!)
>
> On Unix and Netware, these map into execv() and execvp() calls.
Wouldn't it be simpler to just have env==NULL imply using the parent's
environment? That is in fact already the case in the OS/2 implementation
(and Win32 by the looks of it).
--
______________________________________________________________________________
| Brian Havard | "He is not the messiah! |
| brianh@kheldar.apana.org.au | He's a very naughty boy!" - Life of Brian |
------------------------------------------------------------------------------