code review 6049050: crypto/ssh: fix several logic errors.

crypto/ssh: fix several logic errors.

These are the obvious mistakes from my read through. I'll save the
more cosmetic changes for a later CL.

[agl1, 2012-04-19 00:06:03]

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

I'd like you to review this change to
https://agl%40golang.org@code.google.com/p/go.crypto/

[agl1, 2012-04-19 00:17:35]

http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go
File ssh/channel.go (left):

http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go#oldcode239
ssh/channel.go:239: if c.myWindow <= uint32(len(c.pendingData))/2 {
Previously we would let the other side know that it could send more data as soon
as our window was low. However, this didn't ensure that we had actually cleared
out the needed space in c.pendingData by reading it.

http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go#newcode260
ssh/channel.go:260: tail = len(c.pendingData)
In the event that the buffer is wrapping around the end, we want to read until
the end of the buffer.

http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go#newcode275
ssh/channel.go:275: c.serverConn.lock.Lock()
Note the additional serverConn.lock compared to the previous code.

[kardia, 2012-04-19 05:11:55]

Appears to work well in the tests I ran on it (pushing/pulling hg repos through
it). I'll run some additional tests probably this weekend but I doubt it will
uncover anything. Thanks :)

On 2012/04/19 00:17:35, agl1 wrote:
> http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go
> File ssh/channel.go (left):
> 
> http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go#oldcode239
> ssh/channel.go:239: if c.myWindow <= uint32(len(c.pendingData))/2 {
> Previously we would let the other side know that it could send more data as
soon
> as our window was low. However, this didn't ensure that we had actually
cleared
> out the needed space in c.pendingData by reading it.
> 
> http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go
> File ssh/channel.go (right):
> 
> http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go#newcode260
> ssh/channel.go:260: tail = len(c.pendingData)
> In the event that the buffer is wrapping around the end, we want to read until
> the end of the buffer.
> 
> http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go#newcode275
> ssh/channel.go:275: c.serverConn.lock.Lock()
> Note the additional serverConn.lock compared to the previous code.

[dave_cheney.net, 2012-04-19 14:54:37]

LGTM.

Kardia - I would like to see your ideas about the circular buffer integrated if
you still wish to contribute them.

http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go#newcode260
ssh/channel.go:260: tail = len(c.pendingData)
maybe tail := min(c.head + c.length, len(c.pendingData))

[agl1, 2012-04-19 15:17:05]

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

crypto/ssh: fix several logic errors.

These are the obvious mistakes from my read through. I'll save the
more cosmetic changes for a later CL.

R=dave, kardianos
CC=golang-dev
http://codereview.appspot.com/6049050

http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go
File ssh/channel.go (right):

http://codereview.appspot.com/6049050/diff/2001/ssh/channel.go#newcode260
ssh/channel.go:260: tail = len(c.pendingData)
On 2012/04/19 14:54:38, dfc wrote:
> maybe tail := min(c.head + c.length, len(c.pendingData))

Done.