code review 6128059: go.crypto/ssh: move common channel methods into an embe...

go.crypto/ssh: move common channel methods into an embedded struct

This CL introduces a new struct, channel to hold common shared
functions.

* add a new channel struct, which is embeded in {client,server}Chan.
* move common methods from {client,server}Chan into channel.
* remove unneeded used of serverConn.lock in serverChan
 (transport.writePacket has its own mutex).
* remove filteredConn, introduce conn.

[dave_cheney.net, 2012-05-01 10:59:32]

Hello agl@golang.org, gustav.paul@gmail.com (cc: golang-dev@googlegroups.com),

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

[agl1, 2012-05-02 17:03:25]

I think this CL is helpful and I'm not sure that it needs to be split up. I
think it would be ok to land as is. It's not a complete refactoring (I'd still
like to see the client with a ring buffer to solve the problem of its channels
getting too full), but it seems like this is in the right direction.

[dave_cheney.net, 2012-05-02 22:34:51]

Thanks for your comments agl. This CL was a quick and dirty search and
replace as a proof of concept, rather than trying to review the whole
thing, I'd like to split this CL into two; one for the various
renames, and the second for the actual code changes.

On Thu, May 3, 2012 at 3:03 AM,  <agl@golang.org> wrote:
> I think this CL is helpful and I'm not sure that it needs to be split
> up. I think it would be ok to land as is. It's not a complete
> refactoring (I'd still like to see the client with a ring buffer to
> solve the problem of its channels getting too full), but it seems like
> this is in the right direction.
>
> http://codereview.appspot.com/6128059/

[dave_cheney.net, 2012-05-05 08:08:42]

Hello agl@golang.org, gustav.paul@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[agl1, 2012-05-07 14:40:01]

LGTM

http://codereview.appspot.com/6128059/diff/24005/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6128059/diff/24005/ssh/channel.go#newcode246
ssh/channel.go:246: if err = c.writePacket(packet); err != nil {
This suggests that writePacket takes needed locks, but in other places
writePacket is called with locks held.

http://codereview.appspot.com/6128059/diff/24005/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/6128059/diff/24005/ssh/server.go#newcode104
ssh/server.go:104: // lock protects err and channels
(nit) looks like trailing whitespace

[dave_cheney.net, 2012-05-07 15:20:50]

Thank you for your comments.

http://codereview.appspot.com/6128059/diff/24005/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6128059/diff/24005/ssh/channel.go#newcode246
ssh/channel.go:246: if err = c.writePacket(packet); err != nil {
writePacket holds it's own mutex, so there is no need to serialise calls too it.
The other places that call writePacket may be holding the serverLock if they
need to ensure the err field in server conn is properly published. I'll review
all the calls on the server path to double check their locking.

On 2012/05/07 14:40:01, agl1 wrote:
> This suggests that writePacket takes needed locks, but in other places
> writePacket is called with locks held.

http://codereview.appspot.com/6128059/diff/24005/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/6128059/diff/24005/ssh/server.go#newcode104
ssh/server.go:104: // lock protects err and channels
On 2012/05/07 14:40:01, agl1 wrote:
> (nit) looks like trailing whitespace

Done.

[dave_cheney.net, 2012-05-07 15:30:56]

re: writePacket and locks. The only place where writePacket is now called
holding c.serverConn.lock is serverChan.AckRequest, and this lock is only
required to publish serverConn.err correctly.

[dave_cheney.net, 2012-05-07 15:31:36]

Hello agl@golang.org, gustav.paul@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[gpaul, 2012-05-08 22:01:19]

This patch cleans up the channel code quite a bit. Thanks Dave

http://codereview.appspot.com/6128059/diff/20006/ssh/client.go
File ssh/client.go (right):

http://codereview.appspot.com/6128059/diff/20006/ssh/client.go#newcode428
ssh/client.go:428: channel
chanWriter and chanReader embed pointers to channel whereas clientChan and
serverChan embed the value. Is there a particular reason for this or can we
standardize to say using pointers?

http://codereview.appspot.com/6128059/diff/20006/ssh/transport.go
File ssh/transport.go (right):

http://codereview.appspot.com/6128059/diff/20006/ssh/transport.go#newcode50
ssh/transport.go:50: // writer represnts the outgoing connection state.
typo, represents

[agl1, 2012-05-08 22:03:43]

(p.s. if you were waiting for me, my previous LGTM still stands.)

[gpaul, 2012-05-08 22:05:11]

LGTM too. My comments are minor nitpicks, feel free to land.

On Wed, May 9, 2012 at 12:03 AM, <agl@golang.org> wrote:

> (p.s. if you were waiting for me, my previous LGTM still stands.)
>
>
http://codereview.appspot.com/**6128059/<http://codereview.appspot.com/6128059/>
>

[dave_cheney.net, 2012-05-08 22:20:10]

*** Submitted as
http://code.google.com/p/go/source/detail?r=540906e566d5&repo=crypto ***

go.crypto/ssh: move common channel methods into an embedded struct

This CL introduces a new struct, channel to hold common shared
functions.

* add a new channel struct, which is embeded in {client,server}Chan.
* move common methods from {client,server}Chan into channel.
* remove unneeded used of serverConn.lock in serverChan
 (transport.writePacket has its own mutex).
* remove filteredConn, introduce conn.

R=agl, gustav.paul
CC=golang-dev
http://codereview.appspot.com/6128059

http://codereview.appspot.com/6128059/diff/20006/ssh/client.go
File ssh/client.go (right):

http://codereview.appspot.com/6128059/diff/20006/ssh/client.go#newcode428
ssh/client.go:428: channel
I prefer to embed structs directly (ie, not as pointers). The reason
stdin/out/err are pointers, is it makes them easier to pass to things expecting
io.Reader/Writer.

http://codereview.appspot.com/6128059/diff/20006/ssh/transport.go
File ssh/transport.go (right):

http://codereview.appspot.com/6128059/diff/20006/ssh/transport.go#newcode50
ssh/transport.go:50: // writer represnts the outgoing connection state.
On 2012/05/08 22:01:19, gpaul wrote:
> typo, represents

Done.