code review 5433080: exp/ssh: add Std{in,out,err}Pipe methods to Session

exp/ssh: add Std{in,out,err}Pipe methods to Session

[cw, 2011-11-28 17:24:32]

so far so good

http://codereview.appspot.com/5433080/diff/2001/src/pkg/exp/ssh/session.go
File src/pkg/exp/ssh/session.go (right):

http://codereview.appspot.com/5433080/diff/2001/src/pkg/exp/ssh/session.go#ne...
src/pkg/exp/ssh/session.go:298: return nil, errors.New("ssh: Stdin already set")
i'm on the fence about using errors.New here vs something that might return
ssh.Err

[gpaul, 2011-11-29 06:32:02]

LGTM. I look forward to using this. I've been setting session.std{out/err}
to buffers and calling Read() on those. This is much nicer.

Regards,
Gustav

On Mon, Nov 28, 2011 at 7:24 PM, <cw@f00f.org> wrote:

> so far so good
>
>
> http://codereview.appspot.com/**5433080/diff/2001/src/pkg/exp/**
>
ssh/session.go<http://codereview.appspot.com/5433080/diff/2001/src/pkg/exp/ssh/session.go>
> File src/pkg/exp/ssh/session.go (right):
>
> http://codereview.appspot.com/**5433080/diff/2001/src/pkg/exp/**
>
ssh/session.go#newcode298<http://codereview.appspot.com/5433080/diff/2001/src/pkg/exp/ssh/session.go#newcode298>
> src/pkg/exp/ssh/session.go:**298: return nil, errors.New("ssh: Stdin
> already set")
> i'm on the fence about using errors.New here vs something that might
> return ssh.Err
>
>
http://codereview.appspot.com/**5433080/<http://codereview.appspot.com/5433080/>
>

[gpaul, 2011-11-29 06:56:47]

On 2011/11/29 06:32:02, gpaul wrote:
> LGTM. I look forward to using this. I've been setting session.std{out/err}
> to buffers and calling Read() on those. This is much nicer.
> 
> Regards,
> Gustav
> 
> On Mon, Nov 28, 2011 at 7:24 PM, <mailto:cw@f00f.org> wrote:
> 
> > so far so good
> >
> >
> > http://codereview.appspot.com/**5433080/diff/2001/src/pkg/exp/**
> >
>
ssh/session.go<http://codereview.appspot.com/5433080/diff/2001/src/pkg/exp/ssh/session.go>
> > File src/pkg/exp/ssh/session.go (right):
> >
> > http://codereview.appspot.com/**5433080/diff/2001/src/pkg/exp/**
> >
>
ssh/session.go#newcode298<http://codereview.appspot.com/5433080/diff/2001/src/pkg/exp/ssh/session.go#newcode298>
> > src/pkg/exp/ssh/session.go:**298: return nil, errors.New("ssh: Stdin
> > already set")
> > i'm on the fence about using errors.New here vs something that might
> > return ssh.Err
> >
> >
>
http://codereview.appspot.com/**5433080/%3Chttp://codereview.appspot.com/5433...>
> >

Just noticed,
on client_auth_test.go:199
I suggest using pw on line 156 in your CL as you do in TestClientAuthPassword

[dave_cheney.net, 2011-11-29 10:52:56]

Hello gustav.paul@gmail.com, cw@f00f.org, agl@golang.org, rsc@golang.org (cc:
golang-dev@googlegroups.com),

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

[niemeyer, 2011-11-29 12:13:01]

Nice change.. we'll definitely make some good use of this in the near future.

Some comments on the testing only.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test.go
File src/pkg/exp/ssh/session_test.go (right):

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:31: t.Fatal(err)
Fatal doesn't do what you want here. It won't kill the test, and will instead
kill its own goroutine, while the test moves on.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:37: C, err := c.Accept()
I suggest s/c/conn/ and s/C/ch/. The single letters here aren't helping.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:42: t.Fatal(err)
Fatal won't work well here either.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:67:
ch.serverConn.writePacket(marshal(msgChannelRequest, msg))
Interesting.. separate CL, but we need a way to do this without going out of the
public API.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:73: defer l.Close()
These defers should be after the respective value creation. At the end of the
function is too late to defer.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:106: actual := string(stdout.Bytes())
stdout.String()

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:120: stdout, err := session.StdoutPipe()
Would be nice to have testing for the other pipe functions too.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:131: t.Fatalf("Copy of stdout failed: %v", err)
Fatalf here will cause the goroutine to exit, and the channel send below will
never be done. The actual test goroutine will block forever without a note.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:139: actual := string(buf.Bytes())
buf.String()

[dave_cheney.net, 2011-11-30 10:32:58]

Hello gustav.paul@gmail.com, cw@f00f.org, agl@golang.org, rsc@golang.org,
n13m3y3r@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dave_cheney.net, 2011-11-30 10:34:32]

Thanks for your comments Gustavo. I will continue to flesh out the Std...Pipe
tests. I think in that respect I have overstepped what can be done reusing the
ServerShell code.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test.go
File src/pkg/exp/ssh/session_test.go (right):

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:31: t.Fatal(err)
Right, that is good to know. Thanks.

While it doesn't do what I wanted, I think it's still good enough as the client
will fail to complete properly.

On 2011/11/29 12:13:01, niemeyer wrote:
> Fatal doesn't do what you want here. It won't kill the test, and will instead
> kill its own goroutine, while the test moves on.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:37: C, err := c.Accept()
On 2011/11/29 12:13:01, niemeyer wrote:
> I suggest s/c/conn/ and s/C/ch/. The single letters here aren't helping.

Done.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:42: t.Fatal(err)
On 2011/11/29 12:13:01, niemeyer wrote:
> Fatal won't work well here either.

Done.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:67:
ch.serverConn.writePacket(marshal(msgChannelRequest, msg))
Yeah, it's bit messy. TODO added

On 2011/11/29 12:13:01, niemeyer wrote:
> Interesting.. separate CL, but we need a way to do this without going out of
the
> public API.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:73: defer l.Close()
On 2011/11/29 12:13:01, niemeyer wrote:
> These defers should be after the respective value creation. At the end of the
> function is too late to defer.

Done.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:106: actual := string(stdout.Bytes())
On 2011/11/29 12:13:01, niemeyer wrote:
> stdout.String()

Done.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:120: stdout, err := session.StdoutPipe()
PTAL.

On 2011/11/29 12:13:01, niemeyer wrote:
> Would be nice to have testing for the other pipe functions too.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:131: t.Fatalf("Copy of stdout failed: %v", err)
I've changed it to Errorf, the done will fire and the buffer will not match the
expected text.

On 2011/11/29 12:13:01, niemeyer wrote:
> Fatalf here will cause the goroutine to exit, and the channel send below will
> never be done. The actual test goroutine will block forever without a note.

http://codereview.appspot.com/5433080/diff/8003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:139: actual := string(buf.Bytes())
On 2011/11/29 12:13:01, niemeyer wrote:
> buf.String()

Done.

[niemeyer, 2011-11-30 14:18:38]

LGTM, with only a few minors.

Btw, is Output and CombinedOutput in your upcoming plans?

http://codereview.appspot.com/5433080/diff/7003/src/pkg/exp/ssh/session_test.go
File src/pkg/exp/ssh/session_test.go (right):

http://codereview.appspot.com/5433080/diff/7003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:31: t.Fatalf("Unable to accept: %v", err)
This still feels bogus. It's misguiding at best. I suggest at least replacing
this by Errorf + return, so it's clear that the test will _not_ stop running
here.

http://codereview.appspot.com/5433080/diff/7003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:35: t.Fatal("Unable to handshake: %v", err)
Same.

http://codereview.appspot.com/5433080/diff/7003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:43: t.Fatal(err)
Same.

http://codereview.appspot.com/5433080/diff/7003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:106: actual := string(stdout.String())
s/string(s)/s/

http://codereview.appspot.com/5433080/diff/7003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:142: actual := string(buf.String())
s/string(s)/s/

[rsc, 2011-11-30 20:25:52]

This is kind of odd.  The reason we reversed the
API was so that the ssh session would not block:
it could always pull stdin and push stdout/stderr
as needed.  Inserting an io.Pipe here (which is
synchronous) restores the old API and its problems,
but now with an extraneous goroutine in the middle.

I certainly understand that the API is useful, but it
should bypass the goroutine instead of reversing it
with an io.Pipe, and it should be very clearly documented
that if you do not read from Stdout/Stderr fast enough
the SSH session will die.

[dave_cheney.net, 2011-11-30 20:52:03]

Thanks for your comments Gustavo. I hadn't intended to add Output and
CombinedOutput, but it should be pretty straight forward to add them if you were
feeling like it. Otherwise I'll put it on my TODO list.

http://codereview.appspot.com/5433080/diff/7003/src/pkg/exp/ssh/session_test.go
File src/pkg/exp/ssh/session_test.go (right):

http://codereview.appspot.com/5433080/diff/7003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:31: t.Fatalf("Unable to accept: %v", err)
On 2011/11/30 14:18:38, niemeyer wrote:
> This still feels bogus. It's misguiding at best. I suggest at least replacing
> this by Errorf + return, so it's clear that the test will _not_ stop running
> here.

Done.

http://codereview.appspot.com/5433080/diff/7003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:35: t.Fatal("Unable to handshake: %v", err)
On 2011/11/30 14:18:38, niemeyer wrote:
> Same.

Done.

http://codereview.appspot.com/5433080/diff/7003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:43: t.Fatal(err)
On 2011/11/30 14:18:38, niemeyer wrote:
> Same.

Done.

http://codereview.appspot.com/5433080/diff/7003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:106: actual := string(stdout.String())
Doh. Done

On 2011/11/30 14:18:38, niemeyer wrote:
> s/string(s)/s/

http://codereview.appspot.com/5433080/diff/7003/src/pkg/exp/ssh/session_test....
src/pkg/exp/ssh/session_test.go:142: actual := string(buf.String())
On 2011/11/30 14:18:38, niemeyer wrote:
> s/string(s)/s/

Done.

[dave_cheney.net, 2011-11-30 21:07:08]

On 2011/11/30 20:25:52, rsc wrote:
> This is kind of odd.  The reason we reversed the
> API was so that the ssh session would not block:
> it could always pull stdin and push stdout/stderr
> as needed.  Inserting an io.Pipe here (which is
> synchronous) restores the old API and its problems,
> but now with an extraneous goroutine in the middle.

I never intended the API reversal to fix the potential blocking issues with
stdout/err. Granted, under most circumstances it's made the situation better by
providing the draining goroutine for you, and making it easy to provide targets
for that data that don't block. But I wouldn't claim that it's fixed the
problem.

> I certainly understand that the API is useful, but it
> should bypass the goroutine instead of reversing it
> with an io.Pipe, 

I agree, I think I can remove all of the Copy an Pipe conversions, but I need to
do a bit more work behind the scenes. It's coming up next after this CL.

> and it should be very clearly documented
> that if you do not read from Stdout/Stderr fast enough
> the SSH session will die.

I will add a comment to the Pipe() methods to reflect that.

[dave_cheney.net, 2011-11-30 21:14:10]

Hello gustav.paul@gmail.com, cw@f00f.org, agl@golang.org, rsc@golang.org,
n13m3y3r@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-11-30 21:25:08]

LGTM.  Leaving for Gustavo.

[niemeyer, 2011-12-01 10:21:06]

LGTM

Please just fix this comment and I'll submit:

http://codereview.appspot.com/5433080/diff/7005/src/pkg/exp/ssh/session.go
File src/pkg/exp/ssh/session.go (right):

http://codereview.appspot.com/5433080/diff/7005/src/pkg/exp/ssh/session.go#ne...
src/pkg/exp/ssh/session.go:57: started        bool // true once a Shell or Exec
is invoked.
// true once Start, Run, or Shell is invoked.

[dave_cheney.net, 2011-12-01 10:23:42]

Hello gustav.paul@gmail.com, cw@f00f.org, agl@golang.org, rsc@golang.org,
n13m3y3r@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[niemeyer, 2011-12-01 10:30:24]

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

exp/ssh: add Std{in,out,err}Pipe methods to Session

R=gustav.paul, cw, agl, rsc, n13m3y3r
CC=golang-dev
http://codereview.appspot.com/5433080

Committer: Gustavo Niemeyer <gustavo@niemeyer.net>