code review 6586060: go.crypto/ssh: Read returns all unread bytes before ret...

go.crypto/ssh: Read returns all unread bytes before returning io.EOF.

Fixes issue 4158.

[gpaul, 2012-10-02 07:12:11]

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

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

[dave_cheney.net, 2012-10-02 10:42:23]

LGTM, but please wait for agl.

As discussed I am working on a new func test runner that avoid having to provide
a username and password, but it is not finished yet. Once it is ready I will
merge this new test into it.

https://codereview.appspot.com/6586060/diff/5001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/6586060/diff/5001/ssh/channel.go#newcode565
ssh/channel.go:565: err = r.sendWindowAdj(n)
As discussed, this probably needs a comment. Some suggestions include

// sendWindowAdjust can return io.EOF above if the remote peer has closed the
connection, however we want to defer forwarding io.EOF to the called of Read
until the buffer has been drained.

[gpaul, 2012-10-02 14:08:54]

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

Please take another look.

[dave_cheney.net, 2012-10-02 14:12:25]

Please add

Fixes issue NNN

to the description.
On 3 Oct 2012 00:08, <gustav.paul@gmail.com> wrote:

> Hello dave@cheney.net (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**6586060/<http://codereview.appspot.com/6586060/>
>

[dave_cheney.net, 2012-10-02 14:18:12]

On 2012/10/02 14:12:25, dfc wrote:
> Please add
> 
> Fixes issue NNN
> 
> to the description.
> On 3 Oct 2012 00:08, <mailto:gustav.paul@gmail.com> wrote:
> 
> > Hello mailto:dave@cheney.net (cc: mailto:golang-dev@googlegroups.com),
> >
> > Please take another look.
> >
> >
> >
>
http://codereview.appspot.com/**6586060/%3Chttp://codereview.appspot.com/6586...>
> >

LGTM. Leaving for agl. Will submit tomorrow if there are no significant
concerns.

[agl1, 2012-10-02 22:56:22]

LGTM

https://codereview.appspot.com/6586060/diff/10001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/6586060/diff/10001/ssh/channel.go#newcode567
ssh/channel.go:567: // sendWindowAdjust can return io.EOF above if the remote
peer has
s/above //

https://codereview.appspot.com/6586060/diff/10001/ssh/channel.go#newcode570
ssh/channel.go:570: return n, nil
I'd just say err = nil and fall through.

[dave_cheney.net, 2012-10-02 23:25:33]

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

go.crypto/ssh: Read returns all unread bytes before returning io.EOF.

Fixes issue 4158.

R=dave, agl
CC=golang-dev
http://codereview.appspot.com/6586060

Committer: Dave Cheney <dave@cheney.net>

[dave_cheney.net, 2012-10-02 23:26:22]

@gpaul, thank you for this fix
@agl, i've made the changes you suggested before submitting.

[remyoudompheng, 2013-07-21 19:50:29]

R=close