Re: code review 15380048: go.crypto/ssh: use a separate channel for out-of-band (issue 15380048)

[da...@cheney.net]

On 2013/10/23 22:41:30, jpsugar wrote:
> LGTM

Can you please hg mail

% hg clpatch 15380048
abort: codereview issue 15380048 is out of date: patch and recent
changes conflict (e3c12cab1b35->213a06a7ce81)

I'd also like agl to sign off on this as the Server is his baby and this
is a change to the server api.

https://codereview.appspot.com/15380048/

[han...@google.com]

Hello a...@golang.org, da...@cheney.net, jps...@google.com (cc:
golan...@googlegroups.com),

Please take another look.


https://codereview.appspot.com/15380048/

[han...@google.com]

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go#newcode45
ssh/channel.go:45:
I have an API question:

should we put the Ack method on the Request rather than on the Channel,
eg.

type Request struct {
Name string
Payload []byte
WantReply bool

ch *channel
}

func (r *Request) Reply(ok bool, payload []byte) error {
if !r.WantReply { /* error */ }
return r.ch.sendMessage( .. )
}

this makes the API simpler, as it makes it more obvious when AckRequest
can be run.

https://codereview.appspot.com/15380048/

[da...@cheney.net]

Right now Channel is specific to ServerConns, if removing AckRequest
from that interface brings us closer to making that interface less
specific to Server connections, sounds like a good change.

https://codereview.appspot.com/15380048/

[da...@cheney.net]

Small nit.

Still hoping to see a comment from agl.


https://codereview.appspot.com/15380048/diff/60001/ssh/session_test.go
File ssh/session_test.go (right):

https://codereview.appspot.com/15380048/diff/60001/ssh/session_test.go#newcode645
ssh/session_test.go:645: _ = <-ch.IncomingRequests()
s/_ =//

https://codereview.appspot.com/15380048/

[a...@golang.org]

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go#newcode28
ssh/channel.go:28: // SSH connection.
This change seems to suck for a user of this package. Now I have to spin
off a goroutine to manage requests and, if I forget, the channel fills
up and everything grinds to a halt?

https://codereview.appspot.com/15380048/

[han...@google.com]

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go#newcode28
ssh/channel.go:28: // SSH connection.

On 2013/10/25 16:52:12, agl1 wrote:
> This change seems to suck for a user of this package. Now I have to
spin off a
> goroutine to manage requests and, if I forget, the channel fills up
and
> everything grinds to a halt?

A couple of solutions:

1. we buffer incoming requests indefinitely if nobody calls
IncomingRequests()? Then it won't come to a halt, and it will simplify
most use-cases. The disadvantage is that if the remote side sends some
system specific request ("keepalive@randomvendor"), the user still needs
to handle it, and there is nothing that tells him.

2. We let the user register callbacks based on the request type string.
This lets us send failure messages to unknown WantReply requests, so we
will handle "keepalive@randomvendor" correctly. We could either do

type RequestHandler func(incoming *Request) error
func (c *channel) RegisterHandler(reqType string, handler
RequestHandler)

or

func (c *channel) HandleRequest(reqType string) chan *Request


3. We do nothing. This is not very practical, because as it stands you
cannot practically pass off a Channel as io.Reader / io.Writer, since
things like io.ReadFull() can't handle ChannelRequests.


I'm leaning towards #2. What do you think?




I could make it instantiate the channel on demand, ie. if nothing calls
IncomingRequests(), then we have an internal buffer which fills up.

https://codereview.appspot.com/15380048/

[Han-Wen Nienhuys]

Channel is not instantiated for the client side, but they're actually
the same. Clients may send and receive requests too (but we've so far
hidden this internally).

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

[Han-Wen Nienhuys]

Let's continue discussion in the doc I set up

https://docs.google.com/document/d/1yVmyfDxWbnIhDkk4ypNSVjV8KL_GOsfCcr-ZcDpDup4/edit?disco=AAAAAG9xdfA

[a...@golang.org]

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go
File ssh/channel.go (right):

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go#newcode28
ssh/channel.go:28: // SSH connection.

On 2013/10/25 18:31:09, hanwen-google wrote:
> On 2013/10/25 16:52:12, agl1 wrote:
> > This change seems to suck for a user of this package. Now I have to
spin off a
> > goroutine to manage requests and, if I forget, the channel fills up
and
> > everything grinds to a halt?

I see the desire to be able to pass the io.Readers off to other code,
even though it's not a case that I care about.

However, you're actually using the code for real things and I'm not so
I'll take your needs in preference. Thus I think the current code is ok.

However, this is going to break the API and, if you are following the
doc that you linked to, then it's going to continue breaking several
more times in ways that are likely to affect users. (The breakages so
far have either passed me by, or have been more minor.)

Does this call for a copy in go.crypto/ssh/new during the change, or
even in an internal repo? (I would love to use Critique for this.)

That way, the changes can be stablised before switching over and there's
only a single, big, break.

https://codereview.appspot.com/15380048/diff/120001/ssh/channel.go#newcode45
ssh/channel.go:45:

On 2013/10/24 23:54:44, hanwen-google wrote:
> I have an API question:

> should we put the Ack method on the Request rather than on the
Channel, eg.

> type Request struct {
> Name string
> Payload []byte
> WantReply bool

> ch *channel
> }

> func (r *Request) Reply(ok bool, payload []byte) error {
> if !r.WantReply { /* error */ }
> return r.ch.sendMessage( .. )
> }

> this makes the API simpler, as it makes it more obvious when
AckRequest can be
> run.

I think this makes sense.

https://codereview.appspot.com/15380048/

[han...@google.com]

as discussed, we'll work on the gonewssh repo for now, so I'm abandoning
this change.

https://codereview.appspot.com/15380048/

[han...@google.com]

*** Abandoned ***

https://codereview.appspot.com/15380048/