code review 6208043: go.crypto/ssh: make {client,server}Chan use common wind...

go.crypto/ssh: make {client,server}Chan use common window management

[dave_cheney.net, 2012-05-09 16:18:59]

Hello agl@golang.org, gustav.paul@gmail.com, kardianos@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-09 16:23:28]

LGTM

http://codereview.appspot.com/6208043/diff/3005/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6208043/diff/3005/ssh/channel.go#newcode163
ssh/channel.go:163: func (c *serverChan) check() error {
check seems like a very generic name. isClosed? isDead?

[kardia, 2012-05-09 16:37:24]

LGTM.

http://codereview.appspot.com/6208043/diff/3005/ssh/common.go
File ssh/common.go (right):

http://codereview.appspot.com/6208043/diff/3005/ssh/common.go#newcode300
ssh/common.go:300: return false
return true // Requesting 0 bytes be added is a noop, but not a failure.

http://codereview.appspot.com/6208043/diff/3005/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/6208043/diff/3005/ssh/server.go#newcode548
ssh/server.go:548: remoteWin: window{Cond: sync.NewCond(new(sync.Mutex))},
3rd time I've seen this code + c.remoteWin.add(msg.PeersWindow).
Seems possible to have a newWindow(size uint) func that creates a new mutex and
sets the size. But this is just as clear and no harm here.

[dave_cheney.net, 2012-05-09 16:58:09]

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

Please take another look.

[dave_cheney.net, 2012-05-09 17:01:05]

Thank you for your comments. PTAL.

http://codereview.appspot.com/6208043/diff/5001/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6208043/diff/5001/ssh/channel.go#newcode163
ssh/channel.go:163: func (c *serverChan) check() error {
check replicates the checking of c.dead and c.weClosed that used to exist in
c.GetWindowSize. Their may be a better way to do this (or elide it completely).

http://codereview.appspot.com/6208043/diff/3005/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6208043/diff/3005/ssh/channel.go#newcode163
ssh/channel.go:163: func (c *serverChan) check() error {
On 2012/05/09 16:23:28, agl1 wrote:
> check seems like a very generic name. isClosed? isDead?

Maybe I should just move this code into the two Write methods? It's not a lot of
code.

http://codereview.appspot.com/6208043/diff/3005/ssh/common.go
File ssh/common.go (right):

http://codereview.appspot.com/6208043/diff/3005/ssh/common.go#newcode300
ssh/common.go:300: return false
I'd rather not change this at this point. It's been working so far. I will add a
TODO to investigate.

On 2012/05/09 16:37:24, kardia wrote:
> return true // Requesting 0 bytes be added is a noop, but not a failure.

http://codereview.appspot.com/6208043/diff/3005/ssh/server.go
File ssh/server.go (right):

http://codereview.appspot.com/6208043/diff/3005/ssh/server.go#newcode548
ssh/server.go:548: remoteWin: window{Cond: sync.NewCond(new(sync.Mutex))},
Yes, I'm sad that there is no zero value form of sync.Cond.

On 2012/05/09 16:37:24, kardia wrote:
> 3rd time I've seen this code + c.remoteWin.add(msg.PeersWindow).
> Seems possible to have a newWindow(size uint) func that creates a new mutex
and
> sets the size. But this is just as clear and no harm here.

[agl1, 2012-05-09 17:04:06]

LGTM

[dave_cheney.net, 2012-05-09 19:14:55]

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

Please take another look.

[dave_cheney.net, 2012-05-10 19:24:32]

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

Please take another look.

[dave_cheney.net, 2012-05-10 19:25:35]

This is the last change to this CL. PTAL.

[kardia, 2012-05-10 19:33:05]

LGTM

http://codereview.appspot.com/6208043/diff/9002/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6208043/diff/9002/ssh/channel.go#newcode326
ssh/channel.go:326: // TODO(dfc) This lock and check of c.weClosed is necessary
because unlike
Good comment. Why is it a TODO?

[dave_cheney.net, 2012-05-10 19:35:47]

> ssh/channel.go:326: // TODO(dfc) This lock and check of c.weClosed is
> necessary because unlike
> Good comment. Why is it a TODO?

Because I intend to remove this requirement.

[dave_cheney.net, 2012-05-10 19:56:54]

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

go.crypto/ssh: make {client,server}Chan use common window management

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

http://codereview.appspot.com/6208043/diff/9002/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6208043/diff/9002/ssh/channel.go#newcode326
ssh/channel.go:326: // TODO(dfc) This lock and check of c.weClosed is necessary
because unlike
On 2012/05/10 19:33:05, kardia wrote:
> Good comment. Why is it a TODO?

Done.