code review 8596047: go.crypto/ssh: More error reporting improvements.

go.crypto/ssh: More error reporting improvements.

[dsymonds, 2013-04-23 02:37:14]

Hello golang-dev@googlegroups.com,

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

[kardia, 2013-04-23 07:59:04]

I would suggest we use more types in the ssh error code. I like to be able 
to easily check for how it failed.
If you are going to replace a net error with a custom error, please do so 
with care; a user of this package just lost an ability to check certain 
classes of net errors.

On Monday, April 22, 2013 7:37:14 PM UTC-7, David Symonds wrote:
>
> Reviewers: golang-dev1, 
>
> Message: 
> Hello golan...@googlegroups.com <javascript:>, 
>
> I'd like you to review this change to 
> https://code.google.com/p/go.crypto 
>
>
> Description: 
> go.crypto/ssh: More error reporting improvements. 
>
> Please review this at https://codereview.appspot.com/8596047/ 
>
> Affected files: 
>    M ssh/client.go 
>    M ssh/test/test_unix_test.go 
>
>
> Index: ssh/client.go 
> =================================================================== 
> --- a/ssh/client.go 
> +++ b/ssh/client.go 
> @@ -42,7 +42,7 @@ 
>           } 
>           if err := conn.handshake(); err != nil { 
>                   conn.Close() 
> -                return nil, err 
> +                return nil, fmt.Errorf("handshake failed: %v", err) 
>           } 
>           go conn.mainLoop() 
>           return conn, nil 
> Index: ssh/test/test_unix_test.go 
> =================================================================== 
> --- a/ssh/test/test_unix_test.go 
> +++ b/ssh/test/test_unix_test.go 
> @@ -129,9 +129,9 @@ 
>
>   func (s *server) Shutdown() { 
>           if s.cmd != nil && s.cmd.Process != nil { 
> -                if err := s.cmd.Process.Kill(); err != nil { 
> -                        s.t.Error(err) 
> -                } 
> +                // don't check for Kill error; if it fails it's most 
> likely 
> +                // "os: process already finished", and we don't care 
> about that. 
> +                s.cmd.Process.Kill() 
>                   s.cmd.Wait() 
>           } 
>           if s.t.Failed() { 
>
>
>

[dsymonds, 2013-04-23 08:18:38]

On Tue, Apr 23, 2013 at 5:59 PM, Daniel Theophanes <kardianos@gmail.com> wrote:

> I would suggest we use more types in the ssh error code. I like to be able
> to easily check for how it failed.
> If you are going to replace a net error with a custom error, please do so
> with care; a user of this package just lost an ability to check certain
> classes of net errors.

Good point. The useless errors I was seeing were simply io.EOF, which
is meaningless to a program that invoked ssh.Client. Maybe I should
pass through anything that satisfies net.Error?

[kardia, 2013-04-23 16:37:00]

On Tue, 23 Apr 2013 18:18:37 +1000
David Symonds <dsymonds@golang.org> wrote:

> On Tue, Apr 23, 2013 at 5:59 PM, Daniel Theophanes
> <kardianos@gmail.com> wrote:
> 
> > I would suggest we use more types in the ssh error code. I like to
> > be able to easily check for how it failed.
> > If you are going to replace a net error with a custom error, please
> > do so with care; a user of this package just lost an ability to
> > check certain classes of net errors.
> 
> Good point. The useless errors I was seeing were simply io.EOF, which
> is meaningless to a program that invoked ssh.Client. Maybe I should
> pass through anything that satisfies net.Error?

Hi David,

I retract my previous point. In the ssh code there are three real error
types and one pseudo error type.
Real:
	UnexpectedMessageError
	ParseError
	*ExitError

pseudo
	ChannelRequest

However, most of the code just does what you did, call fmt.Errorf or
errors.New. As the code to start a client will either fail or succeed
in a fairly binary fashion, it is probably fine for now.
...
However, right now the ssh server package isn't very pleasant to work
with out of the box in-part due to how errors are handled. Also, would
be nice to handle and send the SUGGESTED close message.

Well, while I'm off topic, I'll just state what changes I'd like to
make to the package:
	Type more errors
	Handle the close message gracefully (issue 3995)
	Make the "UnexpectedMessageError" inspectable (issue 3995)
	Better authentication on server (issues 3994 & 5232)
	(Of course, let's fix the races) (issues 5137 & 5138)

I would like something like:
https://codereview.appspot.com/6546051/
to go in, to test the code we would need to send and receive the close
message, which we currently don't do.
...

Sorry for getting so off topic. Again, I retract my objections to his
CL.
-Daniel

[dave_cheney.net, 2013-04-25 02:20:29]

LGTM. Thank you.

[dsymonds, 2013-04-25 23:44:10]

*** Submitted as
https://code.google.com/p/go/source/detail?r=ecb6c1599438&repo=crypto ***

go.crypto/ssh: More error reporting improvements.

R=golang-dev, kardianos, dave
CC=golang-dev
https://codereview.appspot.com/8596047