FAQ
https://codereview.appspot.com/12837048/diff/21001/ssh/client.go
File ssh/client.go (right):

https://codereview.appspot.com/12837048/diff/21001/ssh/client.go#newcode222
ssh/client.go:222: go io.Copy(agentSock, ch.stdout)
You need to EOF agentSock somehow here. Possibly via Close.

https://codereview.appspot.com/12837048/diff/21001/ssh/client.go#newcode224
ssh/client.go:224: go io.Copy(ch.stdin, agentSock)
You need to close ch.stdin after this.

https://codereview.appspot.com/12837048/

--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

  • Agl at Aug 26, 2013 at 5:27 pm
    https://codereview.appspot.com/12837048/diff/21001/ssh/client.go
    File ssh/client.go (right):

    https://codereview.appspot.com/12837048/diff/21001/ssh/client.go#newcode41
    ssh/client.go:41: // AgentConnector connects to the proper ssh-agent
    Rename AgentDialer, with method Dial. (This is similar to net.Dial,
    although the signature is different because no network and address is
    given.)

    https://codereview.appspot.com/12837048/diff/21001/ssh/client.go#newcode219
    ssh/client.go:219: // has enabled proxying
    missing )

    https://codereview.appspot.com/12837048/diff/21001/ssh/client.go#newcode418
    ssh/client.go:418: if c.config.ForwardingAgentConnector != nil {
    Seems that this condition could be inverted and an early return used to
    save a level of indentation in the following code.

    https://codereview.appspot.com/12837048/diff/21001/ssh/client.go#newcode543
    ssh/client.go:543: // Connector to the local ssh-agent for key
    forwarding.
    // AgentForwardingDialer contains a Dialer that is used to connect to
    the SSH agent. If nil, no key ...

    https://codereview.appspot.com/12837048/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Jpsugar at Aug 26, 2013 at 5:45 pm
    Also need periods at the ends of all the sentences in comments.

    https://codereview.appspot.com/12837048/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Jamwt at Aug 26, 2013 at 6:27 pm

    On 2013/08/26 17:45:22, jpsugar wrote:
    Also need periods at the ends of all the sentences in comments.
    Thanks for the review. I have made all those changes except
    short-circuiting the condition in the case statement. My reasoning is
    this:

    This would be the only case statement (currently) that aborts via
    return. Defensive coding would suggest _not_ returning to protect
    against a future author adding more code in the body of this function
    after the switch() statement on the assumption that all cases will
    continue execution of the function body after the case block.

    Thoughts?

    https://codereview.appspot.com/12837048/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Jamwt at Aug 26, 2013 at 6:28 pm

    On 2013/08/26 18:27:25, jamwt wrote:
    On 2013/08/26 17:45:22, jpsugar wrote:
    Also need periods at the ends of all the sentences in comments.
    Thanks for the review. I have made all those changes except
    short-circuiting
    the condition in the case statement. My reasoning is this:
    This would be the only case statement (currently) that aborts via return.
    Defensive coding would suggest _not_ returning to protect against a future
    author adding more code in the body of this function after the switch()
    statement on the assumption that all cases will continue execution of the
    function body after the case block.
    Thoughts?
    Nevermind, I withdraw this line of thinking. The forwarded-tcpip case
    already makes liberal use of return.

    Okay, I agree, I'll implement that change too.

    https://codereview.appspot.com/12837048/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Jamwt at Aug 26, 2013 at 6:32 pm

    On 2013/08/26 18:28:39, jamwt wrote:
    On 2013/08/26 18:27:25, jamwt wrote:
    On 2013/08/26 17:45:22, jpsugar wrote:
    Also need periods at the ends of all the sentences in comments.
    Thanks for the review. I have made all those changes except
    short-circuiting
    the condition in the case statement. My reasoning is this:

    This would be the only case statement (currently) that aborts via
    return.
    Defensive coding would suggest _not_ returning to protect against a
    future
    author adding more code in the body of this function after the
    switch()
    statement on the assumption that all cases will continue execution
    of the
    function body after the case block.

    Thoughts?
    Nevermind, I withdraw this line of thinking. The forwarded-tcpip case already
    makes liberal use of return.
    Okay, I agree, I'll implement that change too.
    In that spirit I've done an early return on the second error condition
    as well (failure to connect to the agent).

    https://codereview.appspot.com/12837048/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Jpsugar at Aug 26, 2013 at 7:13 pm
    https://codereview.appspot.com/12837048/diff/33002/ssh/client.go
    File ssh/client.go (right):

    https://codereview.appspot.com/12837048/diff/33002/ssh/client.go#newcode41
    ssh/client.go:41: // AgentDialer connects to the proper ssh-agent
    Un-wrap this comment now? Lines seem a little short.

    https://codereview.appspot.com/12837048/diff/33002/ssh/session.go
    File ssh/session.go (right):

    https://codereview.appspot.com/12837048/diff/33002/ssh/session.go#newcode229
    ssh/session.go:229: req := channelRequestMsg{
    Please run gofmt. These appear to be spaces.

    https://codereview.appspot.com/12837048/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Jamwt at Aug 26, 2013 at 7:41 pm

    On 2013/08/26 19:13:33, jpsugar wrote:
    https://codereview.appspot.com/12837048/diff/33002/ssh/client.go
    File ssh/client.go (right):

    https://codereview.appspot.com/12837048/diff/33002/ssh/client.go#newcode41
    ssh/client.go:41: // AgentDialer connects to the proper ssh-agent
    Un-wrap this comment now? Lines seem a little short.
    https://codereview.appspot.com/12837048/diff/33002/ssh/session.go
    File ssh/session.go (right):

    https://codereview.appspot.com/12837048/diff/33002/ssh/session.go#newcode229
    ssh/session.go:229: req := channelRequestMsg{
    Please run gofmt. These appear to be spaces.
    I have been running gofmt.. maybe I'm running the wrong one? `hg
    gofmt`. IME the codereview tool will refuse to upload if it determines
    gofmt needs to be run.

    https://codereview.appspot.com/12837048/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedAug 26, '13 at 4:56p
activeAug 26, '13 at 7:41p
posts8
users3
websitegolang.org

3 users in discussion

Jamwt: 4 posts Jpsugar: 3 posts Agl: 1 post

People

Translate

site design / logo © 2021 Grokbase