code review 6655046: go.crypto/ssh: add terminal modes to ssh.RequestPty()

go.crypto/ssh: add terminal modes to ssh.RequestPty()

[dave_cheney.net, 2012-10-12 05:25:47]

Thank you very much. This looks like a good start.

In addition to these comments, I suggest adding a few sample ModeLists, ie, the
EmptyModeList, for non interactive sessions, and some sample ones that would get
someone going with an interactive terminal.

Think about adding a test that at least uses this function. If there isn't the
facility in our ssh server to verify the modelist, maybe try the ssh/test
subpackage.

https://codereview.appspot.com/6655046/diff/2001/ssh/session.go
File ssh/session.go (right):

https://codereview.appspot.com/6655046/diff/2001/ssh/session.go#newcode54
ssh/session.go:54: const (
Luckily these will be coerced into a byte when they are used in the
append([]byte, byte) statement, so you can drop the uint8's

https://codereview.appspot.com/6655046/diff/2001/ssh/session.go#newcode188
ssh/session.go:188: func (s *Session) RequestPty(term string, h, w int,
termmodes TerminalModes) error {
I'm not sure a map is the right data structure for this. I would make a Mode
struct, then callers can use the compact literal syntax

http://play.golang.org/p/3u7T3PFm6K

https://codereview.appspot.com/6655046/diff/2001/ssh/session.go#newcode194
ssh/session.go:194: termModes = append(termModes, TTY_OP_END)
As this is the only mode that takes no value, I don't think you should export a
constant for it, ie, make it tty_OP_ENV.

[willemvds, 2012-10-12 06:10:56]

https://codereview.appspot.com/6655046/diff/2001/ssh/session.go
File ssh/session.go (right):

https://codereview.appspot.com/6655046/diff/2001/ssh/session.go#newcode188
ssh/session.go:188: func (s *Session) RequestPty(term string, h, w int,
termmodes TerminalModes) error {
I was not sure about the map type either but I do think it works out quite
nicely:
1) Creation syntax is simple
2) Modifying the modes after creation is easy and duplicates are automatically
handled

http://play.golang.org/p/v2ftpYKipb

On 2012/10/12 05:25:47, dfc wrote:
> I'm not sure a map is the right data structure for this. I would make a Mode
> struct, then callers can use the compact literal syntax
> 
> http://play.golang.org/p/3u7T3PFm6K
>

[dave_cheney.net, 2012-10-12 06:14:23]

https://codereview.appspot.com/6655046/diff/2001/ssh/session.go
File ssh/session.go (right):

https://codereview.appspot.com/6655046/diff/2001/ssh/session.go#newcode188
ssh/session.go:188: func (s *Session) RequestPty(term string, h, w int,
termmodes TerminalModes) error {
Fair enough. Well put. Does passing a nil here imply the old emptyModeList ?

[willemvds, 2012-10-15 05:45:06]

https://codereview.appspot.com/6655046/diff/2001/ssh/session.go
File ssh/session.go (right):

https://codereview.appspot.com/6655046/diff/2001/ssh/session.go#newcode188
ssh/session.go:188: func (s *Session) RequestPty(term string, h, w int,
termmodes TerminalModes) error {
Correct. Passing ssh.TerminalModes{} or nil will both have the same effect as
the old emptyModeList.

On 2012/10/12 06:14:23, dfc wrote:
> Fair enough. Well put. Does passing a nil here imply the old emptyModeList ?

[dave_cheney.net, 2012-10-16 23:07:15]

This is looking very good. One more round should do it.

https://codereview.appspot.com/6655046/diff/16001/ssh/example_test.go
File ssh/example_test.go (right):

https://codereview.appspot.com/6655046/diff/16001/ssh/example_test.go#newcode154
ssh/example_test.go:154: // Create client config
Skip everything down to // Set up terminal modes

https://codereview.appspot.com/6655046/diff/16001/ssh/example_test.go#newcode175
ssh/example_test.go:175: ECHO:          0,
for examples sake, please annotate each of these, ie

ECHO: 0 // do not echo
TTY_OP_ISPEED: 14400 // 14.4 kbaud

https://codereview.appspot.com/6655046/diff/16001/ssh/session.go
File ssh/session.go (right):

https://codereview.appspot.com/6655046/diff/16001/ssh/session.go#newcode189
ssh/session.go:189: termModes := make([]byte, 0, 276)
Is 276 a magic number ?

https://codereview.appspot.com/6655046/diff/16001/ssh/test/session_test.go
File ssh/test/session_test.go (right):

https://codereview.appspot.com/6655046/diff/16001/ssh/test/session_test.go#ne...
ssh/test/session_test.go:160: bytes := make([]byte, 1024)
I would use io.Copy into a bytes.Buffer, then rc <- buf.String()

[dave_cheney.net, 2012-10-18 08:59:05]

Excellent. I think this is good to go now. Can you please do a hg mail 6655046
so it is published to golang-dev.

The authors will also need to do a bit of paperwork behind the scenes to add
your name to the A+C files.

https://codereview.appspot.com/6655046/diff/21001/ssh/test/session_test.go
File ssh/test/session_test.go (right):

https://codereview.appspot.com/6655046/diff/21001/ssh/test/session_test.go#ne...
ssh/test/session_test.go:158: go func() {
you can also do

var buf bytes.Buffer
if _, err := io.Copy(&buf, stdout) { 
    t.Fatalf("reading failed: %s", err)
}

[willemvds, 2012-10-18 13:00:17]

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

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

[agl1, 2012-10-18 14:02:59]

Have you signed the ICLA?

http://golang.org/doc/contribute.html#copyright

[willemvds, 2012-10-18 14:36:48]

I thought I already did, but I submitted it again just now to be sure.

On Thu, Oct 18, 2012 at 4:02 PM, <agl@golang.org> wrote:

> Have you signed the ICLA?
>
>
http://golang.org/doc/**contribute.html#copyright<http://golang.org/doc/contr...
>
>
>
https://codereview.appspot.**com/6655046/<https://codereview.appspot.com/6655...
>

[dave_cheney.net, 2012-10-21 05:17:02]

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

go.crypto/ssh: add terminal modes to ssh.RequestPty()

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

Committer: Dave Cheney <dave@cheney.net>

[dave_cheney.net, 2012-10-21 05:18:42]

Thanks Willem,

As it has been several months since we added to the ssh pkg API, and the first
breaking change in a while, would you please write a brief [ANN] to golang-nuts
explaining the change, what it does and how people can update their code.

Cheers

Dave

[remyoudompheng, 2013-07-20 21:23:21]

R=close