On Tue, May 3, 2011 at 7:51 PM, <alex.brainman@gmail.com> wrote: > http://codereview.appspot.com/4437091/diff/4001/src/pkg/os/exec_windows.go#newcode37 > src/pkg/os/exec_windows.go:37: // ...
13 years, 10 months ago
(2011-05-03 08:13:05 UTC)
#3
On Tue, May 3, 2011 at 7:51 PM, <alex.brainman@gmail.com> wrote:
>
http://codereview.appspot.com/4437091/diff/4001/src/pkg/os/exec_windows.go#ne...
> src/pkg/os/exec_windows.go:37: // Terminate requests that the Process
> exit. The Process may ignore this request.
> Your comment is misleading here. TerminateProcess can't be ignored on
> Windows. Process will terminate immediately. Not sure what to do about
> the comment.
The comment may be unclear, but I don't think it's inaccurate. Would
it be better if I change it to "It is possible that the Process will
ignore this request."?
>
http://codereview.appspot.com/4437091/diff/4001/src/pkg/syscall/syscall_windo...
> src/pkg/syscall/syscall_windows.go:140: //sys TerminateProcess(handle
> uint32, exitcode uint32) (errno int)
> s/handle uint32/handle int32/
> CreateProcess returns (handle int32), let's have (handle int32) here
> too.
I copied the uint32 from GetExitCodeProcess just below. Want me to
change that one too?
- Evan
On 2011/05/03 08:13:05, eds wrote: > > I copied the uint32 from GetExitCodeProcess just below. ...
13 years, 10 months ago
(2011-05-03 11:38:43 UTC)
#4
On 2011/05/03 08:13:05, eds wrote:
>
> I copied the uint32 from GetExitCodeProcess just below. Want me to
> change that one too?
>
Please do. Change OpenProcess while you're there too. Thank you.
Alex
http://codereview.appspot.com/4437091/diff/4001/src/pkg/os/exec_unix.go File src/pkg/os/exec_unix.go (right): http://codereview.appspot.com/4437091/diff/4001/src/pkg/os/exec_unix.go#newcode58 src/pkg/os/exec_unix.go:58: // action. i think this is a peculiar pair ...
13 years, 10 months ago
(2011-05-03 18:47:05 UTC)
#5
On Wed, May 4, 2011 at 6:47 AM, <r@golang.org> wrote: > i think this is ...
13 years, 10 months ago
(2011-05-03 20:37:21 UTC)
#6
On Wed, May 4, 2011 at 6:47 AM, <r@golang.org> wrote:
> i think this is a peculiar pair of functions. you're clearly echoing
> unix semantics, which makes sense, but if that's what's wanted, why not
> make it real unix semantics and just provide (perhaps an alias for) the
> signal type?
Signals really don't translate well to Windows. I think the only
signal it can reasonably emulate is SIGKILL. I don't feel like it's
worth having a Signal method that can only take two different signals,
but maybe others disagree. I'm also open to different names on the two
methods I have if that's an issue.
- Evan
On May 3, 2011, at 1:37 PM, Evan Shaw wrote: > On Wed, May 4, ...
13 years, 10 months ago
(2011-05-03 21:22:46 UTC)
#7
On May 3, 2011, at 1:37 PM, Evan Shaw wrote:
> On Wed, May 4, 2011 at 6:47 AM, <r@golang.org> wrote:
>> i think this is a peculiar pair of functions. you're clearly echoing
>> unix semantics, which makes sense, but if that's what's wanted, why not
>> make it real unix semantics and just provide (perhaps an alias for) the
>> signal type?
>
> Signals really don't translate well to Windows. I think the only
> signal it can reasonably emulate is SIGKILL. I don't feel like it's
> worth having a Signal method that can only take two different signals,
> but maybe others disagree. I'm also open to different names on the two
> methods I have if that's an issue.
I think it makes sense for there to be a Signal (or whatever it's called) to
provide access to Unix-like signalling. If Windows can only handle a subset,
that's a better situation that forcing Windows-only options onto Unix.
Or it could be argued that if you want Unix signals, use syscall, I suppose.
But I'm not happy with that.
-rob
On Wed, May 4, 2011 at 9:22 AM, Rob 'Commander' Pike <r@google.com> wrote: > I ...
13 years, 10 months ago
(2011-05-03 21:31:17 UTC)
#8
On Wed, May 4, 2011 at 9:22 AM, Rob 'Commander' Pike <r@google.com> wrote:
> I think it makes sense for there to be a Signal (or whatever it's called) to
provide access to Unix-like signalling. If Windows can only handle a subset,
that's a better situation that forcing Windows-only options onto Unix.
The more I think about it the more reasonable this sounds. Windows can
just return a "not supported" error for most signals.
- Evan
On May 3, 2011, at 2:31 PM, Evan Shaw wrote: > On Wed, May 4, ...
13 years, 10 months ago
(2011-05-03 21:32:01 UTC)
#9
On May 3, 2011, at 2:31 PM, Evan Shaw wrote:
> On Wed, May 4, 2011 at 9:22 AM, Rob 'Commander' Pike <r@google.com> wrote:
>> I think it makes sense for there to be a Signal (or whatever it's called) to
provide access to Unix-like signalling. If Windows can only handle a subset,
that's a better situation that forcing Windows-only options onto Unix.
>
> The more I think about it the more reasonable this sounds. Windows can
> just return a "not supported" error for most signals.
SGTM
On 2011/05/03 21:22:46, r2 wrote: > > I think it makes sense for there to ...
13 years, 10 months ago
(2011-05-03 23:27:18 UTC)
#11
On 2011/05/03 21:22:46, r2 wrote:
>
> I think it makes sense for there to be a Signal (or whatever it's called) to
> provide access to Unix-like signalling. If Windows can only handle a subset,
> that's a better situation that forcing Windows-only options onto Unix.
>
Sounds reasonable. But I think we should also have a function to terminate
process.
func (p *Process) Kill() Error
or
func (p *Process) Terminate() Error
I'm not fussy which name to use. Windows users have no concept of "sending
signal to process to terminate it", they just call appropriate Windows api
TerminateProcess. Unix version of this function will just send KILL to process.
We can even put that description in the function doco.
Alex
Sorry, I've been slow on this one. I've attempted a compromise: kept Kill, removed Terminate, ...
13 years, 10 months ago
(2011-05-08 07:41:09 UTC)
#13
Sorry, I've been slow on this one.
I've attempted a compromise: kept Kill, removed Terminate, and added
Signal. Kill is just a convenience function for Signal(SIGKILL). The
signal constants I've added to os are all POSIX, but aren't
exhaustive. Suggestions for including or excluding signal constants in
os are welcome.
I haven't done anything with os/signal because I'm not quite sure how
it fits in with what I'm doing here. I don't think it makes sense to
have Process.Signal take a Signal interface parameter, but maybe
there's a benefit I don't see.
- Evan
(just a side comment) http://codereview.appspot.com/4437091/diff/20001/src/pkg/os/exec_windows.go File src/pkg/os/exec_windows.go (right): http://codereview.appspot.com/4437091/diff/20001/src/pkg/os/exec_windows.go#newcode35 src/pkg/os/exec_windows.go:35: e := syscall.TerminateProcess(int32(p.handle), 137) Where ...
13 years, 9 months ago
(2011-05-26 17:21:04 UTC)
#16
http://codereview.appspot.com/4437091/diff/20001/src/pkg/os/exec_unix.go File src/pkg/os/exec_unix.go (right): http://codereview.appspot.com/4437091/diff/20001/src/pkg/os/exec_unix.go#newcode48 src/pkg/os/exec_unix.go:48: // Signal sends a signal to the Process. Please ...
13 years, 9 months ago
(2011-05-26 17:25:26 UTC)
#17
On Fri, May 27, 2011 at 5:21 AM, <krasin@google.com> wrote: > http://codereview.appspot.com/4437091/diff/20001/src/pkg/os/exec_windows.go#newcode35 > src/pkg/os/exec_windows.go:35: e ...
13 years, 9 months ago
(2011-05-26 20:55:19 UTC)
#18
On Fri, May 27, 2011 at 5:21 AM, <krasin@google.com> wrote:
>
http://codereview.appspot.com/4437091/diff/20001/src/pkg/os/exec_windows.go#n...
> src/pkg/os/exec_windows.go:35: e :=
> syscall.TerminateProcess(int32(p.handle), 137)
> Where 137 comes from? Could you please point to the documentation / blog
> article about that?
This might actually just be a Linux convention rather than Unix. I
don't have another system to test. POSIX says:
"If the process terminated abnormally due to the receipt of a signal,
the exit status shall be greater than 128 and shall be distinct from
the exit status generated by other signals, but the exact value is
unspecified." (http://pubs.opengroup.org/onlinepubs/9699919799/)
The Advanced Bash-Scripting Guide says the exit status when a process
is killed by a signal is 128+signal number.
(http://tldp.org/LDP/abs/html/exitcodes.html)
I'll change it to say 128+SIGKILL to make it a little clearer.
- Evan
On Fri, May 27, 2011 at 5:25 AM, <rsc@golang.org> wrote: > Please take the types ...
13 years, 9 months ago
(2011-05-26 20:57:22 UTC)
#19
On Fri, May 27, 2011 at 5:25 AM, <rsc@golang.org> wrote:
> Please take the types from os/signal and move them into os.
> Then the argument here should be os.Signal
I need some convincing. Won't Process.Signal just end up doing a type
switch or type assertion? If the method is just looking for one
particular implementation or set of implementations, then why not just
make it a concrete type instead?
The Signal interface makes sense to me when you're getting a Signal
out of a package, but not when you're giving one as input.
- Evan
It's an interface because it can be different concrete type on different systems, not because ...
13 years, 9 months ago
(2011-05-26 21:01:10 UTC)
#20
It's an interface because it can be different concrete type
on different systems, not because we expect someone
else to implement the interface. If p.Signal is passed
something that's not a UnixSignal then it can panic.
But the signature needs to be the same on all systems,
so os.Signal as the type.
Evan Shaw <chickencha@gmail.com> writes: > On Fri, May 27, 2011 at 5:21 AM, <krasin@google.com> wrote: ...
13 years, 9 months ago
(2011-05-26 21:21:43 UTC)
#21
Evan Shaw <chickencha@gmail.com> writes:
> On Fri, May 27, 2011 at 5:21 AM, <krasin@google.com> wrote:
>>
http://codereview.appspot.com/4437091/diff/20001/src/pkg/os/exec_windows.go#n...
>> src/pkg/os/exec_windows.go:35: e :=
>> syscall.TerminateProcess(int32(p.handle), 137)
>> Where 137 comes from? Could you please point to the documentation / blog
>> article about that?
>
> This might actually just be a Linux convention rather than Unix. I
> don't have another system to test. POSIX says:
>
> "If the process terminated abnormally due to the receipt of a signal,
> the exit status shall be greater than 128 and shall be distinct from
> the exit status generated by other signals, but the exact value is
> unspecified." (http://pubs.opengroup.org/onlinepubs/9699919799/)
>
> The Advanced Bash-Scripting Guide says the exit status when a process
> is killed by a signal is 128+signal number.
> (http://tldp.org/LDP/abs/html/exitcodes.html)
>
> I'll change it to say 128+SIGKILL to make it a little clearer.
Essentially all Unix systems use the same approach. But I'm not sure it
makes sense to use a Unix convention on Windows.
Ian
On Fri, May 27, 2011 at 9:21 AM, Ian Lance Taylor <iant@google.com> wrote: > Essentially ...
13 years, 9 months ago
(2011-05-26 21:46:33 UTC)
#22
On Fri, May 27, 2011 at 9:21 AM, Ian Lance Taylor <iant@google.com> wrote:
> Essentially all Unix systems use the same approach. But I'm not sure it
> makes sense to use a Unix convention on Windows.
That's fair, but I don't know of a number that would be better. If
someone can come up with one, I'm happy to change it. The only thing I
can say with some certainty is that the number should be non-zero.
- Evan
LGTM http://codereview.appspot.com/4437091/diff/37002/src/pkg/os/exec_windows.go File src/pkg/os/exec_windows.go (right): http://codereview.appspot.com/4437091/diff/37002/src/pkg/os/exec_windows.go#newcode32 src/pkg/os/exec_windows.go:32: switch sig.(UnixSignal) { Eventually this should be windows ...
13 years, 9 months ago
(2011-06-03 17:02:32 UTC)
#25
http://codereview.appspot.com/4437091/diff/39012/src/pkg/os/exec_windows.go File src/pkg/os/exec_windows.go (right): http://codereview.appspot.com/4437091/diff/39012/src/pkg/os/exec_windows.go#newcode34 src/pkg/os/exec_windows.go:34: // exit with 137 since that's what Unix would ...
13 years, 9 months ago
(2011-06-06 06:13:34 UTC)
#28
Issue 4437091: code review 4437091: os: add Process.Kill and Process.Signal
(Closed)
Created 13 years, 10 months ago by eds
Modified 13 years, 9 months ago
Reviewers:
Base URL:
Comments: 7