Opened 9 years ago

Closed 5 years ago

#2997 defect closed fixed (fixed)

conch and getPeer

Reported by: bernat@… Owned by: z3p
Priority: normal Milestone:
Component: conch Keywords:
Cc: Branch: branches/conch-session-getPeer-2997
branch-diff, diff-cov, branch-cov, buildbot
Author: z3p

Description (last modified by Thijs Triemstra)

I would like to use getPeer from conch but there is no easy way of this. For example, if I take and apply the following patch:


    old new  
    3030    """this is our example protocol that we will run over SSH
    3131    """
    3232    def dataReceived(self, data):
     33        print self.transport.getPeer()
    3334        if data == '\r':
    3435            data = '\r\n'
    3536        elif data == '\x03': #^C

I get:

        exceptions.AttributeError: SSHSessionProcessProtocol instance has no attribute 'getPeer'

The solution seems to be to use self.transport.session.getPeer(). I think that getPeer method should be implemented in SSHSessionProcessProtocol (to be able to use a protocol with SSH or without SSH for example).

In fact, there seems to be a bug in SSHChannel. When using self.transport.session.getPeer(), I get :

          File "/usr/lib/python2.4/site-packages/twisted/conch/ssh/", line 219, in getPeer
            return('SSH', )+self.conn.transport.getPeer()
        exceptions.AttributeError: SSHServerTransport instance has no attribute 'getPeer'

The fix should be to use self.conn.transport.transport.getPeer().

Change History (13)

comment:1 Changed 9 years ago by bernat@…

The first part of this bug is in fact already in #2453. Sorry for that. The second part is still valid though.

comment:2 Changed 5 years ago by z3p

Author: z3p
Branch: branches/conch-session-getPeer-2997

(In [33133]) Branching to 'conch-session-getPeer-2997'

comment:4 Changed 5 years ago by z3p

Keywords: review added

Forgot the review keyword.

comment:5 Changed 5 years ago by z3p

Owner: z3p deleted

I shouldn't review my own ticket.

comment:6 Changed 5 years ago by Thijs Triemstra

Description: modified (diff)

comment:7 Changed 5 years ago by Itamar Turner-Trauring

Keywords: review removed
Owner: set to z3p

Glad you're back to working on conch!

Major issues:

  1. The tests looked like they failed on the Windows build slaves.

Minor Issues:

  1. You're doing (in 'SSHTransportAddress(%r)' % self.address. Better to always do 'SSHTransportAddress(%r)' % (self.address,) in case some jerk decides to pass in a tuple.

Coding standard issues:

  1. You're missing a news file in twisted/conch/topfiles/.
  2. The new address test class should have a docstring.
  3. Two lines between each method in all the code you added, rather than one.
  4. In test docstrings you're using L{code} to refer to objects that don't exist at the given import path; L is supposed to be for links, to objects that are actually in that python module at that full or relative Python path. Use C{} for arbitrary code snippets and references. In practice this was in tests, where docstring correctness doesn't matter.
  5. Some lines in are longer than 80 chars.
  6. @ivar secondary lines ( are usually indented, though I have no idea if that's actually required.

comment:8 Changed 5 years ago by Jean-Paul Calderone

Owner: changed from z3p to teratorn

#5566 was a duplicate of this.

comment:9 Changed 5 years ago by z3p

Keywords: review added
Owner: teratorn deleted

I believe I've addressed those review comments in the latest commit. I'm having trouble loading BuildBot at the moment, but I did start the builds:

comment:10 Changed 5 years ago by therve

Keywords: review removed
Owner: set to z3p
  1. Pyflakes: twisted/conch/test/ local variable 'testCipher' is assigned to but never used
  1. It's better to only import SSHTransportAddress instead of the whole address module if you only use that object.
  1. Please add @since marker to your new module and your 2 new methods.
  1. Please put 3 lines after the imports and before your first class.

All very minor, please merge once fixed. Thanks!

comment:11 Changed 5 years ago by z3p

(In [34443]) comment 1: pyflakes/80cols

Refs #2997

comment:12 Changed 5 years ago by z3p

(In [34445]) comment 4: 3 lines between imports and first class

Refs #2997

comment:13 Changed 5 years ago by z3p

Resolution: fixed
Status: newclosed

(In [34448]) Merge conch-session-getPeer-2997

Author: z3p Reviewer: itamar, therve Fixes: #2997

Implements getPeer()/getHost() for the SSHTransport. This also creates a new SSHTransportAddress which contains the data for those methods.

Note: See TracTickets for help on using tickets.