code review 6479056: go.crypto/ssh: assorted close related fixes

go.crypto/ssh: assorted close related fixes

Fixes issue 3810.

Fixes chanWriter Write after close behaviour bug.

Fixes serverChan writePacket after close bug.

Addresses final comments by agl on 6405064, plus various cleanups.

[dfc, 2012-09-01 01:14:19]

Hello agl@golang.org, kardianos@gmail.com, 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

[kardia, 2012-09-01 03:20:41]

LGTM.

I lightly tested the server code and it gave me no error.

http://codereview.appspot.com/6479056/diff/7006/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6479056/diff/7006/ssh/channel.go#newcode360
ssh/channel.go:360: func (c *serverChan) dead() bool {
Can we re-name this to isDead()? Normally think it would be fine but we also
have setDead and sendDead, and I get confused :)

http://codereview.appspot.com/6479056/diff/7006/ssh/session_test.go
File ssh/session_test.go (right):

http://codereview.appspot.com/6479056/diff/7006/ssh/session_test.go#newcode627
ssh/session_test.go:627: // TODO(dfc) io.Copy can return a non EOF error here
Is this still a TODO?

[dfc, 2012-09-01 03:45:02]

Thanks you for your comments.

http://codereview.appspot.com/6479056/diff/7006/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6479056/diff/7006/ssh/channel.go#newcode360
ssh/channel.go:360: func (c *serverChan) dead() bool {
On 2012/09/01 03:20:41, kardia wrote:
> Can we re-name this to isDead()? Normally think it would be fine but we also
> have setDead and sendDead, and I get confused :)

From previous CLs I believe it is more idiomatic to use dead(), rather than
isDead(). I'm fine with either way as long as there is a consensus.

http://codereview.appspot.com/6479056/diff/7006/ssh/session_test.go
File ssh/session_test.go (right):

http://codereview.appspot.com/6479056/diff/7006/ssh/session_test.go#newcode627
ssh/session_test.go:627: // TODO(dfc) io.Copy can return a non EOF error here
On 2012/09/01 03:20:41, kardia wrote:
> Is this still a TODO?

I need to do more stress testing to confirm. This is the last place where an
error can leak out of close.

[dfc, 2012-09-01 03:45:23]

+fullung

[dfc, 2012-09-01 09:42:01]

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

Please take another look.

[dfc, 2012-09-02 01:13:23]

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

Please take another look.

[dfc, 2012-09-03 23:42:50]

ping. I'd be grateful if gpaul or agl could give this a once over.

[gpaul, 2012-09-04 08:13:30]

LGTM. Just some questions inline.

http://codereview.appspot.com/6479056/diff/10006/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6479056/diff/10006/ssh/channel.go#newcode135
ssh/channel.go:135: return atomic.CompareAndSwapUint32(&c.isClosed, 0, 1)
Why not StoreUint32 like in setDead()?

http://codereview.appspot.com/6479056/diff/10006/ssh/channel.go#newcode352
ssh/channel.go:352: err = io.EOF
consider returning 0, io.EOF here, instead of assigning to err, then returning
it if not nil on the next line

http://codereview.appspot.com/6479056/diff/10006/ssh/channel.go#newcode550
ssh/channel.go:550: w.eof = true
isn't w.eof accessed from different goroutines? Perhaps eof needs to become
eof() and wrapped by sync/atomic as is done for dead->isDead()?

http://codereview.appspot.com/6479056/diff/10006/ssh/client.go
File ssh/client.go (right):

http://codereview.appspot.com/6479056/diff/10006/ssh/client.go#newcode252
ssh/client.go:252: msg := decode(packet)
nice

[dfc, 2012-09-04 08:25:33]

Thank you for your comments, PTAL.

http://codereview.appspot.com/6479056/diff/10006/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6479056/diff/10006/ssh/channel.go#newcode135
ssh/channel.go:135: return atomic.CompareAndSwapUint32(&c.isClosed, 0, 1)
On 2012/09/04 08:13:30, gpaul wrote:
> Why not StoreUint32 like in setDead()?

Done. I don't use the return value

http://codereview.appspot.com/6479056/diff/10006/ssh/channel.go#newcode352
ssh/channel.go:352: err = io.EOF
On 2012/09/04 08:13:30, gpaul wrote:
> consider returning 0, io.EOF here, instead of assigning to err, then returning
> it if not nil on the next line

Good point, now there is no lock, I don't need these gymnastics.

http://codereview.appspot.com/6479056/diff/10006/ssh/channel.go#newcode550
ssh/channel.go:550: w.eof = true
On 2012/09/04 08:13:30, gpaul wrote:
> isn't w.eof accessed from different goroutines? Perhaps eof needs to become
> eof() and wrapped by sync/atomic as is done for dead->isDead()?

see the comment in the chanWriter struct. eof is owned by the caller to
Write/Close.

http://codereview.appspot.com/6479056/diff/10006/ssh/client.go
File ssh/client.go (right):

http://codereview.appspot.com/6479056/diff/10006/ssh/client.go#newcode252
ssh/client.go:252: msg := decode(packet)
On 2012/09/04 08:13:30, gpaul wrote:
> nice

This is my debugging point.

[dfc, 2012-09-04 08:25:53]

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

Please take another look.

[gpaul, 2012-09-04 12:01:54]

On 2012/09/04 08:25:53, dfc wrote:
> Hello mailto:agl@golang.org, mailto:kardianos@gmail.com,
mailto:gustav.paul@gmail.com,
> mailto:fullung@gmail.com (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

LGTM

[agl1, 2012-09-04 13:31:34]

LGTM

[dfc, 2012-09-04 23:47:08]

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

go.crypto/ssh: assorted close related fixes

Fixes issue 3810.

Fixes chanWriter Write after close behaviour bug.

Fixes serverChan writePacket after close bug.

Addresses final comments by agl on 6405064, plus various cleanups.

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