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

Issue 7392048: code review 7392048: os: make FindProcess do some error checking

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 6 months ago by akumar
Modified:
5 years, 9 months ago
Reviewers:
minux1, ality, ioe, rsc, dfc, rminnich, bradfitz
CC:
golang-dev
Visibility:
Public.

Description

os: make FindProcess do some error checking Makes FindProcess on UNIX and Plan 9 check that the process with the given pid is running and that we at least have permissions to send it signals. An equivalent check is already present in the Windows implementation.

Patch Set 1 #

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

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

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

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -8 lines) Patch
M src/pkg/net/http/cgi/host_test.go View 1 2 3 2 chunks +2 lines, -6 lines 0 comments Download
M src/pkg/os/exec_plan9.go View 1 2 3 1 chunk +3 lines, -1 line 0 comments Download
M src/pkg/os/exec_unix.go View 1 2 3 1 chunk +5 lines, -1 line 3 comments Download

Messages

Total messages: 21
akumar
Hello rsc@golang.org, rminnich@gmail.com, ality@pbrane.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go
6 years, 6 months ago (2013-02-22 23:21:46 UTC) #1
ality
I don't agree with this change. The syscall API is not required to be uniform ...
6 years, 6 months ago (2013-02-23 04:32:49 UTC) #2
dfc
I agree. Can we try that first ? On Sat, Feb 23, 2013 at 3:32 ...
6 years, 6 months ago (2013-02-23 04:34:32 UTC) #3
rminnich
On Fri, Feb 22, 2013 at 8:34 PM, Dave Cheney <dave@cheney.net> wrote: > I agree. ...
6 years, 6 months ago (2013-02-23 04:46:08 UTC) #4
dfc
I was echoing Anthony, "I'd prefer to go the other way. Remove any hint of ...
6 years, 6 months ago (2013-02-23 04:46:49 UTC) #5
rminnich
On Fri, Feb 22, 2013 at 8:46 PM, Dave Cheney <dave@cheney.net> wrote: > I was ...
6 years, 6 months ago (2013-02-23 04:50:06 UTC) #6
ality
Well, the net/http/cgi test is calling p.Signal with a syscall.Signal(0) which means no signal will ...
6 years, 6 months ago (2013-02-23 05:20:32 UTC) #7
rminnich
OK, we'll see about that, seems like a nice idea. On Fri, Feb 22, 2013 ...
6 years, 6 months ago (2013-02-23 07:10:52 UTC) #8
akumar
This is an update of the "syscall: Plan9: Define the Signal type" CL. PTAL. Also ...
6 years, 6 months ago (2013-02-23 14:02:52 UTC) #9
minux1
https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go File src/pkg/os/exec_unix.go (right): https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go#newcode59 src/pkg/os/exec_unix.go:59: func findProcess(pid int) (p *Process, err error) { this ...
6 years, 6 months ago (2013-02-23 14:13:14 UTC) #10
ality
minux.ma@gmail.com once said: > https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go#newcode59 > src/pkg/os/exec_unix.go:59: func findProcess(pid int) (p *Process, err > error) ...
6 years, 6 months ago (2013-02-23 14:21:41 UTC) #11
rminnich
https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go File src/pkg/os/exec_unix.go (right): https://codereview.appspot.com/7392048/diff/12001/src/pkg/os/exec_unix.go#newcode59 src/pkg/os/exec_unix.go:59: func findProcess(pid int) (p *Process, err error) { That ...
6 years, 6 months ago (2013-02-23 17:26:27 UTC) #12
bradfitz
On Fri, Feb 22, 2013 at 9:20 PM, Anthony Martin <ality@pbrane.org> wrote: > Well, the ...
6 years, 6 months ago (2013-02-23 17:46:29 UTC) #13
akumar
This CL is the implementation of the second option. The test has been changed. On ...
6 years, 6 months ago (2013-02-23 19:43:50 UTC) #14
bradfitz
I see both views here: a) FindProcess on unix never did anything before, and never ...
6 years, 6 months ago (2013-02-23 19:53:10 UTC) #15
akumar
Sorry, I sent the message below privately to Brad on accident - it was meant ...
6 years, 6 months ago (2013-02-23 20:16:15 UTC) #16
minux1
On Sun, Feb 24, 2013 at 4:15 AM, Akshat Kumar <seed@mail.nanosouffle.net>wrote: > I prefer (a) ...
6 years, 6 months ago (2013-02-23 21:05:23 UTC) #17
dfc
I think this solution is racy. wrt. the windows impl, it might racy as well, ...
6 years, 6 months ago (2013-02-24 01:34:56 UTC) #18
akumar
On 24 February 2013 01:34, <dave@cheney.net> wrote: > I think this solution is racy. wrt. ...
6 years, 6 months ago (2013-02-25 21:03:24 UTC) #19
ioe
On 2013/02/23 21:05:23, minux wrote: > right, the docs says "running process", however, that doesn't ...
6 years, 5 months ago (2013-04-20 23:23:03 UTC) #20
rsc
5 years, 9 months ago (2013-12-18 18:27:34 UTC) #21
R=close (timed out)
Sign in to reply to this message.

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