code review 148370043: os, syscall: add Unsetenv

os, syscall: add Unsetenv

Also address a TODO, making Clearenv pass through to cgo.

Based largely on Minux's earlier https://codereview.appspot.com/82040044

Fixes Issue 6423

[bradfitz, 2014-09-30 21:46:21]

Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com, iant@golang.org,
r@golang.org),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2014-09-30 21:47:40]

Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com, iant@golang.org,
r@golang.org),

Please take another look.

[bradfitz, 2014-09-30 21:50:00]

https://codereview.appspot.com/148370043/diff/60001/src/runtime/env_posix.go
File src/runtime/env_posix.go (right):

https://codereview.appspot.com/148370043/diff/60001/src/runtime/env_posix.go#...
src/runtime/env_posix.go:50: func syscall_unsetenv_c(k string, v string) {
whoops, unnecessary v. removing.

[bradfitz, 2014-09-30 21:50:45]

Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com, iant@golang.org,
r@golang.org),

Please take another look.

[iant, 2014-09-30 22:04:32]

LGTM

But see if anybody else has an opinion on this approach.

[r, 2014-09-30 22:08:30]

i don't necessarily consider this change to fall under the syscall lockdown
since it's os that's getting the functionality and we did say that syscall would
always support the main repo. and the functionality is useful in all
environments. 

the lockdown is more about stopping new system calls coming in.

discuss.

https://codereview.appspot.com/148370043/diff/70001/src/os/env.go
File src/os/env.go (right):

https://codereview.appspot.com/148370043/diff/70001/src/os/env.go#newcode97
src/os/env.go:97: // UnsetEnv unsets a single environment variable.
what's the point of the error return?

[rsc, 2014-09-30 22:13:56]

syscall is allowed to change to support package os and other standard packages.
I would just add Unsetenv to syscall too. We don't need a new package for this.

https://codereview.appspot.com/148370043/diff/70001/src/os/env.go
File src/os/env.go (right):

https://codereview.appspot.com/148370043/diff/70001/src/os/env.go#newcode97
src/os/env.go:97: // UnsetEnv unsets a single environment variable.
I agree.
Also: s/Env/env/; look at the surrounding functions.

[brainman, 2014-09-30 23:26:05]

Windows code looks good. But windows syscall.unsetenv should return error to the
user.

Alex

[bradfitz, 2014-10-01 01:04:17]

Hello rsc@golang.org, iant@golang.org, r@golang.org, alex.brainman@gmail.com
(cc: golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-10-01 01:04:25]

PTAL, now without new package, and comments addressed.


On Tue, Sep 30, 2014 at 4:26 PM, <alex.brainman@gmail.com> wrote:

> Windows code looks good. But windows syscall.unsetenv should return
> error to the user.
>
> Alex
>
> https://codereview.appspot.com/148370043/
>

[bradfitz, 2014-10-01 01:07:33]

https://codereview.appspot.com/148370043/diff/70001/src/os/env.go
File src/os/env.go (right):

https://codereview.appspot.com/148370043/diff/70001/src/os/env.go#newcode97
src/os/env.go:97: // UnsetEnv unsets a single environment variable.
On 2014/09/30 22:08:30, r wrote:
> what's the point of the error return?

It matches Setenv. Both can fail. (especially on Windows)

[brainman, 2014-10-01 02:00:24]

LGTM

Alex

[r, 2014-10-01 02:13:03]

https://codereview.appspot.com/148370043/diff/90001/src/os/env_test.go
File src/os/env_test.go (right):

https://codereview.appspot.com/148370043/diff/90001/src/os/env_test.go#newcode73
src/os/env_test.go:73: func TestUnsetEnv(t *testing.T) {
TestUnsetenv

https://codereview.appspot.com/148370043/diff/90001/src/os/env_test.go#newcode74
src/os/env_test.go:74: const testKey = "TestUnsetEnv"
ditto

https://codereview.appspot.com/148370043/diff/90001/src/os/env_test.go#newcode83
src/os/env_test.go:83: Setenv(testKey, "1")
check error

https://codereview.appspot.com/148370043/diff/90001/src/os/env_test.go#newcode87
src/os/env_test.go:87: Unsetenv(testKey)
check error

[bradfitz, 2014-10-01 02:37:19]

Hello rsc@golang.org, iant@golang.org, r@golang.org, alex.brainman@gmail.com
(cc: golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-10-01 02:37:31]

PTAL done.
 On Sep 30, 2014 7:13 PM, <r@golang.org> wrote:

>
> https://codereview.appspot.com/148370043/diff/90001/src/os/env_test.go
> File src/os/env_test.go (right):
>
> https://codereview.appspot.com/148370043/diff/90001/src/
> os/env_test.go#newcode73
> src/os/env_test.go:73: func TestUnsetEnv(t *testing.T) {
> TestUnsetenv
>
> https://codereview.appspot.com/148370043/diff/90001/src/
> os/env_test.go#newcode74
> src/os/env_test.go:74: const testKey = "TestUnsetEnv"
> ditto
>
> https://codereview.appspot.com/148370043/diff/90001/src/
> os/env_test.go#newcode83
> src/os/env_test.go:83: Setenv(testKey, "1")
> check error
>
> https://codereview.appspot.com/148370043/diff/90001/src/
> os/env_test.go#newcode87
> src/os/env_test.go:87: Unsetenv(testKey)
> check error
>
> https://codereview.appspot.com/148370043/
>

[rsc, 2014-10-01 02:47:45]

https://codereview.appspot.com/148370043/diff/110001/src/syscall/env_unix.go
File src/syscall/env_unix.go (right):

https://codereview.appspot.com/148370043/diff/110001/src/syscall/env_unix.go#...
src/syscall/env_unix.go:53: envLock.RLock()
should be Lock/Unlock not RLock/RUnlock

https://codereview.appspot.com/148370043/diff/110001/src/syscall/env_unix.go#...
src/syscall/env_unix.go:58: envs = append(envs[:i], envs[i+1:]...)
This seems too expensive (especially the map recreation below).
Can we do

envs[i] = ""

and drop the copyenv, and then in Environ we have to filter out the empty
strings when preparing the result, but that's cheap.

[r, 2014-10-01 02:48:34]

LGTM

[bradfitz, 2014-10-01 02:52:58]

Hello rsc@golang.org, iant@golang.org, r@golang.org, alex.brainman@gmail.com
(cc: golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-10-01 02:53:21]

On Tue, Sep 30, 2014 at 7:47 PM, <rsc@golang.org> wrote:

>
> https://codereview.appspot.com/148370043/diff/110001/src/
> syscall/env_unix.go
> File src/syscall/env_unix.go (right):
>
> https://codereview.appspot.com/148370043/diff/110001/src/
> syscall/env_unix.go#newcode53
> src/syscall/env_unix.go:53: envLock.RLock()
> should be Lock/Unlock not RLock/RUnlock
>
> https://codereview.appspot.com/148370043/diff/110001/src/
> syscall/env_unix.go#newcode58
> src/syscall/env_unix.go:58: envs = append(envs[:i], envs[i+1:]...)
> This seems too expensive (especially the map recreation below).
> Can we do
>
> envs[i] = ""
>
> and drop the copyenv, and then in Environ we have to filter out the
> empty strings when preparing the result, but that's cheap.
>

Done.

Do we care about making sure we filter out all environments with that key
if there are dups?

[bradfitz, 2014-10-01 03:34:42]

Hello rsc@golang.org, iant@golang.org, r@golang.org, alex.brainman@gmail.com
(cc: golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-10-01 03:37:12]

PTAL now zeroing out all occurrences if multiple.

Seems like a potential security issue to not.


On Tue, Sep 30, 2014 at 8:34 PM, <bradfitz@golang.org> wrote:

> Hello rsc@golang.org, iant@golang.org, r@golang.org,
> alex.brainman@gmail.com (cc: golang-codereviews@googlegroups.com),
>
> Please take another look.
>
>
> https://codereview.appspot.com/148370043/
>

[rsc, 2014-10-01 13:40:06]

LGTM with tweak

https://codereview.appspot.com/148370043/diff/150001/src/syscall/env_unix.go
File src/syscall/env_unix.go (right):

https://codereview.appspot.com/148370043/diff/150001/src/syscall/env_unix.go#...
src/syscall/env_unix.go:58: for i, pair := range envs {
This is definitely better, but I'm still worried about someone looping over a
large environment calling Unsetenv, and that being quadratic because Unsetenv is
linear.

We could avoid this loop here if above in copyenv we do

if _, ok := env[key]; ok {
    env[key] = i // first mention of key
} else {
    envs[i] = "" // clear second mention of key
}

[bradfitz, 2014-10-01 18:17:14]

*** Submitted as https://code.google.com/p/go/source/detail?r=5cf5d1f289a7 ***

os, syscall: add Unsetenv

Also address a TODO, making Clearenv pass through to cgo.

Based largely on Minux's earlier https://codereview.appspot.com/82040044

Fixes Issue 6423

LGTM=iant, alex.brainman, r, rsc
R=rsc, iant, r, alex.brainman
CC=golang-codereviews
https://codereview.appspot.com/148370043

[gobot, 2014-10-01 18:20:45]

This CL appears to have broken the plan9-amd64-aram builder.
See http://build.golang.org/log/cb69d52beb7a3eef9b4c7c2919a49e262a766dbc

[r, 2014-10-01 18:21:18]

Can you please apply the relevant piece to go.sys or tell me to do so?

-rob


On Wed, Oct 1, 2014 at 11:17 AM,  <bradfitz@golang.org> wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=5cf5d1f289a7 ***
>
> os, syscall: add Unsetenv
>
> Also address a TODO, making Clearenv pass through to cgo.
>
> Based largely on Minux's earlier https://codereview.appspot.com/82040044
>
> Fixes Issue 6423
>
> LGTM=iant, alex.brainman, r, rsc
> R=rsc, iant, r, alex.brainman
> CC=golang-codereviews
> https://codereview.appspot.com/148370043
>
>
> https://codereview.appspot.com/148370043/

[bradfitz, 2014-10-01 18:41:11]

Rob, https://codereview.appspot.com/153810043


On Wed, Oct 1, 2014 at 11:20 AM, Rob Pike <r@golang.org> wrote:

> Can you please apply the relevant piece to go.sys or tell me to do so?
>
> -rob
>
>
> On Wed, Oct 1, 2014 at 11:17 AM,  <bradfitz@golang.org> wrote:
> > *** Submitted as
> > https://code.google.com/p/go/source/detail?r=5cf5d1f289a7 ***
> >
> > os, syscall: add Unsetenv
> >
> > Also address a TODO, making Clearenv pass through to cgo.
> >
> > Based largely on Minux's earlier https://codereview.appspot.com/82040044
> >
> > Fixes Issue 6423
> >
> > LGTM=iant, alex.brainman, r, rsc
> > R=rsc, iant, r, alex.brainman
> > CC=golang-codereviews
> > https://codereview.appspot.com/148370043
> >
> >
> > https://codereview.appspot.com/148370043/
>