conch and getPeer

[bernat@…]

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

sshsimpleserver.py

@@ -30 +30 @@
     """this is our example protocol that we will run over SSH
     """
     def dataReceived(self, data):
+        print self.transport.getPeer()
         if data == '\r':
             data = '\r\n'
         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/channel.py", 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().

[bernat@…]

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

[z3p]

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

[z3p]

Ready for review.

Buildbot results:  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/conch-session-getPeer-2997

[z3p]

Forgot the review keyword.

[z3p]

I shouldn't review my own ticket.

[itamar]

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 address.py) '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 transport.py are longer than 80 chars.
6. @ivar secondary lines (address.py) are usually indented, though I have no idea if that's actually required.

[exarkun]

#5566 was a duplicate of this.

[z3p]

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:  http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/conch-session-getPeer-2997

[therve]

1. Pyflakes: twisted/conch/test/test_transport.py:573: local variable 'testCipher' is assigned to but never used

2. It's better to only import SSHTransportAddress instead of the whole address module if you only use that object.

3. Please add @since marker to your new module and your 2 new methods.

4. Please put 3 lines after the imports and before your first class.

All very minor, please merge once fixed. Thanks!

[z3p]

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

Refs #2997

[z3p]

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

Refs #2997

[z3p]

(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.