Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Message-ID: <01ba01c1a886$bee669e0$94c0b0d0@v505>
From: "William A. Rowe, Jr." <wrowe@covalent.net>
To: "Greg Stein" <gstein@lyra.org>,
	<dev@apr.apache.org>
References: <E16VO5P-00025g-00@zakath.apana.org.au>
 <012801c1a870$c73ecdf0$94c0b0d0@v505> <20020128195019.N11050@lyra.org>
Subject: Re: cvs commit: apr/threadproc/win32 proc.c
Date: Mon, 28 Jan 2002 23:35:20 -0600
MIME-Version: 1.0
Content-Type: text/plain;
	charset="iso-8859-1"
Content-Transfer-Encoding: 7bit

From: "Greg Stein" <gstein@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" <brianh@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