code review 18120043: go.crypto/ssh: Increase window size.

go.crypto/ssh: Increase window size.

Increase window size for channels (session and tcpip) to 64 *
max packet size (32 KB), which is the same value that OpenSSH
uses. Also breaks out the relevant hardcoded constants into named
constants in channel.go.

Fixes issue 6675.

[jborg, 2013-10-27 20:28:53]

Hello golang-dev@googlegroups.com,

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

[dfc, 2013-10-27 21:03:19]

On 2013/10/27 20:28:53, jborg wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://code.google.com/p/go.crypto

Can you please start the CLA process as well. You don't get any notification
when you complete the form but someone from Google will propose a CL to add your
details to the AUTHOR and CONTRIBUTORS files.

[jborg, 2013-10-27 21:18:28]

On 27 okt 2013, at 22:03, dave@cheney.net wrote:
> 
>> On 2013/10/27 20:28:53, jborg wrote:
>> Hello mailto:golang-dev@googlegroups.com,
> 
>> I'd like you to review this change to
>> https://code.google.com/p/go.crypto
> 
> Can you please start the CLA process as well. You don't get any
> notification when you complete the form but someone from Google will
> propose a CL to add your details to the AUTHOR and CONTRIBUTORS files.
> 
> https://codereview.appspot.com/18120043/

Yep, did that earlier this evening, thanks. 

//jb

[hanwen-google, 2013-10-28 08:32:21]

LGTM

(when names are fixed).

https://codereview.appspot.com/18120043/diff/40001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/18120043/diff/40001/ssh/channel.go#newcode29
ssh/channel.go:29: advInitialWindowSize = 64 * advMaxPacketSize // Like OpenSSH
the names are hard to read.  Why not simply 

  channelMaxPacketSize
  channelWindowSize

?

https://codereview.appspot.com/18120043/diff/40001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/18120043/diff/40001/ssh/client.go#newcode363
ssh/client.go:363: // As per RFC 4253 6.1, 32k is also the minimum.
comment can be removed or moved to constant declaration.

[jborg, 2013-10-28 10:56:10]

On 2013/10/28 08:32:21, hanwen-google wrote:
> LGTM
> 
> (when names are fixed).
> 
> https://codereview.appspot.com/18120043/diff/40001/ssh/channel.go
> File ssh/channel.go (right):
> 
> https://codereview.appspot.com/18120043/diff/40001/ssh/channel.go#newcode29
> ssh/channel.go:29: advInitialWindowSize = 64 * advMaxPacketSize // Like
OpenSSH
> the names are hard to read.  Why not simply 
> 
>   channelMaxPacketSize
>   channelWindowSize
> 
> ?
> 
> https://codereview.appspot.com/18120043/diff/40001/ssh/client.go
> File ssh/client.go (right):
> 
> https://codereview.appspot.com/18120043/diff/40001/ssh/client.go#newcode363
> ssh/client.go:363: // As per RFC 4253 6.1, 32k is also the minimum.
> comment can be removed or moved to constant declaration.

Sure, works for me. Updated.

//jb

[hanwen-google, 2013-10-28 12:18:53]

LGTM

https://codereview.appspot.com/18120043/diff/100001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/18120043/diff/100001/ssh/channel.go#newcode28
ssh/channel.go:28: // channelWindowSize defines the window size advertised in
open messages
blank before this line for consistency.

[hanwen-google, 2013-10-29 17:38:58]

On 2013/10/28 12:18:53, hanwen-google wrote:
> LGTM
> 
> https://codereview.appspot.com/18120043/diff/100001/ssh/channel.go
> File ssh/channel.go (right):
> 
> https://codereview.appspot.com/18120043/diff/100001/ssh/channel.go#newcode28
> ssh/channel.go:28: // channelWindowSize defines the window size advertised in
> open messages
> blank before this line for consistency.

there is also a typo in the CL description

[dfc, 2013-10-29 18:09:43]

I've been testing this over the last few days and it does what it says on the
tin. agl, any comments ?

jborg if you could fixup the final review comments and hg mail again I'll get
the CLA sorted out.

[jborg, 2013-10-29 18:29:04]

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

Please take another look.

[agl1, 2013-10-29 19:08:33]

LGTM if others are happy.

[dfc, 2013-10-31 17:25:04]

*** Submitted as
https://code.google.com/p/go/source/detail?r=6478cc9340cb&repo=crypto ***

go.crypto/ssh: Increase window size.

Increase window size for channels (session and tcpip) to 64 *
max packet size (32 KB), which is the same value that OpenSSH
uses. Also breaks out the relevant harcoded constants into named
constants in channel.go.

Fixes issue 6675.

R=golang-dev, dave, hanwen, agl
CC=golang-dev
https://codereview.appspot.com/18120043

Committer: Dave Cheney <dave@cheney.net>

[hanwen-google, 2013-11-05 15:51:37]

Hi Jakob,

On Tue, Oct 29, 2013 at 7:29 PM,  <jakob@nym.se> wrote:
> Hello golang-dev@googlegroups.com, dave@cheney.net, hanwen@google.com,
> agl@golang.org (cc: golang-dev@googlegroups.com),
>

could you prepare the same patch against

 https://code.google.com/p/gosshnew/

?

I'm not sufficiently sure of my mercurial-foo to try cherry-picking it myself.

-- 
Han-Wen Nienhuys
Google Munich
hanwen@google.com