code review 81290043: exec: Add ExitError.ExitStatus() method.

exec: Add ExitError.ExitStatus() method.

This adds a method to the ExitError struct to help retrieve the underlying status code from the process.

It uses the same name that ssh.Waitmsg provides in the new ssh package in go.crypto:
http://godoc.org/code.google.com/p/go.crypto/ssh#Waitmsg.ExitStatus

[david.calavera, 2014-03-27 07:23:25]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[r, 2014-03-27 12:00:00]

This is a new feature so it must wait until after the feature freeze. 

Please send it again after 1.3 is released in June.

[gobot, 2014-04-02 08:21:47]

R=close (assigned by dave@cheney.net)

[david.calavera, 2014-06-11 00:14:43]

Hello golang-codereviews@googlegroups.com (cc:
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[dave_cheney.net, 2014-06-11 09:39:22]

https://codereview.appspot.com/81290043/diff/40001/src/pkg/os/exec/exec.go
File src/pkg/os/exec/exec.go (right):

https://codereview.appspot.com/81290043/diff/40001/src/pkg/os/exec/exec.go#ne...
src/pkg/os/exec/exec.go:300: func (e *ExitError) ExitStatus() int {
I'm not convinced that -1 is the correct value to return if the process is still
running. 

That said, I can't think of any value that is correct. My instinct drives me to
suggest, 0, the zero value, but that has implications that someone calling
ExitStatus() may think the command succeeded.

Maybe this method should block until an exit status code is known.

[giacomo.tartari_gmail.com, 2014-06-11 14:21:43]

On Wednesday, 11 June 2014 11:39:23 UTC+2, da...@cheney.net  wrote:
> https://codereview.appspot.com/81290043/diff/40001/src/pkg/os/exec/exec.go
> 
> File src/pkg/os/exec/exec.go (right):
> 
> 
> 
>
https://codereview.appspot.com/81290043/diff/40001/src/pkg/os/exec/exec.go#ne...
> 
> src/pkg/os/exec/exec.go:300: func (e *ExitError) ExitStatus() int {
> 
> I'm not convinced that -1 is the correct value to return if the process
> 
> is still running.
> 
> 
> 
> That said, I can't think of any value that is correct. My instinct
> 
> drives me to suggest, 0, the zero value, but that has implications that
> 
> someone calling ExitStatus() may think the command succeeded.
> 
> 
> 
> Maybe this method should block until an exit status code is known.
Or use a ",OK" return? Ok is false if still running.
> 
> 
> 
> https://codereview.appspot.com/81290043/

[david.calavera, 2014-06-11 16:32:30]

On 2014/06/11 14:21:43, giacomo.tartari_gmail.com wrote:
> On Wednesday, 11 June 2014 11:39:23 UTC+2, mailto:da...@cheney.net  wrote:
> > https://codereview.appspot.com/81290043/diff/40001/src/pkg/os/exec/exec.go
> > 
> > File src/pkg/os/exec/exec.go (right):
> > 
> > 
> > 
> >
>
https://codereview.appspot.com/81290043/diff/40001/src/pkg/os/exec/exec.go#ne...
> > 
> > src/pkg/os/exec/exec.go:300: func (e *ExitError) ExitStatus() int {
> > 
> > I'm not convinced that -1 is the correct value to return if the process
> > 
> > is still running.
> > 
> > 
> > 
> > That said, I can't think of any value that is correct. My instinct
> > 
> > drives me to suggest, 0, the zero value, but that has implications that
> > 
> > someone calling ExitStatus() may think the command succeeded.
> > 
> > 
> > 
> > Maybe this method should block until an exit status code is known.
> Or use a ",OK" return? Ok is false if still running.
> > 
> > 
> > 
> > https://codereview.appspot.com/81290043/
> 

I don't think 0 is better than -1, it might even cause more confusion as you
mentioned, dfc.

Returning "OK" sounds good to me, but I'd like to keep this method and
go.crypto's compatible. I'm okay with changing go.crypto too if we agree that's
the best solution.

[bradfitz, 2014-06-11 18:26:11]

https://codereview.appspot.com/81290043/diff/40001/src/pkg/os/exec/exec.go
File src/pkg/os/exec/exec.go (right):

https://codereview.appspot.com/81290043/diff/40001/src/pkg/os/exec/exec.go#ne...
src/pkg/os/exec/exec.go:305: s := e.Sys().(syscall.WaitStatus)
FYI, plan9 has no syscall.WaitStatus. This would break that builder.

[r, 2014-06-12 19:37:58]

NOT LGTM

I don't see how this can be done portably. It's not been done already for just
that reason.

Plus the -1 creeps me out.

[gobot, 2014-06-16 20:08:47]

R=close (assigned by r@golang.org)