Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(459)

Issue 5986053: code review 5986053: go.crypto/ssh: replace window channel with an atomic va... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 12 months ago by dfc
Modified:
11 years, 11 months ago
Reviewers:
CC:
golang-dev, agl1, gpaul, kardia, dvyukov
Visibility:
Public.

Description

go.crypto/ssh: replace window channel with an atomic variable and condition Fixes issue 3479. Using a channel to model window size was a mistake. Unlike stdin and stdout, which are streams of data, window size is an variable and should be modeled as such.

Patch Set 1 #

Patch Set 2 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #

Patch Set 3 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #

Patch Set 4 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #

Patch Set 5 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #

Patch Set 6 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #

Total comments: 4

Patch Set 7 : diff -r 45667c991e8e https://code.google.com/p/go.crypto #

Total comments: 6

Patch Set 8 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #

Patch Set 9 : diff -r 91ccd0bd392e https://code.google.com/p/go.crypto #

Total comments: 6

Patch Set 10 : diff -r 45667c991e8e https://code.google.com/p/go.crypto #

Patch Set 11 : diff -r 45667c991e8e https://code.google.com/p/go.crypto #

Patch Set 12 : diff -r 45667c991e8e https://code.google.com/p/go.crypto #

Unified diffs Side-by-side diffs Delta from patch set Stats (+54 lines, -16 lines) Patch
M ssh/client.go View 1 2 3 4 5 6 7 8 9 10 8 chunks +54 lines, -16 lines 0 comments Download

Messages

Total messages: 18
dfc
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go.crypto
11 years, 11 months ago (2012-04-08 11:58:25 UTC) #1
gpaul
LGTM. I haven't been able to reproduce this issue, even when writing >65MB over stdin. ...
11 years, 11 months ago (2012-04-08 15:00:28 UTC) #2
kardia
LGTM. In another CL, I think in chanReader, we should replace the chan []byte buffer ...
11 years, 11 months ago (2012-04-08 17:30:26 UTC) #3
dfc
Hello golang-dev@googlegroups.com, agl@golang.org, gustav.paul@gmail.com, kardianos@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 11 months ago (2012-04-09 01:46:42 UTC) #4
dfc
Thank you for your comments. I've converted the window size to a uint32, there are ...
11 years, 11 months ago (2012-04-09 01:49:06 UTC) #5
agl1
LGTM with a couple of updates. http://codereview.appspot.com/5986053/diff/7003/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/7003/ssh/client.go#newcode257 ssh/client.go:257: c.getChan(msg.PeersId).stdin.win.add(msg.AdditionalBytes) if !c.getChan(msg.PeersId).stdin.win.add(...) ...
11 years, 11 months ago (2012-04-09 14:40:06 UTC) #6
dfc
Hello golang-dev@googlegroups.com, agl@golang.org, gustav.paul@gmail.com, kardianos@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 11 months ago (2012-04-14 03:37:06 UTC) #7
dfc
Thank you for your comments agl. PTAL. Gustav, would you be able to reconfirm that ...
11 years, 11 months ago (2012-04-14 03:40:02 UTC) #8
gpaul
Certainly. I'll let you know by tomorrow. On Sat, Apr 14, 2012 at 5:40 AM, ...
11 years, 11 months ago (2012-04-14 06:18:32 UTC) #9
dvyukov
http://codereview.appspot.com/5986053/diff/9002/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/9002/ssh/client.go#newcode518 ssh/client.go:518: w.Signal() Can't reserve() be called concurrently? http://codereview.appspot.com/5986053/diff/9002/ssh/client.go#newcode529 ssh/client.go:529: w.Wait() ...
11 years, 11 months ago (2012-04-15 06:51:10 UTC) #10
dfc
Hello golang-dev@googlegroups.com, agl@golang.org, gustav.paul@gmail.com, kardianos@gmail.com, dvyukov@google.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 11 months ago (2012-04-15 07:00:27 UTC) #11
dfc
Thank you for your comments Dmity. PTAL. http://codereview.appspot.com/5986053/diff/9002/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/9002/ssh/client.go#newcode518 ssh/client.go:518: w.Signal() Not ...
11 years, 11 months ago (2012-04-15 07:01:01 UTC) #12
dvyukov
http://codereview.appspot.com/5986053/diff/9002/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/9002/ssh/client.go#newcode518 ssh/client.go:518: w.Signal() On 2012/04/15 07:01:02, dfc wrote: > Not quite ...
11 years, 11 months ago (2012-04-15 07:41:34 UTC) #13
gpaul
The latest update still fixes the issue: I am able to correctly transfer >64MB. I'm ...
11 years, 11 months ago (2012-04-15 08:06:40 UTC) #14
dfc
Hello golang-dev@googlegroups.com, agl@golang.org, gustav.paul@gmail.com, kardianos@gmail.com, dvyukov@google.com (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 11 months ago (2012-04-15 09:37:37 UTC) #15
dfc
Thank you for your review. Please take another (final?) look. http://codereview.appspot.com/5986053/diff/9002/ssh/client.go File ssh/client.go (right): http://codereview.appspot.com/5986053/diff/9002/ssh/client.go#newcode518 ...
11 years, 11 months ago (2012-04-15 09:38:18 UTC) #16
dvyukov
FWIW LGTM
11 years, 11 months ago (2012-04-15 10:22:13 UTC) #17
dfc
11 years, 11 months ago (2012-04-15 10:30:52 UTC) #18
*** Submitted as
http://code.google.com/p/go/source/detail?r=b42815235d16&repo=crypto ***

go.crypto/ssh: replace window channel with an atomic variable and condition

Fixes issue 3479.

Using a channel to model window size was a mistake. Unlike stdin and
stdout, which are streams of data, window size is an variable and
should be modeled as such.

R=golang-dev, agl, gustav.paul, kardianos, dvyukov
CC=golang-dev
http://codereview.appspot.com/5986053
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b