code review 5132041: exp/ssh: move common code to common.go

exp/ssh: move common code to common.go

[dave_cheney.net, 2011-09-24 08:21:51]

Hello agl@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[agl1, 2011-09-24 18:11:58]

I suspect that decode can be cleaned up, otherwise looks good.

http://codereview.appspot.com/5132041/diff/13001/src/pkg/exp/ssh/messages.go
File src/pkg/exp/ssh/messages.go (right):

http://codereview.appspot.com/5132041/diff/13001/src/pkg/exp/ssh/messages.go#...
src/pkg/exp/ssh/messages.go:576: case msgDisconnect:
can we not just say

var msg interface{}

at the toplevel of the function, followed by:

switch packet[0] {
case msgDisconnect:
  msg = new(disconnectMsg)
case foo:
...
}

and then

if err := unmarshal(msg, packet, packet[0]); err != nil {
  ...
}

http://codereview.appspot.com/5132041/diff/13001/src/pkg/exp/ssh/server.go
File src/pkg/exp/ssh/server.go (left):

http://codereview.appspot.com/5132041/diff/13001/src/pkg/exp/ssh/server.go#ol...
src/pkg/exp/ssh/server.go:654: case msgChannelData:
I had deliberately processed this without using unmarshal on the basis that it's
probably about ~50% of messages and just worth special casing. On the other
hand, we can always special case it later.

[dave_cheney.net, 2011-09-25 03:13:42]

Thank you for your comments. PTAL

http://codereview.appspot.com/5132041/diff/13001/src/pkg/exp/ssh/messages.go
File src/pkg/exp/ssh/messages.go (right):

http://codereview.appspot.com/5132041/diff/13001/src/pkg/exp/ssh/messages.go#...
src/pkg/exp/ssh/messages.go:576: case msgDisconnect:
Yes, of course, that is much cleaner. Thank you

On 2011/09/24 18:11:58, agl1 wrote:
> can we not just say
> 
> var msg interface{}
> 
> at the toplevel of the function, followed by:
> 
> switch packet[0] {
> case msgDisconnect:
>   msg = new(disconnectMsg)
> case foo:
> ...
> }
> 
> and then
> 
> if err := unmarshal(msg, packet, packet[0]); err != nil {
>   ...
> }

http://codereview.appspot.com/5132041/diff/13001/src/pkg/exp/ssh/server.go
File src/pkg/exp/ssh/server.go (left):

http://codereview.appspot.com/5132041/diff/13001/src/pkg/exp/ssh/server.go#ol...
src/pkg/exp/ssh/server.go:654: case msgChannelData:
I think there is still value in allocating a msgChannelData struct for it, even
if that doesn't go though unmarshall. 

On 2011/09/24 18:11:58, agl1 wrote:
> I had deliberately processed this without using unmarshal on the basis that
it's
> probably about ~50% of messages and just worth special casing. On the other
> hand, we can always special case it later.

[agl1, 2011-09-26 14:25:33]

*** Submitted as http://code.google.com/p/go/source/detail?r=3299d0ca626a ***

exp/ssh: move common code to common.go

R=agl
CC=golang-dev
http://codereview.appspot.com/5132041

Committer: Adam Langley <agl@golang.org>

http://codereview.appspot.com/5132041/diff/18001/src/pkg/exp/ssh/server.go
File src/pkg/exp/ssh/server.go (right):

http://codereview.appspot.com/5132041/diff/18001/src/pkg/exp/ssh/server.go#ne...
src/pkg/exp/ssh/server.go:641: panic(fmt.Sprintf("unknown message: %#v", msg))
I'm going to revert this line before landing rather than add another round trip
to the whole code review. I think this makes it far too easy to panic the
server.

For debugging it's certainly useful to print something, but that can be done
locally when developing and I think println("unknown message:\n" +
hex.Dump(msg)) is clearer in any case.

You're welcome to bring it up in future reviews if you differ on this point.


Cheers

[dave_cheney.net, 2011-09-26 22:13:12]

Good call, arguably the panic was a left over debugging feature. I'll add a unit
test to verify that all message types are digestible. 

Sent from my iPhone

On 27/09/2011, at 0:25, agl@golang.org wrote:

> *** Submitted as
> http://code.google.com/p/go/source/detail?r=3299d0ca626a ***
> 
> exp/ssh: move common code to common.go
> 
> R=agl
> CC=golang-dev
> http://codereview.appspot.com/5132041
> 
> Committer: Adam Langley <agl@golang.org>
> 
> 
> 
> http://codereview.appspot.com/5132041/diff/18001/src/pkg/exp/ssh/server.go
> File src/pkg/exp/ssh/server.go (right):
> 
>
http://codereview.appspot.com/5132041/diff/18001/src/pkg/exp/ssh/server.go#ne...
> src/pkg/exp/ssh/server.go:641: panic(fmt.Sprintf("unknown message: %#v",
> msg))
> I'm going to revert this line before landing rather than add another
> round trip to the whole code review. I think this makes it far too easy
> to panic the server.
> 
> For debugging it's certainly useful to print something, but that can be
> done locally when developing and I think println("unknown message:\n" +
> hex.Dump(msg)) is clearer in any case.
> 
> You're welcome to bring it up in future reviews if you differ on this
> point.
> 
> 
> Cheers
> 
> http://codereview.appspot.com/5132041/