code review 5555044: exp/ssh: handle versions with just '\n'

exp/ssh: handle versions with just '\n'

djm recommend that we do this because OpenSSL was only fixed in 2008:
http://anoncvs.mindrot.org/index.cgi/openssh/sshd.c?revision=1.380&view=markup

[agl1, 2012-01-18 00:31:15]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[dave_cheney.net, 2012-01-18 09:07:57]

Looks pretty good, please see the comment about bytes.Trim

http://codereview.appspot.com/5555044/diff/4003/src/pkg/exp/ssh/transport.go
File src/pkg/exp/ssh/transport.go (right):

http://codereview.appspot.com/5555044/diff/4003/src/pkg/exp/ssh/transport.go#...
src/pkg/exp/ssh/transport.go:366: return versionString, nil
Could this be simplified?

   return bytes.Trim(versionString, "\r\n"), nil

http://codereview.appspot.com/5555044/diff/4003/src/pkg/exp/ssh/transport_tes...
File src/pkg/exp/ssh/transport_test.go (right):

http://codereview.appspot.com/5555044/diff/4003/src/pkg/exp/ssh/transport_tes...
src/pkg/exp/ssh/transport_test.go:33: if !bytes.Equal(buf[:len(buf)-2], result)
{
I think this should be buf[:len(buf)-1] as you already stripped off the last
char on line 27

[jmp, 2012-01-18 18:59:34]

http://codereview.appspot.com/5555044/diff/4003/src/pkg/exp/ssh/transport.go
File src/pkg/exp/ssh/transport.go (right):

http://codereview.appspot.com/5555044/diff/4003/src/pkg/exp/ssh/transport.go#...
src/pkg/exp/ssh/transport.go:361: // We need to remove the [CR]LR from
versionString
s/[CR]LR/[CR]LF/

[agl1, 2012-01-18 20:04:27]

*** Submitted as http://code.google.com/p/go/source/detail?r=f1e930b56e11 ***

exp/ssh: handle versions with just '\n'

djm recommend that we do this because OpenSSL was only fixed in 2008:
http://anoncvs.mindrot.org/index.cgi/openssh/sshd.c?revision=1.380&view=markup

R=dave, jonathan.mark.pittman
CC=golang-dev
http://codereview.appspot.com/5555044

http://codereview.appspot.com/5555044/diff/4003/src/pkg/exp/ssh/transport.go
File src/pkg/exp/ssh/transport.go (right):

http://codereview.appspot.com/5555044/diff/4003/src/pkg/exp/ssh/transport.go#...
src/pkg/exp/ssh/transport.go:361: // We need to remove the [CR]LR from
versionString
On 2012/01/18 18:59:34, jmp wrote:
> s/[CR]LR/[CR]LF/

Done.

http://codereview.appspot.com/5555044/diff/4003/src/pkg/exp/ssh/transport.go#...
src/pkg/exp/ssh/transport.go:366: return versionString, nil
On 2012/01/18 09:07:57, dfc wrote:
> Could this be simplified?
>    return bytes.Trim(versionString, "\r\n"), nil

As it turned out, I had a bug in here hidden by the bad test: the '\n' is never
appended, so we only need to (possibly) remove the last byte. I think that this
is simple enough that's it's not worth invoking bytes.TrimRight, in all it's
closure and Unicode glory.

http://codereview.appspot.com/5555044/diff/4003/src/pkg/exp/ssh/transport_tes...
File src/pkg/exp/ssh/transport_test.go (right):

http://codereview.appspot.com/5555044/diff/4003/src/pkg/exp/ssh/transport_tes...
src/pkg/exp/ssh/transport_test.go:33: if !bytes.Equal(buf[:len(buf)-2], result)
{
On 2012/01/18 09:07:57, dfc wrote:
> I think this should be buf[:len(buf)-1] as you already stripped off the last
> char on line 27

Yes, thanks!