code review 21940043: gosshnew/ssh: implement client using modular connections. (issue 21940043)

[han...@google.com]

Reviewers: agl1, dfc,

Message:
Hello a...@golang.org, da...@cheney.net (cc: a...@golang.org,
d...@golang.org, golan...@googlegroups.com, han...@google.com),

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


Description:
gosshnew/ssh: implement client using modular connections.

Please review this at https://codereview.appspot.com/21940043/

Affected files (+179, -532 lines):
M ssh/client.go
M ssh/session.go
M ssh/session_test.go
M ssh/tcpip.go

[a...@golang.org]

LGTM


https://codereview.appspot.com/21940043/diff/40001/ssh/session.go
File ssh/session.go (right):

https://codereview.appspot.com/21940043/diff/40001/ssh/session.go#newcode192
ssh/session.go:192: ok, err := s.ch.SendRequest("pty-req", true,
marshal(0, req))
maybe consider:

const dummyMessageType = 0

just to make the meaning of this 0 clearer.

https://codereview.appspot.com/21940043/diff/40001/ssh/session.go#newcode326
ssh/session.go:326: return fmt.Errorf("ssh: cound not execute")
typo: cound

maybe "ssh: failed to execute shell" or "ssh: could not execute shell"

https://codereview.appspot.com/21940043/diff/40001/ssh/session_test.go
File ssh/session_test.go (left):

https://codereview.appspot.com/21940043/diff/40001/ssh/session_test.go#oldcode383
ssh/session_test.go:383: session.clientChan.sendWindowAdj(0)
I assume that zero sided updates still work? :)

https://codereview.appspot.com/21940043/diff/40001/ssh/tcpip.go
File ssh/tcpip.go (right):

https://codereview.appspot.com/21940043/diff/40001/ssh/tcpip.go#newcode97
ssh/tcpip.go:97: ok, resp, err := c.mux.SendRequest("tcpip-forward",
true, marshal(0, m))
ditto about the zero.

https://codereview.appspot.com/21940043/

[han...@google.com]

https://codereview.appspot.com/21940043/diff/40001/ssh/session.go
File ssh/session.go (right):

https://codereview.appspot.com/21940043/diff/40001/ssh/session.go#newcode192
ssh/session.go:192: ok, err := s.ch.SendRequest("pty-req", true,
marshal(0, req))

On 2013/11/05 23:25:20, agl1 wrote:
> maybe consider:

> const dummyMessageType = 0

> just to make the meaning of this 0 clearer.

I've added a function marshalBare instead.

https://codereview.appspot.com/21940043/diff/40001/ssh/session.go#newcode326
ssh/session.go:326: return fmt.Errorf("ssh: cound not execute")

On 2013/11/05 23:25:20, agl1 wrote:
> typo: cound

> maybe "ssh: failed to execute shell" or "ssh: could not execute shell"

Done.

https://codereview.appspot.com/21940043/diff/40001/ssh/session_test.go
File ssh/session_test.go (left):

https://codereview.appspot.com/21940043/diff/40001/ssh/session_test.go#oldcode383
ssh/session_test.go:383: session.clientChan.sendWindowAdj(0)

On 2013/11/05 23:25:20, agl1 wrote:
> I assume that zero sided updates still work? :)

there is a test for it in mux_test.go that you said looked good :)

https://codereview.appspot.com/21940043/diff/40001/ssh/tcpip.go
File ssh/tcpip.go (right):

https://codereview.appspot.com/21940043/diff/40001/ssh/tcpip.go#newcode97
ssh/tcpip.go:97: ok, resp, err := c.mux.SendRequest("tcpip-forward",
true, marshal(0, m))

On 2013/11/05 23:25:20, agl1 wrote:
> ditto about the zero.

Done.

https://codereview.appspot.com/21940043/

[han...@google.com]

*** Submitted as
https://code.google.com/p/gosshnew/source/detail?r=889d17fec13d ***

gosshnew/ssh: use modular connection code on the client side.

R=agl, dave
CC=agl, dfc, golang-dev, hanwen
https://codereview.appspot.com/21940043


https://codereview.appspot.com/21940043/