code review 5970056: go.crypto/ssh: fix locking and corruption issues with s... (issue 5970056)

[kard...@gmail.com]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com (cc: a...@golang.org,
golan...@googlegroups.com),

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

Description:
go.crypto/ssh: fix locking and corruption issues with server.
Fixes issue 3204.

Please review this at http://codereview.appspot.com/5970056/

Affected files:
M ssh/channel.go
M ssh/server.go
M ssh/session_test.go

[kard...@gmail.com]

Hello golan...@googlegroups.com (cc: a...@golang.org,
golan...@googlegroups.com),

Please take another look.

http://codereview.appspot.com/5970056/

[kard...@gmail.com]

[da...@cheney.net]

Hello,

Thank you for tackling this. The CL you have submitted is very large and
it looks like it combines some source cleanups with some additions which
is hard for me to follow. Could I encourage you to submit a cleanup CL
first (reordering functions, theirId -> remoteId), which will make this
more complicated change easier to grok.

Cheers

Dave

http://codereview.appspot.com/5970056/

[kard...@gmail.com]

Hello golan...@googlegroups.com, da...@cheney.net (cc: a...@golang.org,

[kard...@gmail.com]

I would normally be ready to back you on this. However, in this case I
wasn't adjusting the existing code; I rewrote the interface function by
function, on occasion taking snippets from the existing code. I
couldn't get the existing code to fully work by just changing it. So
please ignore the delta and treat this as new code (which it is).

A few known issues here:
* We are adjusting the window every time we read. For large batch
sessions, this could ok be but not optimal. For terminal sessions this
is less ideal. Attempts to fix this have not been met with success so
far. This should be addressed in a followup CL.

* The defaultWindowSize const can be adjusted up to ~200000, but values
greater then that causes it to stop functioning correctly. This is less
of a critical issue, but I think it should still be addressed. This
leads me to believe something is still not quite right.

[kard...@gmail.com]

[kard...@gmail.com]

[kard...@gmail.com]

I should talk to more people before mailing out a change set. After
writing out what didn't work, I realized what I could do to make it
behave correctly.

As far as I know, this should take care of all of the known issues, as
well as the two I listed below. Sorry for all the noise in the
meantime.

Regarding the defaultWindowSize: currently set at 32768 byte, I noticed
the openssh client has their's at ~2M byte. Later maybe want a server
option to set the window size?