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

Issue 57890043: code review 57890043: syscall: fix leakage of file descriptors after a fork o...

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 11 months ago by ality
Modified:
6 years, 2 months ago
CC:
jas
Visibility:
Public.

Description

syscall: fix leakage of file descriptors after a fork on Plan 9 Instead of trying to emulate the racy method used on Unix, we simply read the list of open file descriptors from the dup(3) device once we're inside the child and close everything that we don't want to keep. This change was inspired by similar code in the 9front version of page(1). Also remove all uses of the ForkLock since it's unneccesary. Fixes issue 7118. Fixes issue 5605.

Patch Set 1 #

Patch Set 2 : diff -r fbcb2b86201e https://code.google.com/p/go/ #

Total comments: 1

Patch Set 3 : diff -r 9c65fe4ce5a2 https://code.google.com/p/go/ #

Patch Set 4 : diff -r 1fda0c956e3f https://code.google.com/p/go/ #

Patch Set 5 : diff -r 1fda0c956e3f https://code.google.com/p/go/ #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+113 lines, -227 lines) Patch
M src/pkg/net/fd_plan9.go View 1 1 chunk +0 lines, -2 lines 0 comments Download
M src/pkg/net/file_plan9.go View 1 1 chunk +0 lines, -2 lines 0 comments Download
M src/pkg/os/exec/exec_test.go View 1 2 3 2 chunks +15 lines, -7 lines 0 comments Download
M src/pkg/os/file_plan9.go View 1 2 chunks +0 lines, -5 lines 0 comments Download
M src/pkg/syscall/exec_plan9.go View 1 2 3 4 7 chunks +98 lines, -211 lines 2 comments Download

Messages

Total messages: 13
ality
Hello golang-codereviews@googlegroups.com (cc: 0intro@gmail.com, jas@corpus-callosum.com), I'd like you to review this change to https://code.google.com/p/go/
6 years, 11 months ago (2014-01-28 20:38:31 UTC) #1
0intro
Thanks. It looks pretty interesting. I'm occasionally seeing some failures when running the tests here. ...
6 years, 11 months ago (2014-01-29 05:39:19 UTC) #2
bradfitz
That's a lot of deleted lines. :) https://codereview.appspot.com/57890043/diff/20001/src/pkg/syscall/exec_plan9.go File src/pkg/syscall/exec_plan9.go (right): https://codereview.appspot.com/57890043/diff/20001/src/pkg/syscall/exec_plan9.go#newcode140 src/pkg/syscall/exec_plan9.go:140: b := ...
6 years, 11 months ago (2014-01-29 09:10:57 UTC) #3
ality
David du Colombier <0intro@gmail.com> once said: > Thanks. It looks pretty interesting. > > I'm ...
6 years, 11 months ago (2014-01-29 10:28:15 UTC) #4
0intro
os.test 29015: suicide: sys: trap: fault read addr=0x36 pc=0x0002226c --- FAIL: TestKillStartProcess (1.60 seconds) os_test.go:1270: ...
6 years, 11 months ago (2014-01-29 20:44:23 UTC) #5
rminnich
What I like about this is you're really using Plan 9 to effect. Seems it ...
6 years, 11 months ago (2014-01-31 14:55:45 UTC) #6
rsc
Removing ForkLock is fine but the new ForkExec is buggy. https://codereview.appspot.com/57890043/diff/80001/src/pkg/syscall/exec_plan9.go File src/pkg/syscall/exec_plan9.go (left): https://codereview.appspot.com/57890043/diff/80001/src/pkg/syscall/exec_plan9.go#oldcode186 ...
6 years, 11 months ago (2014-02-13 15:25:52 UTC) #7
rsc
R=close hg mail after 1.3.
6 years, 9 months ago (2014-04-17 02:49:43 UTC) #8
bradfitz
Is somebody going to pick this up again, now that 1.3 is out?
6 years, 3 months ago (2014-10-02 18:51:46 UTC) #9
lucio
https://codereview.appspot.com/57890043/diff/80001/src/pkg/syscall/exec_plan9.go File src/pkg/syscall/exec_plan9.go (left): https://codereview.appspot.com/57890043/diff/80001/src/pkg/syscall/exec_plan9.go#oldcode186 src/pkg/syscall/exec_plan9.go:186: // Guard against side effects of shuffling fds below. ...
6 years, 3 months ago (2014-10-03 04:43:51 UTC) #10
mischief
ping on this. hopefully this can solve the flaky builder problem with plan9/amd64.
6 years, 2 months ago (2014-10-29 19:53:46 UTC) #11
lucio
The last two comments (a trivial one by me, but the one from Russ seems ...
6 years, 2 months ago (2014-10-30 03:57:56 UTC) #12
lucio
6 years, 2 months ago (2014-10-30 03:58:10 UTC) #13
On 10/30/14, Lucio De Re <lucio.dere@gmail.com> wrote:
> The last two comments (a trivial one by me, but the one from Russ
> seems quite relevant) haven't been addressed yet.  I don't think this
> will pass...
>
> Lucio.
>
>
> On 10/29/14, mischief@offblast.org <mischief@offblast.org> wrote:
>> ping on this. hopefully this can solve the flaky builder problem with
>> plan9/amd64.
>>
>> https://codereview.appspot.com/57890043/
>>
>
Sign in to reply to this message.

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