Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(615)

Issue 148370043: code review 148370043: os, syscall: add Unsetenv (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 6 months ago by bradfitz
Modified:
9 years, 6 months ago
Reviewers:
r, gobot, brainman, rsc, iant
CC:
rsc, iant, r, brainman, golang-codereviews
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : diff -r 61525d46311930992512297749c1ad914912041b https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 61525d46311930992512297749c1ad914912041b https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 61525d46311930992512297749c1ad914912041b https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 5 : diff -r 61525d46311930992512297749c1ad914912041b https://go.googlecode.com/hg/ #

Total comments: 3

Patch Set 6 : diff -r 61525d46311930992512297749c1ad914912041b https://go.googlecode.com/hg/ #

Total comments: 4

Patch Set 7 : diff -r 61525d46311930992512297749c1ad914912041b https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 8 : diff -r 61525d46311930992512297749c1ad914912041b https://go.googlecode.com/hg/ #

Patch Set 9 : diff -r 61525d46311930992512297749c1ad914912041b https://go.googlecode.com/hg/ #

Total comments: 1

Patch Set 10 : diff -r 61525d46311930992512297749c1ad914912041b https://go.googlecode.com/hg/ #

Patch Set 11 : diff -r 3042795874453efa7be0a082f3f85118b9d06432 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+135 lines, -12 lines) Patch
M src/os/env.go View 1 2 3 4 5 1 chunk +5 lines, -0 lines 0 comments Download
M src/os/env_test.go View 1 2 3 4 5 6 7 8 9 2 chunks +26 lines, -0 lines 0 comments Download
M src/runtime/cgo/gcc_setenv.c View 1 1 chunk +7 lines, -0 lines 0 comments Download
M src/runtime/cgo/setenv.c View 1 1 chunk +3 lines, -0 lines 0 comments Download
M src/runtime/env_posix.go View 1 2 3 4 2 chunks +12 lines, -1 line 0 comments Download
M src/runtime/thunk.s View 1 1 chunk +3 lines, -0 lines 0 comments Download
M src/syscall/env_plan9.go View 1 2 3 4 5 6 7 8 9 3 chunks +35 lines, -3 lines 0 comments Download
M src/syscall/env_unix.go View 1 2 3 4 5 6 7 8 9 4 chunks +36 lines, -8 lines 0 comments Download
M src/syscall/env_windows.go View 1 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 26
bradfitz
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/
9 years, 6 months ago (2014-09-30 21:46:21 UTC) #1
bradfitz
Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com, iant@golang.org, r@golang.org), Please take another look.
9 years, 6 months ago (2014-09-30 21:47:40 UTC) #2
bradfitz
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#newcode50 src/runtime/env_posix.go:50: func syscall_unsetenv_c(k string, v string) { whoops, unnecessary v. ...
9 years, 6 months ago (2014-09-30 21:50:00 UTC) #3
bradfitz
Hello rsc@golang.org (cc: golang-codereviews@googlegroups.com, iant@golang.org, r@golang.org), Please take another look.
9 years, 6 months ago (2014-09-30 21:50:45 UTC) #4
iant
LGTM But see if anybody else has an opinion on this approach.
9 years, 6 months ago (2014-09-30 22:04:32 UTC) #5
r
i don't necessarily consider this change to fall under the syscall lockdown since it's os ...
9 years, 6 months ago (2014-09-30 22:08:30 UTC) #6
rsc
syscall is allowed to change to support package os and other standard packages. I would ...
9 years, 6 months ago (2014-09-30 22:13:56 UTC) #7
brainman
Windows code looks good. But windows syscall.unsetenv should return error to the user. Alex
9 years, 6 months ago (2014-09-30 23:26:05 UTC) #8
bradfitz
Hello rsc@golang.org, iant@golang.org, r@golang.org, alex.brainman@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
9 years, 6 months ago (2014-10-01 01:04:17 UTC) #9
bradfitz
PTAL, now without new package, and comments addressed. On Tue, Sep 30, 2014 at 4:26 ...
9 years, 6 months ago (2014-10-01 01:04:25 UTC) #10
bradfitz
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 ...
9 years, 6 months ago (2014-10-01 01:07:33 UTC) #11
brainman
LGTM Alex
9 years, 6 months ago (2014-10-01 02:00:24 UTC) #12
r
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 ...
9 years, 6 months ago (2014-10-01 02:13:03 UTC) #13
bradfitz
Hello rsc@golang.org, iant@golang.org, r@golang.org, alex.brainman@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
9 years, 6 months ago (2014-10-01 02:37:19 UTC) #14
bradfitz
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 ...
9 years, 6 months ago (2014-10-01 02:37:31 UTC) #15
rsc
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 ...
9 years, 6 months ago (2014-10-01 02:47:45 UTC) #16
r
LGTM
9 years, 6 months ago (2014-10-01 02:48:34 UTC) #17
bradfitz
Hello rsc@golang.org, iant@golang.org, r@golang.org, alex.brainman@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
9 years, 6 months ago (2014-10-01 02:52:58 UTC) #18
bradfitz
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 ...
9 years, 6 months ago (2014-10-01 02:53:21 UTC) #19
bradfitz
Hello rsc@golang.org, iant@golang.org, r@golang.org, alex.brainman@gmail.com (cc: golang-codereviews@googlegroups.com), Please take another look.
9 years, 6 months ago (2014-10-01 03:34:42 UTC) #20
bradfitz
PTAL now zeroing out all occurrences if multiple. Seems like a potential security issue to ...
9 years, 6 months ago (2014-10-01 03:37:12 UTC) #21
rsc
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#newcode58 src/syscall/env_unix.go:58: for i, pair := range envs ...
9 years, 6 months ago (2014-10-01 13:40:06 UTC) #22
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=5cf5d1f289a7 *** os, syscall: add Unsetenv Also address a TODO, making Clearenv ...
9 years, 6 months ago (2014-10-01 18:17:14 UTC) #23
gobot
This CL appears to have broken the plan9-amd64-aram builder. See http://build.golang.org/log/cb69d52beb7a3eef9b4c7c2919a49e262a766dbc
9 years, 6 months ago (2014-10-01 18:20:45 UTC) #24
r
Can you please apply the relevant piece to go.sys or tell me to do so? ...
9 years, 6 months ago (2014-10-01 18:21:18 UTC) #25
bradfitz
9 years, 6 months ago (2014-10-01 18:41:11 UTC) #26
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/
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b