code review 9738053: go.crypto/ssh: add a error return to decode(), and avoi...

go.crypto/ssh: add a error return to decode(), and avoid casting decode() output.

[hanwen-google, 2013-05-31 22:22:42]

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

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

[gobot, 2013-06-01 02:07:00]

R=dfc (assigned by dfc)

[dave_cheney.net, 2013-06-01 05:39:52]

Thank you for this proposal. I'd like to hear from a few other folks about the
error handling.

https://codereview.appspot.com/9738053/diff/3001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/9738053/diff/3001/ssh/client.go#newcode255
ssh/client.go:255: continue
I'm not sure about continuing after an error

https://codereview.appspot.com/9738053/diff/3001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/9738053/diff/3001/ssh/server.go#newcode565
ssh/server.go:565: decoded, _ := decode(packet)
I think it is worth handling the error code here.

[kardia, 2013-06-01 06:13:58]

I like making error value out of band.
LGTM after comments addressed.

https://codereview.appspot.com/9738053/diff/3001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/9738053/diff/3001/ssh/client.go#newcode253
ssh/client.go:253: if err != nil {
You should continue on UnexpectedMessageError type, and return on any other
error. Before continuing you should replicate the error message in the default
case of this switch.

https://codereview.appspot.com/9738053/diff/3001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/9738053/diff/3001/ssh/server.go#newcode565
ssh/server.go:565: decoded, _ := decode(packet)
Agree. Please handle this error. However, please ignore the
UnexpectedMessageError for now to maintain current working status. return on any
other error.

[hanwen-google, 2013-06-01 10:52:31]

PTAL

https://codereview.appspot.com/9738053/diff/3001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/9738053/diff/3001/ssh/client.go#newcode253
ssh/client.go:253: if err != nil {
On 2013/06/01 06:13:58, kardia wrote:
> You should continue on UnexpectedMessageError type, and return on any other
> error. Before continuing you should replicate the error message in the default
> case of this switch.

Done.

Note that the error should really go onto stderr rather than stdout, but let's
leave that for another CL.

https://codereview.appspot.com/9738053/diff/3001/ssh/client.go#newcode255
ssh/client.go:255: continue
On 2013/06/01 05:39:53, dfc wrote:
> I'm not sure about continuing after an error

Done.

https://codereview.appspot.com/9738053/diff/3001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/9738053/diff/3001/ssh/server.go#newcode565
ssh/server.go:565: decoded, _ := decode(packet)
On 2013/06/01 06:13:58, kardia wrote:
> Agree. Please handle this error. However, please ignore the
> UnexpectedMessageError for now to maintain current working status. return on
any
> other error.

Done.

[kardia, 2013-06-01 21:21:52]

LGTM for error handling.
Agree that errors should not print to Stdout and that it should be in a
different CL.

[hanwen-google, 2013-06-06 13:34:18]

Thanks, do I need to do anything to get the patch in?

[agl1, 2013-06-06 14:24:47]

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

go.crypto/ssh: add a error return to decode(), and avoid casting decode()
output.

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

Committer: Adam Langley <agl@golang.org>

[hanwen-google, 2013-06-06 15:42:54]

*** Abandoned ***