code review 3787046: runtime: revert 6974:1f3c3696babb

runtime: revert 6974:1f3c3696babb

I missed that environment is used during runtime setup,
well before go init() functions run. Implemented os-dependent
runtime.goenvs functions to allow for different unix, plan9 and
windows versions of environment discovery.

[brainman, 2011-01-05 00:05:10]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[brainman, 2011-01-05 00:05:44]

Sorry, Russ, I'm back to it again, but it was wishful thinking on my part <g>.
But paulzhol brought me back to earth:

"... I'm not sure adding os.init() is the right solution because
runtime·getenv is called very early, in runtime·schedinit for exmple. ..."

Alex

[paulzhol, 2011-01-05 21:35:32]

On 2011/01/05 00:05:44, brainman wrote:
> Sorry, Russ, I'm back to it again, but it was wishful thinking on my part <g>.
> But paulzhol brought me back to earth:
> 
> "... I'm not sure adding os.init() is the right solution because
> runtime·getenv is called very early, in runtime·schedinit for exmple. ..."
> 
> Alex

Hi,

Maybe you don't have to revert your changes - runtime·getenv is called very few
times (getting GOMAXPROCS and some other go exports). 

I was planning on just adding the equivalent to GetEnvironmentVariable in Plan 9
by calling a few open-read-close syscalls directly from runtime. Maybe you can
do the same.

Pavel

[paulzhol, 2011-01-05 21:44:36]

from runtime·getenv that is.

[brainman, 2011-01-06 06:01:17]

On 2011/01/05 21:35:32, paulzhol wrote:
> Maybe you don't have to revert your changes - runtime·getenv is called very
few
> times (getting GOMAXPROCS and some other go exports). 

Does it matter how many times getenv is called? Even if it is called once, I
still have to provide correct data, therefor I must implement all env fetching
right in runtime. Maybe I'm missing some point here?

> I was planning on just adding the equivalent to GetEnvironmentVariable in Plan
9
> by calling a few open-read-close syscalls directly from runtime. Maybe you can
> do the same.

That is what this CL is doing - implements env fetching inside runtime package.
While all that code is equivalent to 1 line of Go code in os, still I can't
remove it.

Alex

[paulzhol, 2011-01-06 10:49:07]

All I was saying is that because runtime.getenv is called a few times,
you don't need to setup os.Envs from runtime, if you can already do
that in your os.init.
You can just call GetEnvironmentVariable from runtime.getenv to get a
single variable.

Pavel
On Thursday, January 6, 2011,  <alex.brainman@gmail.com> wrote:
> On 2011/01/05 21:35:32, paulzhol wrote:
>
> Maybe you don't have to revert your changes - runtime·getenv is called
>
> very few
>
> times (getting GOMAXPROCS and some other go exports).
>
>
> Does it matter how many times getenv is called? Even if it is called
> once, I still have to provide correct data, therefor I must implement
> all env fetching right in runtime. Maybe I'm missing some point here?
>
>
> I was planning on just adding the equivalent to GetEnvironmentVariable
>
> in Plan 9
>
> by calling a few open-read-close syscalls directly from runtime. Maybe
>
> you can
>
> do the same.
>
>
> That is what this CL is doing - implements env fetching inside runtime
> package. While all that code is equivalent to 1 line of Go code in os,
> still I can't remove it.
>
> Alex
>
> http://codereview.appspot.com/3787046/
>

[brainman, 2011-01-06 22:36:00]

On 2011/01/06 10:49:07, paulzhol wrote:
> ... You can just call GetEnvironmentVariable from runtime.getenv to get a
> single variable.
> 

I'm with you now. I will investigate the option.

Alex

[brainman, 2011-01-10 04:51:41]

On 2011/01/06 22:36:00, brainman wrote:
> On 2011/01/06 10:49:07, paulzhol wrote:
> > ... 
> 
> ... I will investigate the option.
> 

I tried to change the code as you've suggested - to have getenv call into os
directly, but I have a problem. I can allocate block of memory, read value of an
environment variable into it and return it to the caller. The problem I have,
the caller doesn't free memory returned by getenv at the moment, because all
that memory is stored in os.Env slice read-only and don't need to be managed. I
don't think it is good idea to change this model. If you have any suggestions.

Alex

[paulzhol, 2011-01-10 07:49:44]

On 2011/01/10 04:51:41, brainman wrote:
> I tried to change the code as you've suggested - to have getenv call into os
> directly, but I have a problem. I can allocate block of memory, read value of
an
> environment variable into it and return it to the caller. The problem I have,
> the caller doesn't free memory returned by getenv at the moment, because all
> that memory is stored in os.Env slice read-only and don't need to be managed.
I
> don't think it is good idea to change this model. If you have any suggestions.
> 
> Alex


Maybe a fixed size os.Envs can be allocated in by runtime.goargs (just need to
set envc to 10 for example):
genvv = runtime·malloc(envc*sizeof genvv[0]);

Each time you return a result from runtime.getenv you can store a copy on this
slice, increasing its length.

Then when your os.init does Envs = Environ(), the garbage collector will cleanup
the old runtime values ?

Pavel

[brainman, 2011-01-10 09:52:34]

On 2011/01/10 07:49:44, paulzhol wrote:
> Maybe a fixed size os.Envs can be allocated in by runtime.goargs (just need to
> set envc to 10 for example):
> genvv = runtime·malloc(envc*sizeof genvv[0]);
> 
> Each time you return a result from runtime.getenv you can store a copy on this
> slice, increasing its length.
> 

I was thinking about it, but then we should protect the update operation from
concurrent change. We might as well do what this CL does: insert all environment
variables at startup and then use them read-only. Why do you think the
alternative is better?

Alex

[paulzhol, 2011-01-10 10:19:20]

On 2011/01/10 09:52:34, brainman wrote:
 
> I was thinking about it, but then we should protect the update operation from
> concurrent change. We might as well do what this CL does: insert all
environment
> variables at startup and then use them read-only. Why do you think the
> alternative is better?
> 
> Alex

Do you need to worry about concurrent updates from runtime? I thought we are
dealing with bootstrap code to have some control off the runtime at a very early
stage.
I am also biased because for Plan 9 it is much easier for me to get a single
env. variable compared to getting all of them with their values. 

Pavel

[brainman, 2011-01-10 12:41:23]

On 2011/01/10 10:19:20, paulzhol wrote:
> Do you need to worry about concurrent updates from runtime? I thought we are
> dealing with bootstrap code to have some control off the runtime at a very
early
> stage.

I didn't look at the code in question, also who knows what happens in the
future. Should we restrict getenv to only work in single thread environment?

> I am also biased because for Plan 9 it is much easier for me to get a single
> env. variable compared to getting all of them with their values. 

I'm biased too: this CL is done, new one I need to start again and, while most
of the code should be similar, there will be some new utf8 to utf16 conversion
required for me to pass environment variable names from go runtime and into
Windows. I would rather keep what this CL is, but I'm quite happy to change if
you have some trouble fetching all environment variables.

Alex

[paulzhol, 2011-01-10 14:33:45]

On 2011/01/10 12:41:23, brainman wrote:
> On 2011/01/10 10:19:20, paulzhol wrote:
> > Do you need to worry about concurrent updates from runtime? I thought we are
> > dealing with bootstrap code to have some control off the runtime at a very
> early
> > stage.
> 
> I didn't look at the code in question, also who knows what happens in the
> future. Should we restrict getenv to only work in single thread environment?
> 
> > I am also biased because for Plan 9 it is much easier for me to get a single
> > env. variable compared to getting all of them with their values. 
> 
> I'm biased too: this CL is done, new one I need to start again and, while most
> of the code should be similar, there will be some new utf8 to utf16 conversion
> required for me to pass environment variable names from go runtime and into
> Windows. I would rather keep what this CL is, but I'm quite happy to change if
> you have some trouble fetching all environment variables.
> 
> Alex

It's a good CL, and I haven't written any code yet :-) so you should commit it.

Pavel

[rsc, 2011-01-11 16:23:56]

LGTM

http://codereview.appspot.com/3787046/diff/2001/src/pkg/runtime/plan9/thread.c
File src/pkg/runtime/plan9/thread.c (right):

http://codereview.appspot.com/3787046/diff/2001/src/pkg/runtime/plan9/thread....
src/pkg/runtime/plan9/thread.c:23: extern Slice os·Envs;
These are the default settings unless something
else initialized them, so I think this function body
can go away.

[brainman, 2011-01-12 00:48:22]

*** Submitted as http://code.google.com/p/go/source/detail?r=883639139011 ***

runtime: revert 6974:1f3c3696babb

I missed that environment is used during runtime setup,
well before go init() functions run. Implemented os-dependent
runtime.goenvs functions to allow for different unix, plan9 and
windows versions of environment discovery.

R=rsc, paulzhol
CC=golang-dev
http://codereview.appspot.com/3787046

http://codereview.appspot.com/3787046/diff/2001/src/pkg/runtime/plan9/thread.c
File src/pkg/runtime/plan9/thread.c (right):

http://codereview.appspot.com/3787046/diff/2001/src/pkg/runtime/plan9/thread....
src/pkg/runtime/plan9/thread.c:23: extern Slice os·Envs;
On 2011/01/11 16:23:56, rsc wrote:
> These are the default settings unless something
> else initialized them, so I think this function body
> can go away.

Done.

[brainman, 2011-01-12 00:48:33]

Thank you for review.