twisted.conch.ssh.session has poor tests

[z3p]

See #2678, #2679, #2682, #2707, #2709.

[z3p]

Ready for review in branches/session-2710-2.

[therve]

First of all, there is a minor conflict on connection.py, you'll need to merge forward.

General:

• We don't yet stated on the blank lines, but 2 between classes/1 between method is a minimum. You can also apply the 3/2 rule.

unix.py:

• huge typo in the imports lines
• typo on subsystem in lookupSubsystem

session.py:

• use try/except/else return in the __init__ of SSHSession instead of try/return/except
• the test execCommand.func_code.co_argcount == 3 is a bit scary. There might be a better way, although I don't have one...
• most warnings should be set with stack_level=2, so that it appears in the good file when you use assertWarns.
• docstring of SSHSession.request_exec is cut somewhere
• docstring of SSHSession.getPty says @aram modes.
• I don't really like log.deferr, I'd prefer log.err. I don't know the official position on this :).
• the plain 'except:' are not great. In 2.6, {{{except Exception}}} will do the 'right' thing by not catching SystemExit/KeyboardInterrupt, but it'll change the semantic... Rha I don't know what's the good solution.

test_session.py:

• typo in the docstring twisted//conch/ssh/session.py. I would prefer twisted.conch.ssh.session though.
• StubClient must create the StubTransport in __init__, or you'll have to clean the buffer of transport (test_client fails with until-failure).

I guess this following ones are modified in other branches, so don't take my comments into account if it's the case:

test_userauth.py:

• unused import reactor

avatar.py:

• unused import log

That's it for now!

[z3p]

The last two are modified in other branches. The rest are ready for review in session-2710-3.

[therve]

• the change in StubClient miss a self, it broke a test
• test_requestExecGetsSession and test_requestShellGetsSession print warnings, they are missing a supress statement I guess
• speaking of it, suppress in general should be used with the message of the warning, so that it doesn't swallow all warnings
• while you're at it, if you do except Exception, you could do except Exception e and pass the exception instance to log.err.
• SSHSession.request_exec docstring is still broken
• there is bunch of docstrings missing in test_session: StubSessionForStubNewAvatar, MockProcessProtocol, MockProtocol in particular.

[radix]

log.err() seems preferable to log.err(e) here. log.err() will include traceback information, log.err(e) will not.

[z3p]

I think I've fixed these things (not including log.err) in seession-2710-3 so I'm putting this back up for review.

[therve]

Replying to radix:

log.err() seems preferable to log.err(e) here. log.err() will include traceback information, log.err(e) will not.

My bad, I didn't think there was a difference (that's kind of weird there is one in fact), but that was just for optimization so it doesn't really matter.

[therve]

• unused import util in test_session
• the conch script now prints some warnings, it should be fixed. I guess you should also open a ticket to add tests to it.

That's it!

[z3p]

I've fixed the two warnings I got when I ran the script and removed the unused import. As for the tests, once this is merged, I'm planning to merge forward free-assert-2112-3 to work on the script tests.

[therve]

I still have one warning:

twisted/conch/scripts/conch.py:346: DeprecationWarning: Using SSHSessionClient is deprecated.  Use SSHSession instead.

c = session.SSHSessionClient()

Otherwise it looks good to me, but I guess exarkun would like to have a look.

[z3p]

I've fixed this warning; bouncing to exarkun.

[exarkun]

simple stuff:

• avatar.py
  • no copyright statement or module docstring
  • twisted.python.log imported but unused
  • zope.interface import should come before twisted imports (imports should be in order of increasing dependencies - ie, if A uses B and C uses both and can reasonably know about A's use of B, C should import B before it imports A. This simplifies import-related debugging.)
  • ConchUser.lookupChannel removes trailing whitespace but leaves whitespace around the = in the method call on the same lines, might as well fix that at the same time.
• interfaces.py
  • Lots of trailing whitespace
  • some methods (changed/added in this branch) smooshed right up against the method either before or after them - two blank lines between method definitions, please.
  • Could lookupSession's docstring use epytext?
• conch.py
  • is the change to eofReceived tested anywhere? (is eofReceived tested anywhere?)
  • what's up with the changes to the packRequest_pty_req calls? it looks like passing a list is deprecated in favor of passing a string, but the changes go the other way.
• session.py
  • some trailing whitespace
  • a module-level string above the list of supported signals - this should be a comment or something
  • SSHSession class docstring could use L{} in a few places, instead of C{}, to generate links to relevant API docs.
  • clobbering methods with instance attributes if there's no ISession is kinda ugly. Wouldn't it be cleaner to override requestReceived, do the check there, and then call the base implementation?
  • the log.err(e) call in request_session could probably use a 2nd argument explaining the context of the error.
  • doing argument count checks is pretty hairy - request_session will do nasty stuff to functions with varargs or optional args. not sure if there's much that can practically be done about this while preserving the method name openShell. It's a sticky point, though.
  • similar comment for request_exec. it might make sense to at least factor the counting code out into a helper function shared between these two.

Overall, I'm a bit more concerned with the backwards compatibility for the IConchUser/ISession change. Having # old style and # new style peppered throughout the code is certainly a recipe for weird bugs, broken compatibility, and other maintenance headaches. A less complex way to implement this kind of compatibility is to factor all of the old-style support code into one class, all of the new-style support code into another class, and then somehow arrange to have an instance of the right class at the right time (the most straightforward way being to have a completely separate object responsible for the area of the code with deprecations and have the main class, ie SSHSession, instantiate the right class based on what kind of object it gets). There are various subtleties involving interfaces (eg, if you create a new interface defining for the new API, then you can rely on adapters to pick the right implementation for you), but the basic idea remains the same.

I haven't looked over this code carefully enough yet to know if the current implementation is doing all of the right things wrt backwards compatibility. If you like, I can try to do that, and if it turns out to look correct, the branch can probably be merged (once above points are addressed, of course). However, I'd much prefer if the code were factored in the simpler way I just described, since it will result in better overall code and reviewing it will be much easier. :)

[z3p]

• avatar.py: fixed
• interfaces.py: fixed
• conch.py: the eofReceived change was because of a deprecation warning, but I'm not particular about keeping it. The packRequest_pty_req change is str -> list.
• session.py: I fixed whitespace, C{} -> L{}, and abstracted out the argument counting.

I disagree about the "clobbering methods." I think it's much cleaner than overriding requestReceived and special-casing the four requests there.

I understand what you're saying about backwards compatibility, but I don't think that moving all the backwards compatible code into a separate object will make the code much cleaner. It just replaces the # old style and # new style comments with method calls. I've written the code so that the tests that previously passed still pass with the new code, so as far as I'm concerned it's backwards compatible.

[glyph]

Replying to z3p:

I disagree about the "clobbering methods." I think it's much cleaner than overriding requestReceived and special-casing the four requests there.

Dispatching requests to different methods is a fairly standard "design pattern", especially within Twisted. Why is changing the interface of an object's attributes a preferable alternative?

I understand what you're saying about backwards compatibility, but I don't think that moving all the backwards compatible code into a separate object will make the code much cleaner. It just replaces the # old style and # new style comments with method calls.

... and moves all the backward compatibility code into a single unit which can be independently documented, and perhaps more importantly, independently removed when it is officially end-of-lifed. We have a lot more tools to work with method calls than with comments.

I've written the code so that the tests that previously passed still pass with the new code, so as far as I'm concerned it's backwards compatible.

Maaaybe this is okay for the code under discussion because it doesn't have too many users, but in general the issue is not the code you've written, it's the code that other people (launchpad et. al., in this case) have written that you don't have access to, but will break in some way you don't expect.

[z3p]

Okay, under the weight of both of your greater experience, I've moved the deprecated SSHSession code into a DeprecatedSSHSession class, which is instantiated to call the old methods. Currently I call it's methods with DeprecatedSSHSession(self).<method> each time because I couldn't decide on an appropriate place to make one instantiation. I also added a requestReceived method which does the ISession check. Ready for review.

[exarkun]

cftp tests are failing now, mostly like this:

  File "/home/exarkun/Projects/Twisted/trunk/twisted/conch/ssh/session.py", line 71, in requestReceived
    self.session = ISession(self.avatar)
exceptions.TypeError: ('Could not adapt', <twisted.conch.test.test_filetransfer.FileTransferTestAvatar instance at 0xb71de2cc>, <InterfaceClass twisted.conch.interfaces.ISession>)

I guess I didn't explain the backwards compatibility factoring idea very well, since the DeprecatedSSHSession(self).<method> thing isn't quite what I had in mind.

Consider request_subsystem:

    def request_subsystem(self, data):
        """
        The remote side has requested a subsystem.  Payload::
            string  subsystem name

        Try to get a subsystem object by calling our adapter's lookupSubsystem
        method.  If that method returns a subsystem, then connect to our
        client and return True.  Otherwise, return False.
        """
        subsystem, rest = common.getNS(data)
        log.msg('asking for subsystem "%s"' % subsystem)
        if hasattr(self.session, 'lookupSubsystem'): # new ISession
            client = self.session.lookupSubsystem(subsystem, rest)
            if client:
                self.makeConnection(client)
                return True
            else:
                return False
        else: # old ISession
            # we pass data instead of rest because that's what the old
            # code did
            return DeprecatedSSHSession(self).requestSubsystem(subsystem,
                    data)

The specific goals of the factoring change I tried to suggest are that:

• request_subsystem shouldn't need to know that the old ISession didn't have a lookupSubsystem method.
• request_subsystem shouldn't need to contain code for switching between old and new versions of the interface
• Implementations of the old ISession which happened to implement lookupSubsystem shouldn't be treated as implementations of the new ISession.

The ideal implementation of request_subsystem might look something like this:

    def request_subsystem(self, data):
        """
        The remote side has requested a subsystem.  Payload::
            string  subsystem name

        Try to get a subsystem object by calling our adapter's lookupSubsystem
        method.  If that method returns a subsystem, then connect to our
        client and return True.  Otherwise, return False.
        """
        subsystem, rest = common.getNS(data)
        log.msg('asking for subsystem "%s"' % subsystem)
        client = self.session.lookupSubsystem(subsystem, rest)
        if client:
            self.makeConnection(client)
            return True
        else:
            return False

If it can be made to work, this accomplishes all three of the above goals. I think it can be made to work, if self.session is initialized something like this (in either __init__ or requestReceived):

    session = getOldSession(avatar)
    if session is not None:
        session = AdaptOldSessionToNewSession(session)
    else:
        session = getNewSession(avatar)
    self.session = session

The specific details of getOldSession, getNewSession, and AdaptOldSessionToNewSession are important, of course, but in a lot of ways they're only secondary to the advantage this buys all the rest of the methods on SSHSession.

The most foolproof way to implement those three methods is to introduce a new interface which will ultimately supercede ISession, then replace getOldSession with ISession, getNewSession and AdaptOldSessionToNewSession with INewSession, and register an adapter from ISession to INewSession (which might have some overlap with DeprecatedSSHSession).

I hope this makes more sense. Sorry about the miscommunication earlier.

[glyph]

(In [22764]) Branching to 'session-2710-4'

[z3p]

Ready for review.

[therve]

• pyflakes errors:

  twisted/conch/scripts/conch.py:11: 'agent' imported but unused
  twisted/conch/scripts/conch.py:15: 'defer' imported but unused
  twisted/conch/scripts/conch.py:18: 'errno' imported but unused
  twisted/conch/scripts/conch.py:18: 'base64' imported but unused
  twisted/conch/scripts/conch.py:18: 'stat' imported but unused
  twisted/conch/scripts/conch.py:213: redefinition of unused 'errno' from line 18
  twisted/conch/scripts/conch.py:453: redefinition of unused 'errno' from line 18
  twisted/conch/scripts/conch.py:486: undefined name '_savedMode'
  

• trailing whitespaces
• test_filetransfer fails for me:

  [ERROR]: twisted.conch.test.test_filetransfer.TestFileTransferClose.test_stopConnectionServiceClosesChannel
  
  Traceback (most recent call last):
    File "/home/therve/Projects/Twisted/trunk/twisted/conch/test/test_filetransfer.py", line 503, in test_stopConnectionServiceClosesChannel
      self.interceptConnectionLost(sftpServer)
    File "/home/therve/Projects/Twisted/trunk/twisted/conch/test/test_filetransfer.py", line 435, in interceptConnectionLost
      origConnectionLost = sftpServer.connectionLost
  exceptions.AttributeError: _ProtocolToProcessProtocol instance has no attribute 'connectionLost'
  

• test_cftp is failing too
• ISessionApplication is missing documentation
• _SubsystemOnlyApplicationFactory.closed docstring is not finished. pass is unnecessary too.
• code is missing blank lines

That's it. I didn't really look deeply at the code yet, but it looked basically good. Thanks!

[z3p]

(In [23019]) Branching to 'session-2710-5'

[z3p]

I think I got all this stuff. I grepped around for trailing whitespace, but I didn't see any, so let me know if there's still some around.

[therve]

I have made the modifications for the whitespaces, and fixed a syntax problem with python 2.3.

• SSHSessionProcessProtocolTestCase.test_getSignalName is failing on several platforms. On windows 'module' object has no attribute 'SIGALRM'. On OS X exceptions.KeyError: 33. On some linux exceptions.KeyError: 66.

• StubApplicationFactoryForStubNewAvatar and StubSessionForStubOldAvatar use hasattr for several attributes: it should instead provide default value None for this attributes
• StubTransport has a undocumented close attribute
• tests should use assertIsInstance instead of assertTrue(isinstance(...))
• test_lookupSubsystem and test_lookupSubsystemProtocol both use a callLater(0). I don't know what's the good solution, but StubConnection could probably have a deferred that fire on some conditions (when a quantity of data sent reach a number for example).
• all calls to flushLoggedErrors should check their output. Otherwise, it could mask unexpected errors.
• test_processEndedWithExitSignalNoCoreDump and test_processEndedWithExitSignalCoreDump skip message is cut at the middle.
• in test_extendedDataReceived, if I comment the line pp.transport = object nothing happens

• in SSHSession, there are lots of 'if self.attribute', whereas we really want to check 'if self.attribute is not None'.

[therve]

(In [23184]) Address some points

Refs #2710

[jml]

Apparently addresses the issue in #2687.

[therve]

I think I've addressed all my points :). Can you have look?

[jml]

Hi Thomas & Paul,

This is a review of branches/session-2710-5.

This diff is over 3000 lines long. Although sometimes work can't be broken up, you should make an effort. The time it takes to review a branch scales non-linearly with the size of the diff.

Before you get into the review, I want to thank both of you for taking the time to do this work. Conch more than any other part of Twisted will benefit from better tests and being easier to test.

There's going to be another review round-trip and there *are* bits of the diff that I have serious issues with. Still, the patch as a whole shows clear thinking and a strong desire to separate concerns that were previously mixed together. You're attention to backwards compatibility is also greatly appreciated.

Because I'm coming to this branch cold, I'm going to be doing a full review. Personally, I hate it when an old branch of mine is subject to a fresh review by a fresh reviewer -- it means a lot of decisions get questioned again and feels like a lot of work little or no reason. It's also likely that the new reviewer will contradict the old reviewer.

I'm going to try as hard as I can to focus only on code cleanliness, and to assume that you and the previous reviewers knew what they were doing. What that means is I'll flag everything that's wrong and everything that I don't get, and I'll try to distinguish between the two.

I'd very much appreciate it if you could respond to each of my points, even if it is to say "no, I don't think I'll do that". The priority now is getting this branch landed. If you reply point-wise, my re-review can probably be done trivially.

Also, feel free to grab me on IRC or on skype ('blackjml') if you want to discuss any of these points.

Index: twisted/conch/test/test_filetransfer.py =================================================================== --- twisted/conch/test/test_filetransfer.py (revision 23210) +++ twisted/conch/test/test_filetransfer.py (working copy) @@ -56,11 +56,6 @@ return os.path.join(os.getcwd(), self.homeDir) -class ConchSessionForTestAvatar: - - def init(self, avatar): - self.avatar = avatar - if unix: if not hasattr(unix, 'SFTPServerForUnixConchUser'): # unix should either be a fully working module, or None. I'm not sure @@ -463,7 +458,7 @@ self.interceptConnectionLost(sftpServer) # close session - testSession.closeReceived() + testSession.closed()

XXX I don't know what this is for. Let's see if I find out.

self.assertSFTPConnectionLost() Index: twisted/conch/scripts/conch.py =================================================================== --- twisted/conch/scripts/conch.py (revision 23210) +++ twisted/conch/scripts/conch.py (working copy) @@ -8,20 +8,21 @@ #""" Implementation module for the conch command. #"""

Please either delete this comment, or uncomment the docstring and make it comply with Twisted conventions.

-from twisted.conch.client import agent, connect, default, options +from twisted.conch.client import connect, default, options from twisted.conch.error import ConchError from twisted.conch.ssh import connection, common from twisted.conch.ssh import session, forwarding, channel -from twisted.internet import reactor, stdio, defer, task +from twisted.internet import reactor, stdio, task from twisted.python import log, usage -import os, sys, getpass, struct, tty, fcntl, base64, signal, stat, errno +import os, sys, getpass, struct, tty, fcntl, signal, errno + class ClientOptions(options.ConchOptions): - + synopsis = """Usage: conch [options] host [command] """ - + optParameters = [['escape', 'e', '~'], ['localforward', 'L', None, 'listen-port:host:port Forward local port to remote address'], ['remoteforward', 'R', None, 'listen-port:host:port Forward remote port to local address'],

Please wrap these lines to fit in 79 columns.

@@ -81,7 +82,7 @@ exitStatus = 0 old = None _inRawMode = 0 -_savedRawMode = None +_savedMode = None def run(): global options, old @@ -161,7 +162,7 @@ options.identitys = ['~/.ssh/id_rsa', '~/.ssh/id_dsa'] host = optionshost? if not optionsuser?: - optionsuser? = getpass.getuser() + optionsuser? = getpass.getuser() if not optionsport?: optionsport? = 22 else: @@ -210,7 +211,6 @@ try: os.close(i) except OSError, e: - import errno if e.errno != errno.EBADF: raise @@ -273,7 +273,7 @@ def requestRemoteForwarding(self, remotePort, hostport): data = forwarding.packGlobal_tcpip_forward(('0.0.0.0', remotePort)) - d = self.sendGlobalRequest('tcpip-forward', data, + d = self.sendGlobalRequest('tcpip-forward', data, wantReply=1) log.msg('requesting remote forwarding %s:%s' %(remotePort, hostport)) d.addCallback(self._cbRemoteForwarding, remotePort, hostport) @@ -283,7 +283,7 @@ log.msg('accepted remote forwarding %s:%s' % (remotePort, hostport)) self.remoteForwards[remotePort] = hostport log.msg(repr(self.remoteForwards)) - + def _ebRemoteForwarding(self, f, remotePort, hostport): log.msg('remote forwarding %s:%s failed' % (remotePort, hostport)) log.msg(f) @@ -330,7 +330,7 @@ else: # because of the unix thing self.class.bases[0].channelClosed(self, channel) - + class SSHSession(channel.SSHChannel): name = 'session' @@ -360,7 +360,7 @@ term = os.environTERM? winsz = fcntl.ioctl(fd, tty.TIOCGWINSZ, '12345678') winSize = struct.unpack('4H', winsz) - ptyReqData = session.packRequest_pty_req(term, winSize, ) + ptyReqData = session.packRequest_pty_req(term, winSize, [])

XXX I wonder why this is different.

self.conn.sendRequest(self, 'pty-req', ptyReqData) signal.signal(signal.SIGWINCH, self._windowResized) self.conn.sendRequest(self, 'exec', \ @@ -370,7 +370,7 @@ term = os.environTERM? winsz = fcntl.ioctl(fd, tty.TIOCGWINSZ, '12345678') winSize = struct.unpack('4H', winsz) - ptyReqData = session.packRequest_pty_req(term, winSize, ) + ptyReqData = session.packRequest_pty_req(term, winSize, [])

XXX I wonder why this is different.

self.conn.sendRequest(self, 'pty-req', ptyReqData) signal.signal(signal.SIGWINCH, self._windowResized) self.conn.sendRequest(self, 'shell', ) @@ -408,7 +408,8 @@ channels = self.conn.channels.keys() channels.sort() for channelId in channels: - self.stdio.write(' #%i %s\r\n' % (channelId, str(self.conn.channels[channelId]))) + self.stdio.write(' #%i %s\r\n' % (channelId, + str(self.conn.channels[channelId]))) return self.write('~' + char) else: @@ -425,8 +426,8 @@ def eofReceived(self): log.msg('got eof') - self.stdio.closeStdin() - + self.stdio.loseWriteConnection() + def closeReceived(self): log.msg('remote side closed %s' % self) self.conn.sendClose(self) @@ -449,7 +450,6 @@ try: os.close(i) except OSError, e: - import errno if e.errno != errno.EBADF: raise @@ -472,8 +472,8 @@ winSize = struct.unpack('4H', winsz) newSize = winSize[1], winSize[0], winSize[2], winSize[3] self.conn.sendRequest(self, 'window-change', struct.pack('!4L', *newSize)) - + class SSHListenClientForwardingChannel(forwarding.SSHListenClientForwardingChannel): pass class SSHConnectForwardingChannel(forwarding.SSHConnectForwardingChannel): pass @@ -504,7 +504,7 @@ new[0] = new[0] & ~tty.IUCLC # lflag - new[3] = new[3] & ~(tty.ISIG | tty.ICANON | tty.ECHO | tty.ECHO | + new[3] = new[3] & ~(tty.ISIG | tty.ICANON | tty.ECHO | tty.ECHO | tty.ECHOE | tty.ECHOK | tty.ECHONL) if hasattr(tty, 'IEXTEN'): new[3] = new[3] & ~tty.IEXTEN Index: twisted/conch/ssh/session.py =================================================================== --- twisted/conch/ssh/session.py (revision 23210) +++ twisted/conch/ssh/session.py (working copy) @@ -1,9 +1,7 @@ -# -*- test-case-name: twisted.conch.test.test_conch -*- -# Copyright (c) 2001-2004 Twisted Matrix Laboratories. +# -*- test-case-name: twisted.conch.test.test_session -*- +# Copyright (c) 2001-2007 Twisted Matrix Laboratories.

It's 2008 now.

# See LICENSE for details. -# - """ This module contains the implementation of SSHSession, which (by default) allows access to a shell and a python interpreter over SSH. @@ -11,214 +9,890 @@ Maintainer: U{Paul Swartz<mailto:z3p@…>} """ -import struct +import struct, warnings, signal, os, sys +from zope.interface import implements + from twisted.internet import protocol -from twisted.python import log -from twisted.conch.interfaces import ISession -from twisted.conch.ssh import common, channel +from twisted.python import components, log +from twisted.conch.interfaces import ISessionApplication +from twisted.conch.interfaces import ISessionApplicationFactory, ISession +from twisted.conch.ssh import common, channel, connection + +# supportedSignals is a list of signals that every session channel is supposed +# to support. +supportedSignals = ["ABRT", "ALRM", "FPE", "HUP", "ILL", "INT", "KILL", "PIPE", + "QUIT", "SEGV", "TERM", "USR1", "USR2"] + + + class SSHSession(channel.SSHChannel): + """ + A channel implementing the server side of the 'session' channel. This is + the channel that a client requests when it wants a subsystem, shell, or + command. + @ivar earlyData: data sent to this channel before a client is present. + @type earlyData: C{str} + @ivar earlyExtended: extended data sent to this channel before a client is + present. + @type earlyExtended: C{list} + @ivar applicationFactory: an object to which requests for shells and such + are dispatched. It implements I{ISessionApplicationFactory}. + @ivar sessionApplication: an object which handles interacting with the + other side of the channel. It implements I{ISessionApplication}. + @ivar client: a C{ProcessProtocol} to which data sent to this channel is + sent. This variable is deprecated. + @type client: L{protocol.ProcessProtocol} + @ivar session: an object implementing L{ISession} to which requests for + shells or commands are dispatched. This variable is deprecated. + """ + + + # set so that this object can be used as a client channel. name = 'session' + + + def setattr(self, k, v): + """ + Trap the 'client' attribute, what used to be the old name (roughly) for + 'sessionApplication', and 'session', which triggers setting up + _DeprecatedSSHSession as our application factory. + """ + if k == 'client': + # Someone is trying to inform us of an old-style client. Clear the + # buffers (because this would not have previously delivered any + # data, only delivered subsequent data) and set the old-style + # "client" object up as a new-style processProtocol. + self.earlyData = + self.earlyExtended = [] + self.setupSession(_TransportToProcessProtocol(v.transport)) + if k == 'session' and v is not None: + # Someone is trying to inform us of an old-style session. Wrap it + # in a _DeprecatedSSHSession. + self.applicationFactory = _DeprecatedSSHSession(self, v) + self.dict[k] = v +

I saw the 'setattr' and had a moment of quiet terror. Now I see that it's there for backwards compatibility -- this code will make my life easier, so thanks :)

However, I think that you could and should implement this with properties.

+ + def applicationFactory(self): + """ + Produce an applicationFactory dynamically if one does not yet exist. + """ + if self.avatar is not None: + factoryCandidate = ISessionApplicationFactory(self.avatar, None) + if factoryCandidate is None: + # Maybe it implements the old version. + oldStyle = ISession(self.avatar, None) + if oldStyle is not None: + # See setattr above. + self.session = oldStyle + else: + # Maybe it doesn't implement either. The test + # SFTP server doesn't implement a session, because + # subsystems were just looked up in the avatar. + # Use a _SubsystemOnlyApplicationFactory. + self.applicationFactory = _SubsystemOnlyApplicationFactory( + self) + else: + self.applicationFactory = factoryCandidate + return self.applicationFactory + applicationFactory = property(applicationFactory) +

This method doesn't do what it says it does. The check at the beginning is for whether 'self.avatar' is None. Calling this method multiple times will create an applicationFactory multiple times. Is this deliberate? If so, please update the docstring. If not, then I *think* you need to change the guard at the top of the function.

Also, it's not clear to me that applicationFactory should be None if no avatar is set. The docstring should state that this is the case and explain why.

+ + def client(self): + """ + This used to be the C{ProcessProtocol} that was connected to us. + Since we're not connected to a C{ProcessProtocol} anymore, we + fake it. + """ + if isinstance(self.sessionApplication, + _ProcessProtocolToSessionApplication): + return self.sessionApplication.processProtocol + elif isinstance(self.sessionApplication, + SSHSessionProcessProtocolApplication): + return self.sessionApplication.processProtocol + client = property(client) +

You should extract the if k == 'client' bit out of setattr and make it the setter for this property.

Also, I think this should raise a DeprecationWarning.

+ + # This used to be set in the older SSHSession implementation. + session = None +

And this needs to have a getter. I'm sorry, but there's code out there that uses this.

+ def init(self, *args, **kw): channel.SSHChannel.init(self, *args, **kw) - self.buf = - self.client = None - self.session = None + self.earlyData = + self.earlyExtended = [] + self.sessionApplication = None + + def setupSession(self, sessionApplication): + """ + Connect us to the application. We set ourselves as it's channel + instance variable, and the application becomes our sessionApplication. + If any data was sent early, send it now. + """

Is this supposed to be public? If not, make it _setUpSession.

fIs there already an API or compatibility constraint on the name? If not, make it setUpSession.

+ # make sure we have an ISessionApplication + sessionApplication = ISessionApplication(sessionApplication) + sessionApplication.channel = self + sessionApplication.connectionMade() + # Maybe change this name too...

Please say why you think this name should maybe change.

+ self.sessionApplication = sessionApplication + if self.earlyData: + b, self.earlyData = self.earlyData, None + self.dataReceived(b) + if self.earlyExtended: + b, self.earlyExtended = self.earlyExtended, None + for dataType, data in b: + self.extReceived(dataType, data) +

I guess 'b' stands for 'bytes'. Could you please rename it?

+ def request_subsystem(self, data): - subsystem, ignored= common.getNS(data) + """ + The remote side has requested a subsystem. Payload:: + string subsystem name + + Try to get a subsystem object by calling our adapter's lookupSubsystem + method. If that method returns a subsystem, then connect to our + client and return True. Otherwise, return False. + """

I'm surprised you don't return the subsystem. Why don't you?

+ subsystem, rest = common.getNS(data) log.msg('asking for subsystem "%s"' % subsystem) - client = self.avatar.lookupSubsystem(subsystem, data) + client = self.applicationFactory.lookupSubsystem(subsystem, rest) if client: - pp = SSHSessionProcessProtocol(self) - proto = wrapProcessProtocol(pp) - client.makeConnection(proto) - pp.makeConnection(wrapProtocol(client)) - self.client = pp - return 1 + self.setupSession(client) + return True else: - log.msg('failed to get subsystem') - return 0 + return False + def request_shell(self, data): + """ + The remote side has requested a shell. No payload. Call the + application factory's openShell() method; it returns something + implementing I{ISessionApplication} that will become our + application. If there's no exception, return True. + Otherwise, return False. + """ log.msg('getting shell') - if not self.session: - self.session = ISession(self.avatar) try: - pp = SSHSessionProcessProtocol(self) - self.session.openShell(pp) - except: - log.deferr() - return 0 + self.setupSession(self.applicationFactory.openShell()) + except Exception:

This looks wrong. Why are you catching all exceptions here and not in other 'request_' methods?

Also, a bare except is the way to go, as long as you catch and reraise KeyboardInterrupt.

+ log.err() + return False else: - self.client = pp - return 1 + return True + def request_exec(self, data): - if not self.session: - self.session = ISession(self.avatar) - f,data = common.getNS(data) - log.msg('executing command "%s"' % f) + """ + The remote side has requested to execute a command. Payload:: + string command line + + Call the application factory's execCommand method with the + command line. It should return something implementing + I{ISessionApplication} that becomes our client. If there's no + exception, return True. Otherwise, return False. + """ + command, data = common.getNS(data) + log.msg('executing command "%s"' % command) try: - pp = SSHSessionProcessProtocol(self) - self.session.execCommand(pp, f) - except: - log.deferr() - return 0 + self.setupSession(self.applicationFactory.execCommand(command)) + except Exception: + log.err() + return False

Same comment as before.

else: - self.client = pp - return 1 + return True + def request_pty_req(self, data): - if not self.session: - self.session = ISession(self.avatar) - term, windowSize, modes = parseRequest_pty_req(data) - log.msg('pty request: %s %s' % (term, windowSize)) + """ + The remote side has requested a psuedoterminal (PTY). Payload:: + string terminal name + uint32 columns + uint32 rows + uint32 xpixels + uint32 ypixels + string modes + + Modes is:: + 0 or more of:: + byte mode number + uint32 mode value + + Call the application factory's getPTY method. If no exception + is raised, return True. Otherwise, return False. + """

Thanks for including these docstrings, btw. They are a real help.

try: - self.session.getPty(term, windowSize, modes) - except: + term, windowSize, modes = parseRequest_pty_req(data) + log.msg('pty request: %s %s' % (term, windowSize)) + self.applicationFactory.getPTY(term, windowSize, modes) + except Exception: log.err() - return 0 + return False else: - return 1 + return True + def request_window_change(self, data): - if not self.session: - self.session = ISession(self.avatar) - winSize = parseRequest_window_change(data) + """ + The remote side has changed the window size. Payload:: + uint32 columns + uint32 rows + uint32 xpixels + uint32 ypixels + + + Call the application factory's windowChanged method. If no + exception is raised, return True. Otherwise, return False. + """ try: - self.session.windowChanged(winSize) - except: + winSize = parseRequest_window_change(data) + self.applicationFactory.windowChanged(winSize) + except Exception: log.msg('error changing window size') log.err() - return 0 + return False else: - return 1 + return True

OK, I'm beginning to suspect you just missed out the except clause in request_subsytem. Please make sure you add it and switch to bare excepts.

+ def dataReceived(self, data): - if not self.client: - #self.conn.sendClose(self) - self.buf += data + """ + We got data from the remote side. If we have an application, + send the data to it. Otherwise, buffer the data. + + @type data: C{str} + """ + if self.sessionApplication is None: + self.earlyData += data

Perhaps StringIO would be more appropriate than a string for earlyData. If you think so, whack an XXX comment somewhere in this file saying so -- this branch is already too big. Be sure to put your name and the date on the comment.

return - self.client.transport.write(data) + self.sessionApplication.dataReceived(data) + def extReceived(self, dataType, data): - if dataType == connection.EXTENDED_DATA_STDERR: - if self.client and hasattr(self.client.transport, 'writeErr'): - self.client.transport.writeErr(data) + """ + We got extended data from the remote side. If we have an + application, send the data to it. Otherwise, buffer the data. + + @type dataType: C{int} + @type data: C{str} + """

You know, I'd always wondered what this method was for. Thanks for adding the docstring :)

+ if self.sessionApplication is not None: + self.sessionApplication.extendedDataReceived(dataType, data) else: - log.msg('weird extended data: %s'%dataType) + self.earlyExtended.append((dataType, data)) + def eofReceived(self): - if self.session: - self.session.eofReceived() - elif self.client: - self.conn.sendClose(self) + """ + The remote side has closed its write side. If we have an + application factory, notify it. If we have an application, + notify it. + """ + if self.applicationFactory is not None: + self.applicationFactory.eofReceived() + if self.sessionApplication is not None: + self.sessionApplication.eofReceived() + def closed(self): - if self.session: - self.session.closed() - elif self.client: - self.client.transport.loseConnection() + """ + The channel is closed. If we have an application factory, + notify it. If we have an application, tell it the connection + is lost. + """

It *might* be worth mentioning here that just because the channel is closed, that doesn't mean that the SSH connection to the client is closed.

Just a thought.

+ if self.applicationFactory is not None: + self.applicationFactory.closed() + if self.sessionApplication is not None: + self.sessionApplication.closed() - #def closeReceived(self): - # self.loseConnection() # don't know what to do with this - def loseConnection(self): - if self.client: - self.client.transport.loseConnection() - channel.SSHChannel.loseConnection(self) + def closeReceived(self): + """ + The remote side has requested that we no longer send data. If + we have an application factory, notify it. If we have an + application, notify it. + """

Good docstring.

-class _ProtocolWrapper(protocol.ProcessProtocol): + if self.applicationFactory is not None: + self.applicationFactory.closeReceived() + if self.sessionApplication is not None: + self.sessionApplication.closeReceived() + + + # client methods +

'client methods' is ambiguous. Please say what they are used for, why they are different to the previous methods and, if you think it's appropriate, why they are in this class at all.

+ + def getPty(self, term, windowSize, modes, wantReply=False): + """ + Request a PTY from the other side. + + @param term: the type of terminal (e.g. xterm) + @type term: C{str} + @param windowSize: the size of the window: (rows, cols, xpixels, + ypixels) + @type windowSize: C{tuple} + @param modes: termanal modes; a list of (modeNumber, modeValue) pairs.

It's spelled 'terminal'. It might be worth putting in a 'see also', so people who want to know what modeNumber and modeValue mean can find out.

+ @type modes: C{list} + @param wantReply: True if we want a reply to this request. + @type wantReply: C{bool} + + @returns: if wantReply, a Deferred that will be called back when + the request has succeeded or failed; else, None. + @rtype: C{Deferred}/C{None} + """ + data = packRequest_pty_req(term, windowSize, modes) + return self.conn.sendRequest(self, 'pty-req', data, + wantReply=wantReply) + + + def openSubsystem(self, subsystem, data=, wantReply=False): + """ + Request that a subsystem be opened on the other side. + + @param subsystem: the name of the subsystem + @type subsystem: C{str} + @param data: any extra data to send with the request + @type data: C{str} + @param wantReply: True if we want a reply to this request. + @type wantReply: C{bool} + + @returns: if wantReply, a Deferred that will be called back when + the request has succeeded or failed; else, None. + @rtype: C{Deferred}/C{None} + """ + return self.conn.sendRequest(self, 'subsystem', common.NS(subsystem) + + data, wantReply=wantReply) + + + def openShell(self, wantReply=False): + """ + Request that a shell be opened on the other side. + + @param wantReply: True if we want a reply to this request. + @type wantReply: C{bool} + + @returns: if wantReply, a Deferred that will be called back when + the request has succeeded or failed; else, None. + @rtype: C{Deferred}/C{None} + """ + return self.conn.sendRequest(self, 'shell', , wantReply=wantReply) + + + def execCommand(self, command, wantReply=False): + """ + Request that a command be executed on the other side. + + @param command: the command to execute + @type command: C{str} + @param wantReply: True if we want a reply to this request. + @type wantReply: C{bool} + + @returns: if wantReply, a Deferred that will be called back when + the request has succeeded or failed; else, None. + @rtype: C{Deferred}/C{None} + """ + return self.conn.sendRequest(self, 'exec', common.NS(command), + wantReply=wantReply) + + + def changeWindowSize(self, windowSize, wantReply=False): + """ + Inform the other side that the local terminal size has changed. + + @param windowSize: the new size of the window: (rows, cols, xpixels, + ypixels) + @type windowSize: C{tuple} + @param wantReply: True if we want a reply to this request. + @type wantReply: C{bool} + + @returns: if wantReply, a Deferred that will be called back when + the request has succeeded or fails; else, None. + @rtype: C{Deferred}/C{None} + """ + data = packRequest_window_change(windowSize) + return self.conn.sendRequest(self, 'window-change', data, + wantReply=wantReply) +

I'm guessing that this wantReply business is for backwards compat? It seems a little strange otherwise.

+ + +class _SubsystemOnlyApplicationFactory: """ + An application factory which which is only good for looking up a + subsystem. It is used when there is not an ISession adapter + defined for the avatar. Its use is deprecated. + """

If it's deprecated then please make sure it issues a deprecation warning when used. See twisted.python.deprecate.

+ implements(ISessionApplicationFactory) + + + def init(self, sessionChannel): + self.sessionChannel = sessionChannel + + + def lookupSubsystem(self, subsystem, data): + """ + The previous ConchUser avatar had subsystems looked up through + the avatar instead of through the ISession adapter. To be backwards + compatible, try to look up the subsystem through the avatar. + """ + self.sessionChannel.earlyData = + self.sessionChannel.earlyExtended = [] + client = self.sessionChannel.avatar.lookupSubsystem(subsystem, + common.NS(subsystem) + data) + return wrapProcessProtocol(client) + + + def eofReceived(self): + self.sessionChannel.loseConnection() + + + def closeReceived(self): + """ + The old closeReceived method did not exist, so we dispatch to our + parent's method. + """ + channel.SSHChannel.closeReceived(self.sessionChannel) + + + def closed(self): + """ + The old implementation of this method didn't do anything, so we + don't do anything either. + """ + + + +class _DeprecatedSSHSession(_SubsystemOnlyApplicationFactory): + """ + This class brings the deprecated functionality of the old SSHSession + into a single place. + """ + + + def init(self, sessionChannel, oldISessionProvider): + _SubsystemOnlyApplicationFactory.init(self, sessionChannel) + self.oldISessionProvider = oldISessionProvider + + + def getPTY(self, term, windowSize, modes): + """ + The name of this method used to be getPty. + """ + return self.oldISessionProvider.getPty(term, windowSize, modes) + + + def openShell(self): + """ + The old openShell interface passed in a ProcessProtocol which would be + connected to the shell. + """ + pp = SSHSessionProcessProtocol(self.sessionChannel) + self.oldISessionProvider.openShell(pp) + return SSHSessionProcessProtocolApplication(pp) + + + def execCommand(self, command): + """ + The old execCommand interface passed in a ProcessProtocol which would + be connected to the command. + """ + pp = SSHSessionProcessProtocol(self.sessionChannel) + self.oldISessionProvider.execCommand(pp, command) + return SSHSessionProcessProtocolApplication(pp) + + + def eofReceived(self): + """ + The old eofReceived method closed the session connection. + """ + self.oldISessionProvider.eofReceived() + self.sessionChannel.conn.sendClose(self.sessionChannel) + + + def closed(self): + """ + The old closed method closed the client's transport. + """ + if self.sessionChannel.sessionApplication is not None: + self.sessionChannel.sessionApplication.processProtocol.transport.loseConnection() + self.oldISessionProvider.closed() + + + def windowChanged(self, newWindowSize): + """ + Just pass this method through. + """ + self.oldISessionProvider.windowChanged(newWindowSize) + + + +class _ProtocolToProcessProtocol(protocol.ProcessProtocol): + """ This class wraps a L{Protocol} instance in a L{ProcessProtocol} instance. + + @ivar proto: the C{Protocol} we're wrapping. """

This belongs in twisted.internet, not twisted.conch. Please add a XXX comment saying so (with name and the date).

+ + def init(self, proto): self.proto = proto - def connectionMade(self): self.proto.connectionMade() - def outReceived(self, data): self.proto.dataReceived(data) + def getattr(self, attr): + """ + This class did not previously exist, so some uses expect this object + to implement the Protocol interface. To handle this case, we pass + requests through to the wrapped object. + """ + return getattr(self.proto, attr)

Is there a reason for not using proxyForInterfaces?

- def processEnded(self, reason): self.proto.connectionLost(reason) -class _DummyTransport: + def setattr(self, attr, value): + """ + See the documentation for getattr. + """ + if attr in ('proto', 'transport'): + self.dict[attr] = value + else: + setattr(self.proto, attr, value) - def init(self, proto): - self.proto = proto - def dataReceived(self, data): - self.proto.transport.write(data) + def connectionMade(self): + """ + Connect our C{Protocol} to our transport. + """ + self.proto.makeConnection(self.transport) + self.transport.proto = self - def write(self, data): + + def outReceived(self, data): + """ + Give the data to the C{Protocol}s dataReceived method. + """ self.proto.dataReceived(data) - def writeSequence(self, seq): - self.write(.join(seq)) - def loseConnection(self): - self.proto.connectionLost(protocol.connectionDone) + def processEnded(self, reason): + """ + Give the C{Failure} to the C{Protocol}s connectionLost method. + """ + self.proto.connectionLost(reason) + + def wrapProcessProtocol(inst): + """ + If we're passed a C{Protocol}, wrap it in a C{ProcessProtocol}. + Otherwise, just return what we were passed. + """ if isinstance(inst, protocol.Protocol): - return _ProtocolWrapper(inst) + return _ProtocolToProcessProtocol(inst) else: return inst

Can you use adapters for this instead?

If not, add a XXX comment saying that you *should* use adapters, and ideally, file a bug about it.

-def wrapProtocol(proto): - return _DummyTransport(proto) + +class _TransportToProcessProtocol(protocol.ProcessProtocol): + """ + This wraps something implementing I{ITransport} in a + C{ProcessProtocol} because that's how the old interface worked. +

Please say specifically which object you mean by "the old interface".

Given that this is backwards compatibility code, it would be good to say in which version of Twisted this was added. That way, we'll know when we can remove it.

+ @ivar applicationTransport: the ITransport we're wrapping. + """ + + + def init(self, applicationTransport): + self.applicationTransport = applicationTransport + + + def outReceived(self, data): + """ + When we get data, write it to our transport. + """ + self.applicationTransport.write(data) + + + def errReceived(self, errData): + """ + When we get extended data, give it to the writeErr method of + our transport. + """ + # XXX need to test when writeErr isn't available + self.applicationTransport.writeErr(errData) + + + def processEnded(self, reason): + """ + When we're told the process ended, tell the transport to drop + the connection. Yes, this loses data. + """ + self.applicationTransport.loseConnection() + + + +class _ProcessProtocolToSessionApplication: + """ + This adapts a C{ProcessProtocol} to an ISessionApplication. + + @ivar processProtocol: the C{ProcessProtocol} we're adapting. + """ +

It would be good to explain in the docstring why you need this adapter.

Same comments on including the version apply.

+ + implements(ISessionApplication) + + + def init(self, processProtocol): + self.processProtocol = processProtocol + + + def connectionMade(self): + """ + Connect the C{ProcessProtocol} to the channel. + """ + self.processProtocol.makeConnection(self.channel) + + + def dataReceived(self, data): + """ + Give the data to the C{ProcessProtocol} + """ + self.processProtocol.outReceived(data) + + + def extendedDataReceived(self, type, data): + """ + If the extended data came from standard error, give it to the + C{ProcessProtocol}. Otherwise drop it on the floor. + """ + if type == connection.EXTENDED_DATA_STDERR: + self.processProtocol.errReceived(data) + + + def eofReceived(self): + """ + Tell the C{ProcessProtocol} that no more data will be sent to + standard input. + """ + self.processProtocol.inConnectionLost() + + + def closeReceived(self): + """ + Tell the C{ProcessProtocol} that it shouldn't send any more data. + """ + self.processProtocol.outConnectionLost() + self.processProtocol.errConnectionLost() + + + def closed(self): + """ + Tell the C{ProcessProtocol} that the process ended. + + TODO: catch request_exit_status to give a better error message. + """ + self.processProtocol.processEnded(protocol.connectionDone) + + + +components.registerAdapter(_ProcessProtocolToSessionApplication, + protocol.ProcessProtocol, ISessionApplication) + + + class SSHSessionProcessProtocol(protocol.ProcessProtocol): + """ + This is a C{ProcessProtocol} that is used to connect to old-style + ISession implementations. An instance of this is passed to + openShell() and execCommand(), and then it's wrapped using + SSHSessionProcessProtocolApplication. + """ -# implements = I + + _signalNames = None +

Please add a comment explaining the structure of this variable.

+ def init(self, session): self.session = session - def connectionMade(self): - if self.session.buf: - self.transport.write(self.session.buf) - self.session.buf = None + def _getSignalName(self, signum): + """ + Get a signal name given a signal number. + """ + if self._signalNames is None: + self.class._signalNames = {} + # make sure that the POSIX ones are the defaults + for signame in supportedSignals: + signame = 'SIG' + signame + sigvalue = getattr(signal, signame, None) + if sigvalue is not None: + self._signalNames[sigvalue] = signame + for k, v in signal.dict.items(): + if k.startswith('SIG') and not k.startswith('SIG_'): + if v not in self._signalNames: + self._signalNames[v] = k + '@' + sys.platform + return self._signalNames[signum] +

So, figuring this out, _signalNames maps from signal numbers to signal names.

The names are either the regular names (e.g. 'SIGKILL') or they are names with platforms at the end (e.g. 'SIGCHLD@linux'). They are in the latter if they aren't in 'supportedSignals', which appears to be a global.

The 'SIG_' guard is there because the Python signal module has variables that start with 'SIG_' that aren't the names of signals.

OK, I think I've got it. Here are the recommendations:

• Make supportedSignals 'SUPPORTED_SIGNALS'.
• If 'supported' really means 'all platforms', then say so.
• The self.class thing smells like premature optimization. Just use self.

+ def outReceived(self, data): + """ + Sent data from standard out over the channel. + """ self.session.write(data) + def errReceived(self, err): + """ + Send data from standard error as extended data of type + EXTENDED_DATA_STDERR. + """ self.session.writeExtended(connection.EXTENDED_DATA_STDERR, err) + def inConnectionLost(self): + """ + If we're told the incoming connection has been lost, send an EOF + over the channel. + """ self.session.conn.sendEOF(self.session) - def connectionLost(self, reason = None): - self.session.loseConnection() - def processEnded(self, reason = None): - if reason and hasattr(reason.value, 'exitCode'): - log.msg('exitCode: %s' % repr(reason.value.exitCode)) - self.session.conn.sendRequest(self.session, 'exit-status', struct.pack('!L', reason.value.exitCode)) + def outConnectionLost(self): + """ + If we're running as an subsystem, close the connection. + """ + if self.session.session is None: # we're running as an old-style + # subsystem + self.session.loseConnection() + + + def processEnded(self, reason=None): + """ + When we are told the process ended, try to notify the other side about + how the process ended using the exit-signal or exit-status requests. + Also, close the channel. + """ + if reason is not None and hasattr(reason.value, 'exitCode'): + err = reason.value + if err.signal is not None: + signame = self._getSignalName(err.signal) + log.msg('exitSignal: %s' % (signame,)) + if hasattr(os, 'WCOREDUMP') and os.WCOREDUMP(err.status): + coreDumped = 1 + else: + coreDumped = 0

If core was dumped, the log should say so. This is very useful information for debugging.

+ self.session.conn.sendRequest(self.session, 'exit-signal', + common.NS(signame[3:]) + chr(coreDumped) + + common.NS() + common.NS()) + elif err.exitCode is not None: + log.msg('exitCode: %r' % (err.exitCode,)) + self.session.conn.sendRequest(self.session, 'exit-status', + struct.pack('>L', err.exitCode))

What if 'err' has no exitCode and no signal?

self.session.loseConnection() - # transport stuff (we are also a transport!) + # also a transport :(

I share your sadness, but not your understanding. Why must this be a transport? What can we do about it? Please explain these things in the comment.

def write(self, data): + """ + If we're used like a transport, just send the data to the channel. + """ self.session.write(data) - def writeSequence(self, seq): - self.session.write(.join(seq)) def loseConnection(self): + """ + If we're use like a transport, send the close message. + """

'used', not 'use'.

self.session.loseConnection() -class SSHSessionClient(protocol.Protocol): + +class SSHSessionProcessProtocolApplication: + """ + Another layer of wrapping to make the old-style ISession + implemention work. This adapts SSHSessionProcessProtocol to + ISessionApplication. + + @ivar processProtocol: the C{SSHSessionProcessProtocol} we're adapting. + """ +

Gosh there are a lot of these. I think it would be worth giving a summary in the module-level docstring. I'm starting to find it all a bit confusing.

+ + implements(ISessionApplication) + + + def init(self, processProtocol): + self.processProtocol = processProtocol + + + def connectionMade(self): + """ + Don't need to do anything because SSHSessionProcessProtocol's + transport doesn't do anything with this. + """ + + def dataReceived(self, data): - if self.transport: - self.transport.write(data) + """ + When we get data, write it to the SSHSessionProcessProtocol's + transport. + """ + self.processProtocol.transport.write(data) + + def extendedDataReceived(self, dataType, data): + """ + If we get extended data from standard error and the transport + has a writeErr method, pass the data along. + """ + if not hasattr(self.processProtocol.transport, 'writeErr'): + return + if dataType == connection.EXTENDED_DATA_STDERR: + self.processProtocol.transport.writeErr(data) + + + def eofReceived(self): + """ + Don't need to implement this because + SSHSessionProcessProtocol's transport doesn't do anything with + this. + """ + + + def closeReceived(self): + """ + Don't need to implement this because + SSHSessionProcessProtocol's transport doesn't do anything with + this. + """ + + + def closed(self): + """ + Don't need to implement this because + SSHSessionProcessProtocol's transport doesn't do anything with + this. + """ +

No adapter registration?

+ + +class SSHSessionClient(protocol.Protocol): + """ + A class the conch command-line client uses to connect the channel + to standard output. Deprecated. + """ + def dataReceived(self, data): + """ + Send data to the transport. + """ + self.transport.write(data) +

If it's deprecated, raise a warning on construction.

+ + # methods factored out to make live easier on server writers def parseRequest_pty_req(data): - """Parse the data from a pty-req request into usable data. + """ + Parse the data from a pty-req request into usable data. @returns: a tuple of (terminal type, (rows, cols, xpixel, ypixel), modes) """

Please say where 'terminal type' and 'modes' come from.

By the way, there's no harm at all in referring to the RFC number and section in a docstring, especially for a protocol as complex as SSH.

@@ -226,30 +900,48 @@ cols, rows, xpixel, ypixel = struct.unpack('>4L', rest[: 16]) modes, ignored= common.getNS(rest[16:]) winSize = (rows, cols, xpixel, ypixel) - modes = [(ord(modes[i]), struct.unpack('>L', modes[i+1: i+5])[0]) for i in range(0, len(modes)-1, 5)] + modes = [(ord(modes[i]), struct.unpack('>L', modes[i+1: i+5])[0]) for i in + range(0, len(modes)-1, 5)]

No space after the colon, spaces around the '-' in 'len(modes)-1'.

return term, winSize, modes + + def packRequest_pty_req(term, (rows, cols, xpixel, ypixel), modes): - """Pack a pty-req request so that it is suitable for sending. + """ + Pack a pty-req request so that it is suitable for sending. NOTE: modes must be packed before being sent here. """ termPacked = common.NS(term) winSizePacked = struct.pack('>4L', cols, rows, xpixel, ypixel) - modesPacked = common.NS(modes) # depend on the client packing modes + if not isinstance(modes, str): + modes = .join([chr(m[0]) + struct.pack('>L', m[1]) for m in modes]) + else: + warnings.warn("packRequest_pty_req should be packing the modes.", + DeprecationWarning, stacklevel=2)

There's an API for this now: twisted.python.deprecate.

+ modesPacked = common.NS(modes) return termPacked + winSizePacked + modesPacked + + def parseRequest_window_change(data): - """Parse the data from a window-change request into usuable data. + """ + Parse the data from a window-change request into usuable data. @returns: a tuple of (rows, cols, xpixel, ypixel) """ cols, rows, xpixel, ypixel = struct.unpack('>4L', data) return rows, cols, xpixel, ypixel + + def packRequest_window_change((rows, cols, xpixel, ypixel)): - """Pack a window-change request so that it is suitable for sending. """ + Pack a window-change request so that it is suitable for sending. + """ return struct.pack('>4L', cols, rows, xpixel, ypixel) -import connection + + +all = ['SSHSession', 'packRequest_window_change', 'packRequest_pty_req', + 'parseRequest_window_change', 'parseRequest_pty_req']

I think the world would be a happier place if this were at the top of the file.

Index: twisted/conch/avatar.py =================================================================== --- twisted/conch/avatar.py (revision 23210) +++ twisted/conch/avatar.py (working copy) @@ -1,9 +1,11 @@ # -*- test-case-name: twisted.conch.test.test_conch -*- -from interfaces import IConchUser -from error import ConchError -from ssh.connection import OPEN_UNKNOWN_CHANNEL_TYPE -from twisted.python import log +# Copyright (c) 2007 Twisted Matrix Laboratories. +# See LICENSE for details. +

2008 :)

from zope import interface +from twisted.conch.interfaces import IConchUser +from twisted.conch.error import ConchError +from twisted.conch.ssh.connection import OPEN_UNKNOWN_CHANNEL_TYPE class ConchUser: interface.implements(IConchUser) @@ -17,12 +19,16 @@ if not klass: raise ConchError(OPEN_UNKNOWN_CHANNEL_TYPE, "unknown channel") else: - return klass(remoteWindow = windowSize, - remoteMaxPacket = maxPacket, + return klass(remoteWindow=windowSize, + remoteMaxPacket=maxPacket, data=data, avatar=self) def lookupSubsystem(self, subsystem, data): - log.msg(repr(self.subsystemLookup)) + """ + This is deprecated in favor of ISession.lookupSubsystem. However, + because there is already a warning in SSHSession.request_subsystem, + we do not warn again here. + """ klass = self.subsystemLookup.get(subsystem, None) if not klass: return False Index: twisted/conch/interfaces.py =================================================================== --- twisted/conch/interfaces.py (revision 23210) +++ twisted/conch/interfaces.py (working copy) @@ -4,7 +4,7 @@ from zope.interface import Interface class IConchUser(Interface): - """A user who has been authenticated to Cred through Conch. This is + """A user who has been authenticated to Cred through Conch. This is the interface between the SSH connection and the user. @ivar conn: The SSHConnection object for this user. @@ -30,26 +30,24 @@ @rtype: subclass of L{SSHChannel}/C{tuple} """ - def lookupSubsystem(subsystem, data): - """ - The other side requested a subsystem. - subsystem is the name of the subsystem being requested. - data is any other packet data (often nothing). - - We return a L{Protocol}. - """ + def gotGlobalRequest(requestType, data): """ A global request was sent from the other side. - + By default, this dispatches to a method 'channel_channelType' with any - non-alphanumerics in the channelType replace with _'s. If it cannot - find a suitable method, it returns an OPEN_UNKNOWN_CHANNEL_TYPE error. + non-alphanumerics in the channelType replace with _'s. If it cannot + find a suitable method, it returns an OPEN_UNKNOWN_CHANNEL_TYPE error. The method is called with arguments of windowSize, maxPacket, data. """ + class ISession(Interface): + """ + The old, deprecated interface for implementing SSH session applications. + Please don't use this for anything. + """

Please say which version it was deprecated in.

def getPty(term, windowSize, modes): """ @@ -82,13 +80,143 @@ """ Called when the other side has indicated no more data will be sent. """ - + def closed(): """ Called when the session is closed. """ + + +class ISessionApplicationFactory(Interface): + """ + The new, good interface for implementing an SSH session. Use this to + implement the application side of an SSH server session. + + @ivar channel: the L{SSHChannel} object to which this application is + connected. Use this attribute to send data, etc. + """ + + def getPTY(term, windowSize, modes): + """ + Get a psuedo-terminal for use by a shell or command. + + If a psuedo-terminal is not available, or the request otherwise + fails, raise an exception. + """ +

"pseudo", not "psuedo".

+ + def lookupSubsystem(subsystem, data): + """ + The other side requested a subsystem.

Blank line.

+ @param subsystem: the name of the subsystem being requested. + @param data: any other packet data (often nothing). + + @return: something that implements I{IConchApplication} + @raises: If the subsystem is unavailable, raise an exception. + """ + + + def openShell(): + """ + The other side requested a shell. + + @return: something that implementes I{IConchApplication} + @raises: If the subsystem is unavailable, raise an exception. + """ + + + def execCommand(command): + """ + The other side requested a shell. + + @param command: the command the other side wants us to execute + @type command: C{str} + + @return: something that implementes I{IConchApplication} + @raises: If the subsystem is unavailable, raise an exception. + """ + + + def windowChanged(newWindowSize): + """ + Called when the size of the remote screen has changed. + """ + + + def eofReceived(): + """ + Called when the other side has indicated no more data will be sent. + """ + + + def closeReceived(): + """ + Called when the other side has indicated it wants no more data. + """ + + + def closed(): + """ + Called when the session channel is closed. + """ + +

One more blank line please.

+class ISessionApplication(Interface): + """ + Objects which are returned by an ISessionApplicationFactory implementor + should implement this class. It receives data from the channel and can + send it back. + + @ivar channel: the C{SSHSession} to which this application is connected. + """ + + + def connectionMade(): + """ + Called when this application is connected to the channel. + """ + + + def dataReceived(data): + """ + Called when data is received from the other side. + + @type data: C{str} + """ + + + def extendedDataReceived(extendedDataType, extendedData): + """ + Called when extended data is received from the other side. + + @param extendedDataType: the type of extended data. Typically, this + is connection.EXTENDED_DATA_STDERR. + @type extendedDataType: C{int} + @type extendedData: C{str} + """ + + + def eofReceived(): + """ + Called when the other side has said it will not send any more data. + """ + + + def closeReceived(): + """ + Called when the other side has said it will not accept any more data. + """ + + + def closed(): + """ + Called when the application is closed on both sides. + """ + + + class ISFTPServer(Interface): """ The only attribute of this class is "avatar". It is the avatar @@ -350,5 +478,3 @@ @param attrs: a dictionary in the same format as the attrs argument to L{openFile}. """ - - Index: twisted/conch/unix.py =================================================================== --- twisted/conch/unix.py (revision 23210) +++ twisted/conch/unix.py (working copy) @@ -5,19 +5,20 @@ from twisted.python import components, log from twisted.internet.error import ProcessExitedAlready from zope import interface -from ssh import session, forwarding, filetransfer -from ssh.filetransfer import FXF_READ, FXF_WRITE, FXF_APPEND, FXF_CREAT, FXF_TRUNC, FXF_EXCL +from twisted.conch.ssh import session, forwarding, filetransfer +from twisted.conch.ssh.filetransfer import FXF_READ, FXF_WRITE, FXF_APPEND +from twisted.conch.ssh.filetransfer import FXF_CREAT, FXF_TRUNC, FXF_EXCL from twisted.conch.ls import lsLine -from avatar import ConchUser -from error import ConchError -from interfaces import ISession, ISFTPServer, ISFTPFile +from twisted.conch.avatar import ConchUser +from twisted.conch.error import ConchError +from twisted.conch.interfaces import ISession, ISFTPServer, ISFTPFile +from twisted.conch import ttymodes import struct, os, time, socket import fcntl, tty import pwd, grp import pty -import ttymodes try: import utmp @@ -48,9 +49,6 @@ {"session": session.SSHSession, "direct-tcpip": forwarding.openConnectForwardingClient}) - self.subsystemLookup.update( - {"sftp": filetransfer.FileTransferServer}) - def getUserGroupId(self): return self.pwdData[2:4] @@ -67,10 +65,10 @@ hostToBind, portToBind = forwarding.unpackGlobal_tcpip_forward(data) from twisted.internet import reactor try: listener = self._runAsUser( - reactor.listenTCP, portToBind, + reactor.listenTCP, portToBind, forwarding.SSHListenForwardingFactory(self.conn, (hostToBind, portToBind), - forwarding.SSHListenServerForwardingChannel), + forwarding.SSHListenServerForwardingChannel), interface = hostToBind) except: return 0 @@ -95,7 +93,8 @@ # remove all listeners for listener in self.listeners.itervalues(): self._runAsUser(listener.stopListening) - log.msg('avatar %s logging out (%i)' % (self.username, len(self.listeners))) + log.msg('avatar %s logging out (%i)' % (self.username, + len(self.listeners))) def _runAsUser(self, f, *args, **kw): euid = os.geteuid() @@ -160,7 +159,6 @@ b = utmp.UtmpRecord(utmp.WTMP_FILE) b.pututline(entry) b.endutent() - def getPty(self, term, windowSize, modes): self.environTERM? = term @@ -168,9 +166,13 @@ self.modes = modes master, slave = pty.openpty() ttyname = os.ttyname(slave) - self.environSSH_TTY? = ttyname + self.environSSH_TTY? = ttyname self.ptyTuple = (master, slave, ttyname) + def lookupSubsystem(subsystem, data): + if subsystem == 'sftp': + return filetransfer.FileTransferServer(data) + def openShell(self, proto): from twisted.internet import reactor if not self.ptyTuple: # we didn't get a pty-req @@ -191,7 +193,7 @@ shell, ['-%s' % shellExec], self.environ, homeDir, uid, gid, usePTY = self.ptyTuple) self.addUTMPEntry() - fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ, + fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ, struct.pack('4H', *self.winSize)) if self.modes: self.setModes() @@ -232,7 +234,7 @@ finally: os.setegid(egid) os.seteuid(euid) - + def setModes(self): pty = self.pty attr = tty.tcgetattr(pty.fileno()) @@ -276,7 +278,7 @@ def windowChanged(self, winSize): self.winSize = winSize - fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ, + fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ, struct.pack('4H', *self.winSize)) def _writeHack(self, data):

Here the diff ends, what follows is the added test file.

=== twisted/conch/test/test_session.py === # Copyright (c) 2007-2008 Twisted Matrix Laboratories. # See LICENSE for details. """ Tests for the 'session' channel implementation in twisted.conch.ssh.session. """

This is a good place to refer to the RFC.

import os, signal, sys, struct from zope.interface import implements from twisted.conch.ssh import common, connection, session from twisted.internet import defer, protocol, error from twisted.python import components, failure from twisted.trial import unittest class SubsystemOnlyAvatar(object): """ A stub class representing an avatar that is only useful for getting a subsystem. """ def lookupSubsystem(self, name, data): """ If the other side requests the 'subsystem' subsystem, allow it by returning a MockProcessProtocol to implement it. Otherwise, fail. """ if name == 'subsystem': return MockProcessProtocol()

When I read 'fail', I think 'raise some sort of error', not 'return None'. The docstring would be more helpful if it explained the impact of returning None in this case.

class StubOldAvatar(object): """ A stub class representing the avatar representing the authenticated user. """

Please explain why it's 'Old'.

def lookupSubsystem(self, name, data): """ If the user requests the TestSubsystem subsystem, connect them to our MockProcessProtocol. If they request the protocol subsystem, connect them to a MockProtocol. """ if name == 'TestSubsystem': self.subsystem = MockProcessProtocol() self.subsystem.packetData = data return self.subsystem elif name == 'protocol': self.subsystem = MockProtocol() self.subsystem.packetData = data return self.subsystem

And if they request neither, what happens?

class StubSessionForStubOldAvatar(object): """ A stub ISession implementation for our StubOldAvatar. @ivar avatar: the C{StubOldAvatar} we are adapting. @ivar ptyRequest: if present, the terminal, window size, and modes passed to the getPty method. @ivar windowChange: if present, the window size passed to the windowChangned method. @ivar shellProtocol: if present, the C{SSHSessionProcessProtocol} passed to the openShell method. @ivar shellTransport: if present, the C{EchoTransport} connected to shellProtocol. @ivar execProtocol: if present, the C{SSHSessionProcessProtocol} passed to the execCommand method. @ivar execTransport: if present, the C{EchoTransport} connected to execProtocol. @ivar execCommandLine: if present, the command line passed to the execCommand method. @ivar gotEOF: if present, an EOF message was received. @ivar gotClosed: if present, a closed message was received. """

I guess you have all of those here so you can manipulate them from tests.

Looking down, I guessed wrong. These are like a log of the things that have been done to this session object. It's worth making a general comment about this in the docstring.

You should also mention that certain calls to certain methods will trigger errors.

Note that a more usual way of doing this is to have a 'log' or 'calls' attribute that appends information about the calls to a list. No need to change it though.

implements(session.ISession) def init(self, avatar): """ Store the avatar we're adapting. """ self.avatar = avatar self.shellProtocol = None def getPty(self, terminal, window, modes): """ If the terminal is 'bad', fail. Otherwise, store the information in the ptyRequest variable. """ if terminal != 'bad': self.ptyRequest = (terminal, window, modes) else: raise RuntimeError('not getting a pty') def windowChanged(self, window): """ If all the window sizes are 0, fail. Otherwise, store the size in the windowChange variable. """ if window == (0, 0, 0, 0): raise RuntimeError('not changing the window size') else: self.windowChange = window def openShell(self, pp): """ If we have gotten a shell request before, fail. Otherwise, store the process protocol in the shellProtocol variable, connect it to the EchoTransport and store that as shellTransport. """ if self.shellProtocol is not None: raise RuntimeError('not getting a shell this time') else: self.shellProtocol = pp self.shellTransport = EchoTransport(pp) def execCommand(self, pp, command): """ If the command is 'true', store the command, the process protocol, and the transport we connect to the process protocol. Otherwise, just store the command and raise an error. """ self.execCommandLine = command if command == 'true': self.execProtocol = pp elif command[:4] == 'echo': self.execProtocol = pp self.execTransport = EchoTransport(pp) pp.outReceived(command[5:]) else: raise RuntimeError('not getting a command') def eofReceived(self): """ Note that EOF has been received. """ self.gotEOF = True def closed(self): """ Note that close has been received. """ self.gotClosed = True components.registerAdapter(StubSessionForStubOldAvatar, StubOldAvatar, session.ISession) class StubNewAvatar(object): """ A stub avatar. It does not need any methods, as it is only used as the object StubSessionForStubNewAvatar adapts. """ class StubApplicationFactoryForStubNewAvatar: """ A stub ISessionApplicationFactory implementation for our StubNewAvatar. @ivar avatar: the C{StubOldAvatar} we are adapting. @ivar inConnectionOpen: C{True} if the input side is open. @ivar outConnectionOpen: C{True} if the output side is open. @ivar ended: C{True} if the session has ended. @ivar subsystem: if present, the C{MockApplication} that is the current subsystem. It has a packetData ivar which is the C{str} of data passed in the subsystem request. @ivar term: if present, the terminal name passed to getPty @ivar windowSize: if present, the window size passed to getPty or windowChanged @ivar modes: if present, the terminal modes passed to getPty @ivar command: if present, the C{MockApplication} that is the current command. It has a command ivar which is the C{str} giving the command's name. @ivar shell: if present, the C{MockApplication} that is the current shell. """ implements(session.ISessionApplicationFactory) def init(self, avatar): self.avatar = avatar self.inConnectionOpen = True self.outConnectionOpen = True self.ended = False self.subsystem = None self.command = None self.shell = None def lookupSubsystem(self, subsystem, data): """ Request a subsystem. If one has been requested already, raise an exception. Otherwise, set self.subsystem to a MockApplication and return it. """ if self.subsystem is not None: raise ValueError('already opened a subsystem') if subsystem == 'echo': self.subsystem = MockApplication() self.subsystem.packetData = data return self.subsystem def getPTY(self, term, windowSize, modes): """ Request a psuedoterminal. Store the data passed to us. """

"pseudo", not "psuedo".

self.term = term self.windowSize = windowSize self.modes = modes def windowChanged(self, newSize): """ The window size has changed. Store the data. """ self.windowSize = newSize def execCommand(self, command): """ Request a command. If one has been requested already, raise an exception. Otherwise, set self.command to a MockApplication and return it. """ if self.command is not None: raise RuntimeError('already executed a command') self.command = MockApplication() self.command.command = command return self.command def openShell(self): """ Request a shell. If one has been requested already, raise an exception. Otherwise, set self.shell to a MockApplication and return it. """ if self.shell is not None: raise RuntimeError('already opened a shell') self.shell = MockApplication() return self.shell def eofReceived(self): """ Close the input side. """ self.inConnectionOpen = False def closeReceived(self): """ Close the output side. """ self.outConnectionOpen = False def closed(self): """ End the session. """ self.ended = True

I see you've taken the same approach here. The same comments apply.

components.registerAdapter(StubApplicationFactoryForStubNewAvatar, StubNewAvatar, session.ISessionApplicationFactory) class MockApplication: """ A mock ISessionApplication. This is the new interface that clients of SSHSession should implement. @ivar data: a C{list} of C{str} passed to our dataReceived. @ivar extendedData: a C{list} of C{tuple} of (C{int}, C{str}) passed to our extendedDataReceived. @ivar hasClosed: a C{bool} indicating whether the application is closed. @ivar gotEOF: if present, we have received an EOF from the other side. @ivar gotClose: if present, we have received a close message from the other side """ implements(session.ISessionApplication) def connectionMade(self): """ Called when the application is connected to the other side. Initialize our instance variables. """ self.data = [] self.extendedData = [] self.hasClosed = False def dataReceived(self, data): """ We got some data. Store it, and echo it back with a tilde appended. """

Why?

self.data.append(data) self.channel.write(data + '~') def extendedDataReceived(self, type, data): """ We got some extended data. Store it, and echo it back with an incremented data type and with a tilde appended to the data. """

Again, please say _why_ you are appending a tilde.

self.extendedData.append((type, data)) self.channel.writeExtended(type + 1, data + '~') def eofReceived(self): """ Note that we received an EOF. """ self.gotEOF = True def closeReceived(self): """ Note that we received a close message. """ self.gotClose = True def closed(self): """ Note that this application is closed. """ self.hasClosed = True class MockApplicationSendingOnConnection(MockApplication): """ This is a a simple subclass of MockApplication which sends some data when it is connected. """ def connectionMade(self): """ Write an introduction when we are connected. """ MockApplication.connectionMade(self) self.channel.write('intro') class MockProcessProtocol(protocol.ProcessProtocol): """ A mock ProcessProtocol which echoes back data sent to it and appends a tilde.

I'm beginning to suspect a tilde fetish. Those sleek curves, that wavy form, the way it reminds me of home...

Say why you are adding a tilde. What's the intent?

@ivar data: a C{str} of data received. @ivar err: a C{str} of error data received. @ivar inConnectionOpen: True if the input side is open. @ivar outConnectionOpen: True if the output side is open. @ivar errConnectionOpen: True if the error side is open. @ivar ended: False if the protocol has not ended, a C{Failure} if the process has ended. """ packetData = def connectionMade(self): """ Set up variables. """ self.data = self.err = self.inConnectionOpen = True self.outConnectionOpen = True self.errConnectionOpen = True self.ended = False if self.packetData: self.outReceived(self.packetData) def outReceived(self, data): """ Data was received. Store it and echo it back with a tilde. """ self.data += data self.transport.write(data + '~') def errReceived(self, data): """ Error data was received. Store it and echo it back backwards. """ self.err += data self.transport.write(data[::-1]) def inConnectionLost(self): """ Close the input side. """ self.inConnectionOpen = False def outConnectionLost(self): """ close the output side. """

'Close', not 'close'.

self.outConnectionOpen = False def errConnectionLost(self): """ Close the error side. """ self.errConnectionOpen = False def processEnded(self, reason): """ End the process and store the reason. """ self.ended = reason class EchoTransport: """ A transport for a ProcessProtocol which echos data that is sent to it with a Window newline (\\r\\n) appended to it. If a null byte is in the data,

The normal way to say this is 'CR LF'.

disconnect. When we are asked to disconnect, disconnect the C{ProcessProtocol} with a 0 exit code. @ivar proto: the C{ProcessProtocol} connected to us. @ivar data: a C{str} of data written to us. """ def init(self, p): """ Initialize our instance variables. @param p: a C{ProcessProtocol} to connect to ourself. """ self.proto = p p.makeConnection(self) self.closed = False self.data =

If you feel like it, rename 'p' to 'processProtocol'.

def write(self, data): """ We got some data. Give it back to our C{ProcessProtocol} with a newline attached. Disconnect if there's a null byte. """ self.data += data self.proto.outReceived(data) self.proto.outReceived('\r\n') if '\x00' in data: # mimic 'exit' for the shell test self.loseConnection() def loseConnection(self): """ If we're asked to disconnect (and we haven't already) shut down the C{ProcessProtocol} with a 0 exit code. """ if self.closed: return

Put a newline after the colon.

self.closed = 1 self.proto.inConnectionLost() self.proto.outConnectionLost() self.proto.errConnectionLost() self.proto.processEnded(failure.Failure( error.ProcessTerminated(0, None, None))) class MockProtocol(protocol.Protocol): """ A sample Process which stores the data passed to it.

This isn't a 'Process'. Perhaps you mean 'protocol'?

@ivar data: a C{str} of the data passed to us. @ivar open: True if the channel is open. @ivar reason: if not None, the reason the protocol was closed. """ packetData =

packetData lacks documentation. What is it? Why is it there?

def connectionMade(self): """ Set up the instance variables. """

Not the most helpful docstring in the world. Nothing obviously better springs to mind. I see you use this in other places too.

self.data = self.open = True self.reason = None if self.packetData: self.dataReceived(self.packetData) def dataReceived(self, data): """ Store the received data. """ self.data += data self.transport.write(data + '~')

Hey look! It's that tilde again!

def connectionLost(self, reason): """ Close the protocol and store the reason. """ self.open = False self.reason = reason

At this point I'm really starting to look forward to reading the tests.

class StubConnection: """ A stub for twisted.conch.ssh.connection.SSHConnection. Record the data that channels send, and when they try to close the connection. @ivar data: a C{dict} mapping C{SSHChannel}s to a C{list} of C{str} of data they sent. @ivar extData: a C{dict} mapping C{SSHChannel}s to a C{list} of C{tuple} of (C{int}, C{str}) of extended data they sent. @ivar requests: a C{dict} mapping C{SSHChannel}s to a C{list} of C{tuple} of (C{str}, C{str}) of channel requests they made. @ivar eofs: a C{dict} mapping C{SSHChannel}s to C{true} if they have sent an EOF. @ivar closes: a C{dict} mapping C{SSHChannel}s to C{true} if they have sent a close. """ def init(self): """ Initialize our instance variables. """ self.data = {} self.extData = {} self.requests = {} self.eofs = {} self.closes = {} def logPrefix(self): """ Return our logging prefix. """ return "MockConnection" def sendData(self, channel, data): """ Record the sent data. """ self.data.setdefault(channel, []).append(data) def sendExtendedData(self, channel, type, data): """ Record the sent extended data. """ self.extData.setdefault(channel, []).append((type, data)) def sendRequest(self, channel, request, data, wantReply=False): """ Record the sent channel request. """ self.requests.setdefault(channel, []).append((request, data, wantReply)) if wantReply: return defer.succeed(None) def sendEOF(self, channel): """ Record the sent EOF. """ self.eofs[channel] = True def sendClose(self, channel): """ Record the sent close. """ self.closes[channel] = True class StubTransport: """ A stub transport which records the data written. @ivar buf: the data sent to the transport. @type buf: C{str} @ivar err: the extended data sent to the transport. @type err: C{str} @ivar close: flags indicating if the transport has been closed. @type close: C{bool} """ buf = err = close = False def write(self, data): """ Record data in the buffer. """ self.buf += data def writeErr(self, data): """ Record the extended data in the buffer. """ self.err += data

It records extended data and it's called 'writeErr'? That seems weird. Can you please explain the discrepancy in the docstring.

def loseConnection(self): """ Note that the connection was closed. """ self.close = True class StubClient(object): """ A stub class representing the client to a SSHSession. @ivar transport: A C{StubTransport} object which keeps track of the data passed to it. """ def init(self): self.transport = StubTransport() class OldSessionInterfaceTestCase(unittest.TestCase): """ Tests for the old SSHSession class interface. This interface is not ideal, but it is tested in order to maintain backwards compatibility. """

Thanks for the docstring, it helps.

def setUp(self): """ Make an SSHSession object to test. Give the chanel some window so that it's allowed to send packets. """

"channel", not "chanel".

If the numbers are arbitrary, say so. If they aren't, say why they were chosen.

self.session = session.SSHSession(remoteWindow=500, remoteMaxPacket=100, conn=StubConnection(), avatar=StubOldAvatar()) def test_init(self): """ Test that SSHSession initializes its buffer (buf), client, and ISession adapter. The avatar should not need to be adaptable to an ISession immediately. """ s = session.SSHSession(avatar=object) # use object because it doesn't # have an adapter

I don't like this style of comment, and it doesn't appear much in the rest of Twisted. It's no big deal though.

A comment here that applies to all of your other tests:

Don't say "Test that" in the docstring of a test. We know it's a test. Saying "test that" or "check that" or "foo should bar" encourages one to write docstrings that speak in terms of implemention.

Instead, describe the behaviour. "SSHSession initializes its buffer, ...".

self.assertEquals(s.buf, ) self.assertIdentical(s.client, None) self.assertIdentical(s.session, None) def _testGetsSession(self): """ Check that a function will cause SSHSession to generate a session. We return a C{tuple} of a C{SSHSession} and aC{Deferred} which expects

Add a space after 'a'.

to be called back with a function and arguments to be called. After the function is called, the C{SSHSession} should have a session attribute which provides I{ISession}. """

This would be a good spot to say *why* you need this helper.

s = session.SSHSession() d = defer.Deferred() def check(fargs):

Say "functionAndArgs", please. For me.

Actually, maybe it's better to assume it's a nullary function and then use lambdas when you need to invoke it.

f = fargs[0] args = fargs[1:] s.avatar = StubOldAvatar() self.assertIdentical(s.session, None) f(*args)

At this point I'm curious, what sort of function do we expect here?

self.assertTrue(session.ISession.providedBy(s.session)) d.addCallback(check) return s, d

This is one weird function.

def test_requestShellGetsSession(self): """ If an ISession adapter isn't already present, request_shell should get one. """ s, d = self._testGetsSession() return d.callback((s.requestReceived, 'shell', )) def test_requestExecGetsSession(self): """ If an ISession adapter isn't already present, request_exec should get one. """ s, d = self._testGetsSession() return d.callback((s.requestReceived, 'exec', common.NS('true'))) def test_requestPtyReqGetsSession(self): """ If an ISession adapter isn't already present, request_pty_req should get one. """ s, d = self._testGetsSession() return d.callback((s.requestReceived, 'pty_req', session.packRequest_pty_req( 'term', (0, 0, 0, 0), []))) def test_requestWindowChangeGetsSession(self): """ If an ISession adapter isn't already present, request_window_change should get one. """ s, d = self._testGetsSession() return d.callback((s.requestReceived, 'window_change', session.packRequest_window_change((1, 1, 1, 1))))

This is a long, long way from idiomatic Twisted code.

 http://www.osnews.com/images/comics/wtfm.jpg

Looking back at _testGetsSession, everything up to and including the assertIdentical line could easily go into setUp. Then, the tests could simply call the methods they are testing and then do the assertion manually.

From all I can see, the Deferred is a red herring.

def test_client(self): """ Test that SSHSession passes data and extended data along to a client. """ self.session.dataReceived('1') self.session.extReceived(connection.EXTENDED_DATA_STDERR, '3') # these don't get sent as part of the client interface self.session.client = StubClient() self.session.dataReceived('2') self.session.extReceived(connection.EXTENDED_DATA_STDERR, '4') self.assertEquals(self.session.client.transport.buf, '2') self.assertEquals(self.session.client.transport.err, '4') self.session.eofReceived() self.assertTrue(self.session.conn.closes[self.session]) self.session.closed() self.assertTrue(self.session.client.transport.close) self.session.client.transport.close = False

Please split this into three tests, one for each assert block.

def test_lookupSubsystem(self): """ When a client requests a subsystem, the SSHSession object should get the subsystem by calling avatar.lookupSubsystem, and attach it as the client. """ self.session.session = None # old SSHSession didn't have this attribute self.session.dataReceived('1') self.session.extReceived(connection.EXTENDED_DATA_STDERR, '2') self.assertFalse(self.session.requestReceived( 'subsystem', common.NS('BadSubsystem'))) self.assertIdentical(self.session.client, None)

Please add a comment explaining why you have this assertion.

self.assertTrue(self.session.requestReceived( 'subsystem', common.NS('TestSubsystem') + 'data')) self.assertIsInstance(self.session.client, protocol.ProcessProtocol) self.assertIdentical(self.session.sessionApplication.processProtocol, self.session.avatar.subsystem) self.assertEquals(self.session.conn.data[self.session], \x00\x00\x00\x0dTestSubsystemdata~?) self.session.dataReceived('more data') self.assertEquals(self.session.conn.data[self.session][-1], 'more data~') self.session.closeReceived() self.assertTrue(self.session.conn.closes[self.session])

This test is too big. Please split it up.

def test_lookupSubsystemProtocol(self): """ Test that lookupSubsystem correctly handles being returned a Protocol. """

Please define "correctly" in the docstring.

self.session.session = None # old SSHSession didn't have this attribute self.session.dataReceived('1') self.session.extReceived(connection.EXTENDED_DATA_STDERR, '2') self.assertIdentical(self.session.client, None) self.assertTrue(self.session.requestReceived( 'subsystem', common.NS('protocol') + 'data')) self.assertIsInstance(self.session.client, protocol.ProcessProtocol) self.assertIdentical( self.session.sessionApplication.processProtocol.proto, self.session.avatar.subsystem) self.assertEquals(self.session.sessionApplication.processProtocol.connectionLost, self.session.sessionApplication.processProtocol.proto.connectionLost) self.session.sessionApplication.processProtocol.foo = 'foo' self.assertEquals(self.session.sessionApplication.processProtocol.proto.foo, 'foo') self.assertIdentical( self.session.sessionApplication.processProtocol.transport.proto, self.session.sessionApplication.processProtocol) self.assertEquals(self.session.conn.data[self.session], \x00\x00\x00\x08protocoldata~?) self.session.dataReceived('more data') self.assertEquals(self.session.conn.data[self.session][-1], 'more data~') self.session.closeReceived() self.assertTrue(self.session.conn.closes[self.session]) self.session.closed() self.assertEquals(self.session.sessionApplication.processProtocol.proto.reason, protocol.connectionDone)

Please split this test up.

def test_lookupSubsystemDoesNotNeedISession(self): """ Previously, if one only wanted to implement a subsystem, an ISession adapter wasn't needed. """ s = session.SSHSession(avatar=SubsystemOnlyAvatar(), conn=StubConnection()) self.assertTrue(s.request_subsystem(common.NS('subsystem') + 'data')) self.assertNotIdentical(s.applicationFactory, None) self.assertIdentical(s.conn.closes.get(s), None) s.eofReceived() self.assertTrue(s.conn.closes.get(s)) # these should not raise errors s.closeReceived() s.closed()

Testing negatives is always tricky. I think you could explain better exactly how you *aren't* using ISession here, and highlight the bits that you think should blow up.

681 lines to go. Time for breakfast.

def test_requestShell(self): """ When a client requests a shell, the SSHSession object should get the shell by getting an ISession adapter for the avatar, then calling openShell() with a ProcessProtocol to attach. """ # gets a shell the first time self.assertTrue(self.session.requestReceived('shell', )) self.assertIsInstance(self.session.session, StubSessionForStubOldAvatar) self.assertIsInstance(self.session.client, session.SSHSessionProcessProtocol) self.assertIdentical(self.session.session.shellProtocol, self.session.client) # doesn't get a shell the second time self.assertFalse(self.session.requestReceived('shell', )) errors = self.flushLoggedErrors()

You should specify the type of error that you are flushing here, so that you don't accidentally flush a real error.

self.assertEquals(len(errors), 1) errors[0].trap(RuntimeError)

Part of the difficulty in reviewing this is that there are so many assertions in these tests, which means they are testing many things.

If you can think of one, please extract custom assert* methods that can express your intent at a higher level.

def test_requestShellWithData(self): """ When a client executes a shell, it should be able to give pass data back and forth. """

"back and forth" is usually followed by "between X and Y". Please say where the data is going to and coming from.

self.assertTrue(self.session.requestReceived('shell', )) self.assertIsInstance(self.session.session, StubSessionForStubOldAvatar) self.session.dataReceived('some data\x00') self.assertEquals(self.session.session.shellTransport.data, 'some data\x00') self.assertEquals(self.session.conn.data[self.session], ['some data\x00', '\r\n']) self.assertTrue(self.session.session.shellTransport.closed) self.assertEquals(self.session.conn.requests[self.session], [('exit-status', '\x00\x00\x00\x00', False)]) def test_requestExec(self): """ When a client requests a command, the SSHSession object should get the command by getting an ISession adapter for the avatar, then calling execCommand with a ProcessProtocol to attach and the command line. """ self.assertFalse(self.session.requestReceived('exec', common.NS('false'))) errors = self.flushLoggedErrors()

errors = self.flushLoggedErrors(RuntimeError)

self.assertEquals(len(errors), 1) errors[0].trap(RuntimeError) self.assertIdentical(self.session.client, None) self.assertTrue(self.session.requestReceived('exec', common.NS('true'))) self.assertIsInstance(self.session.session, StubSessionForStubOldAvatar) self.assertIsInstance(self.session.client, session.SSHSessionProcessProtocol) self.assertIdentical(self.session.session.execProtocol, self.session.client) self.assertEquals(self.session.session.execCommandLine, 'true')

Uhhh, that doesn't *really* execute 'true' on the command line, does it?

If it does, please replace it with something that will work on all platforms (e.g. 'python -c pass'). If it doesn't please replace it with something obviously bogus.

def test_requestExecWithData(self): """ When a client executes a command, it should be able to give pass data back and forth. Setting the remoteWindowLeft to 4 tests that the C{SSHChannel} buf instance variable isn't overriden by SSHSession. """

Why is 4 special? What is it about setting remoteWindowLeft that affects SSHChannel.buf?

self.session.remoteWindowLeft = 4 self.assertTrue(self.session.requestReceived('exec', common.NS('echo hello'))) self.assertIsInstance(self.session.session, StubSessionForStubOldAvatar) self.session.dataReceived('some data') self.assertEquals(self.session.session.execTransport.data, 'some data') self.session.addWindowBytes(20)

Why 20?

You know, it's entirely OK to do something like:

arbitraryInteger = 20 self.session.addWindowBytes(arbitraryInteger)

self.assertEquals(self.session.conn.data[self.session], ['hell', 'osome data\r\n']) self.session.eofReceived() self.session.closeReceived() self.session.closed() self.assertTrue(self.session.session.execTransport.closed) self.assertEquals(self.session.conn.requests[self.session], [('exit-status', '\x00\x00\x00\x00', False)]) def test_requestPty(self): """ When a client requests a PTY, the SSHSession object should make the request by getting an ISession adapter for the avatar, then calling getPty with the terminal type, the window size, and any modes the client gave us. """ self.assertFalse( self.session.requestReceived('pty_req', session.packRequest_pty_req('bad', (1, 2, 3, 4), []))) self.assertIsInstance(self.session.session, StubSessionForStubOldAvatar) errors = self.flushLoggedErrors()

errors = self.flushLoggedErrors(RuntimeError)

self.assertEquals(len(errors), 1) errors[0].trap(RuntimeError)

This is begging for a custom assertion. It's perfectly OK to add one here, and then later we can move it into trial.

self.assertTrue(self.session.requestReceived('pty_req', session.packRequest_pty_req('good', (1, 2, 3, 4), []))) self.assertEquals(self.session.session.ptyRequest, ('good', (1, 2, 3, 4), []))

I can't remember, do 'good' and 'bad' activate specific behaviours on the stub objects? If they do, add a comment saying so.

def test_requestWindowChange(self): """ When the client requests to change the window size, the SSHSession object should make the request by getting an ISession adapter for the avatar, then calling windowChanged with the new window size. """ self.assertFalse(self.session.requestReceived('window_change', session.packRequest_window_change((0, 0, 0, 0)))) errors = self.flushLoggedErrors() self.assertEquals(len(errors), 1) errors[0].trap(RuntimeError) self.assertIsInstance(self.session.session, StubSessionForStubOldAvatar)

You do this a lot. I'm guessing it's because requestReceived should set session. Why not test that in one place and then remove the assertion from all your other tests?

self.assertTrue(self.session.requestReceived('window_change', session.packRequest_window_change((1, 2, 3, 4)))) self.assertEquals(self.session.session.windowChange, (1, 2, 3, 4)) def test_eofReceived(self): """ When an EOF is received and a ISession adapter is present, it should be notified of the EOF message. """ self.session.session = session.ISession(self.session.avatar) self.session.eofReceived() self.assertTrue(self.session.session.gotEOF) def test_closeReceived(self): """ When a close is received, the session should send a close message. """ self.assertIdentical(self.session.closeReceived(), None) self.assertTrue(self.session.conn.closes[self.session]) def test_closed(self): """ When a close is received and a ISession adapter is present, it should be notified of the close message. """ self.session.session = session.ISession(self.session.avatar) self.session.closed() self.assertTrue(self.session.session.gotClosed) class TestHelpers(unittest.TestCase): """ Tests for the 4 helper functions: parseRequest_* and packRequest_*. """ def test_parseRequest_pty_req(self): """ The payload of a pty-req message is:: string terminal uint32 columns uint32 rows uint32 x pixels uint32 y pixels string modes Modes are:: byte mode number uint32 mode value """ self.assertEquals(session.parseRequest_pty_req(common.NS('xterm') + '\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00\x00\x03\x00\x00\x00' '\x04\x00\x00\x00\x05\x05\x00\x00\x00\x06'), ('xterm', (2, 1, 3, 4), [(5, 6)]))

If you are happy to use NS to convert 'xterm', you should be OK with using a similar helper to convert the Python integers into their wire format. Please do this.

Alternatively, format the byte string differently:

request = (common.NS('xterm') +

'\x00\x00\x00\x01' + '\x00\x00\x00\x02' + '\x00\x00\x00\x03' + '\x00\x00\x00\x04' + '\x00\x00\x00\x05' + '\x00\x00\x00\x06')

self.assertEquals(session.parseRequest_pty_req(request),

('xterm', (2, 1, 3, 4), [(5, 6)]))

def test_packRequest_pty_req(self): """ See test_parseRequest_pty_req for the payload format. """ self.assertEquals(self.assertWarns(DeprecationWarning, "packRequest_pty_req should be packing the modes.", unittest.file, session.packRequest_pty_req, 'xterm', (2, 1, 3, 4), '\x05\x00\x00\x00\x06'), common.NS('xterm') + '\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00' '\x00\x03\x00\x00\x00\x04\x00\x00\x00\x05\x05\x00\x00\x00\x06')

There's no reason to nest these calls so much. Local variables are as cheap as chips.

Use TestCase.callDeprecated instead of assertWarns.

def test_parseRequest_window_change(self): """ The payload of a window_change request is:: uint32 columns uint32 rows uint32 x pixels uint32 y pixels """ self.assertEquals(session.parseRequest_window_change('\x00\x00\x00\x01' '\x00\x00\x00\x02\x00\x00\x00\x03\x00\x00\x00\x04'), (2, 1, 3, 4))

Same comments as for the first test.

def test_packRequest_window_change(self): """ See test_parseRequest_window_change for the payload format. """ self.assertEquals(session.packRequest_window_change((2, 1, 3, 4)), '\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00\x00\x03\x00\x00\x00\x04')

Same comments as for the first test.

class SSHSessionProcessProtocolTestCase(unittest.TestCase): """ SSHSessionProcessProtocol is an obsolete class used as a ProcessProtocol for executed programs. It has has a couple transport methods, namely write() and loseConnection() """

'couple of transport methods'.

def setUp(self): self.session = session.SSHSession(conn=StubConnection(), remoteWindow=500, remoteMaxPacket=100) self.transport = StubTransport() self.pp = session.SSHSessionProcessProtocol(self.session) self.pp.makeConnection(self.transport) def test_init(self): """ SSHSessionProcessProtocol should set self.session to the session passed to the init method. """ self.assertEquals(self.pp.session, self.session) def test_getSignalName(self): """ _getSignalName should return the name of a signal when given the signal number. Also, it should only generate the signal dictionary once. """ signal.SIGTwistedTest = signal.NSIG + 1 # value can't exist normally # Force reinitialization of signals self.pp.class._signalNames = None try: for signame in session.supportedSignals: signame = 'SIG' + signame value = getattr(signal, signame) self.assertEquals(self.pp._getSignalName(value), signame, "%i: %s != %s" % (value, self.pp._getSignalName(value), signame)) self.assertEquals(self.pp._getSignalName(signal.SIGTwistedTest), 'SIGTwistedTest@' + sys.platform) newPP = session.SSHSessionProcessProtocol(self.session) self.assertNotIdentical(newPP._signalNames, None) self.assertIdentical(newPP._signalNames, self.pp._signalNames)

This tests three things, and should be three tests.

finally: del signal.SIGTwistedTest # Force reinitialization of signals self.pp.class._signalNames = None

See, that just goes to show that storing it on the class is a bad idea.

if getattr(signal, 'SIGALRM', None) is None: test_getSignalName.skip = "Not all signals available" def test_outReceived(self): """ When data is passed to the outReceived method, it should be sent to the session's write method. """ self.pp.outReceived('test data') self.assertEquals(self.session.conn.data[self.session], test data?) def test_write(self): """ When data is passed to the write method, it should be sent to the session channel's write method. """ self.pp.write('test data') self.assertEquals(self.session.conn.data[self.session], test data?) def test_errReceived(self): """ When data is passed to the errReceived method, it should be sent to the session's writeExtended method. """ self.pp.errReceived('test data') self.assertEquals(self.session.conn.extData[self.session], [(1, 'test data')]) def test_inConnectionLost(self): """ When inConnectionLost is called, it should send an EOF message, """ self.pp.inConnectionLost() self.assertTrue(self.session.conn.eofs[self.session]) def test_outConnectionLost(self): """ When outConnectionLost is called and there is no ISession adapter, the connection should be closed, """ self.pp.outConnectionLost() self.assertTrue(self.session.conn.closes[self.session]) def test_loseConnection(self): """ When loseConnection() is called, it should call loseConnection on the session channel. """ self.pp.loseConnection() self.assertTrue(self.session.conn.closes[self.session]) def test_processEndedWithExitCode(self): """ When processEnded is called, if there is an exit code in the reason it should be sent in an exit-status method. The connection should be closed. """ self.pp.processEnded(failure.Failure(error.ProcessDone(None))) self.assertEquals(self.session.conn.requests[self.session], [('exit-status', '\x00' * 4, False)])

Explain where the magic number comes from.

self.assertTrue(self.session.conn.closes[self.session]) def test_processEndedWithExitSignalCoreDump(self): """ When processEnded is called, if there is an exit signal in the reason it should be sent in an exit-signal message. The connection should be closed. """ self.pp.processEnded(failure.Failure(error.ProcessTerminated(None, signal.SIGTERM, 128))) self.assertEqual(self.session.conn.requests[self.session], [('exit-signal', common.NS('TERM') + '\x01' + common.NS() + common.NS(), False)])

Two magic numbers here. Please explain where they come from.

(Hint: coreDumped = '\x01' would go a long way)

self.assertTrue(self.session.conn.closes[self.session]) def test_processEndedWithExitSignalNoCoreDump(self): """ When processEnded is called, if there is an exit signal in the reason it should be sent in an exit-signal message. If no core was dumped, don't set the core-dump bit. """ self.pp.processEnded( failure.Failure(error.ProcessTerminated(None, signal.SIGTERM, 0))) self.assertEqual(self.session.conn.requests[self.session], [('exit-signal', common.NS('TERM') + '\x00' + common.NS() + common.NS(), False)]) self.assertTrue(self.session.conn.closes[self.session])

Magic number again.

if not hasattr(os, 'WCOREDUMP'): skipMsg = "can't run this w/o os.WCOREDUMP" test_processEndedWithExitSignalCoreDump.skip = skipMsg test_processEndedWithExitSignalNoCoreDump.skip = skipMsg class SSHSessionProcessProtocolApplicationTestCase(unittest.TestCase): """ SSHSessionProcessProtocolApplicationTestCase is an class used to connect to a SSHSessionProcessProtocol as an application. """ def test_dataReceived(self): """ When data is received, it should be sent to the transport. """ client = StubClient() app = session.SSHSessionProcessProtocolApplication(client) app.dataReceived('test data') self.assertEquals(client.transport.buf, 'test data')

This is a beautiful test.

def test_extendedDataReceived(self): """ When extended data of the type EXTENDED_DATA_STDERR is received, it should be passed along to SSHSessionProcessProtocol's transport.writeErr. """ transport = StubTransport() pp = MockProcessProtocol() pp.makeConnection(transport) app = session.SSHSessionProcessProtocolApplication(pp) app.extendedDataReceived(255, "ignore this")

What does 255 mean?

What does this line test?

app.extendedDataReceived(connection.EXTENDED_DATA_STDERR, "data") self.assertEquals(transport.err, "data") # should not raise an error if writeErr doesn't exist app.extendedDataReceived(connection.EXTENDED_DATA_STDERR, "more")

Huh what? You haven't done anything to writeErr. And if you did, it should go into another test.

Also, "should not raise an error" is a bad thing to test for. It's much easier and clearer to test for what a thing should do, rather than what it should not do.

class SSHSessionClientTestCase(unittest.TestCase): """ SSHSessionClient is an obsolete class used to connect standard IO to an SSHSession. """ def test_dataReceived(self): """ When data is received, it should be sent to the transport. """ client = session.SSHSessionClient() client.transport = StubTransport() client.dataReceived('test data') self.assertEquals(client.transport.buf, 'test data') class ServerSessionTestCase(unittest.TestCase): """ Tests that verify server functionality of SSHSession. """ def setUp(self): self.conn = StubConnection() self.avatar = StubNewAvatar() self.session = session.SSHSession(remoteWindow=131072, remoteMaxPacket=32768, conn=self.conn, avatar=self.avatar) def test_init(self): """ Test that the session is initialized correctly, """

Correctly is a weasel word in tests. Of course you are testing that something happens correctly. Tell us what 'correctly' means.

self.assertEquals(self.session.client, None) self.assertTrue(session.ISessionApplicationFactory.providedBy( self.session.applicationFactory), str(self.session.applicationFactory)) def test_client(self): """ Test that the session performs correctly when it has a client application. """

This docstring is unclear. "correctly", as mentioned before, adds no meaning.

Looking at the test below, it would be easier to write a docstring if you were testing fewer things.

client = MockApplication() self.session.dataReceived('1') self.session.extReceived(connection.EXTENDED_DATA_STDERR, '2') self.session.extReceived(2, '3') self.session.setupSession(client) self.session.dataReceived('out') self.assertEquals(client.data, ['1', 'out']) self.assertEquals(self.session.conn.data[self.session], ['1~', 'out~']) self.session.extReceived(connection.EXTENDED_DATA_STDERR, 'err') self.assertEquals(client.extendedData, [(connection.EXTENDED_DATA_STDERR, '2'), (2, '3'), (connection.EXTENDED_DATA_STDERR, 'err')]) self.assertEquals(self.session.conn.extData[self.session], [(connection.EXTENDED_DATA_STDERR + 1, '2~'), (2 + 1, '3~'), (connection.EXTENDED_DATA_STDERR + 1, 'err~')]) self.session.eofReceived() self.assertTrue(client.gotEOF) self.assertFalse(client.hasClosed, 'closed() called during EOF') self.session.closeReceived() self.assertTrue(client.gotClose) self.assertFalse(client.hasClosed, 'closed() called during closeReceived') self.session.closed() self.assertTrue(client.hasClosed, 'closed() not called')

This test tests so many things. Please split it up as much as possible.

def test_applicationFactory(self): """ Test that SSHSession handles dispatching to applicationFactory correctly. """ self.session.eofReceived() self.assertFalse(self.session.applicationFactory.inConnectionOpen) self.session.closeReceived() self.assertFalse(self.session.applicationFactory.outConnectionOpen) self.session.closed() self.assertTrue(self.session.applicationFactory.ended)

What does correctly mean?

I think this test would be clearer if you used the more traditional 'call logging' style of mock object, rather than one with many custom booleans.

def test_subsystem(self): """ Test that SSHSession handles subsystem requests by dispatching to the application factory's requestSubsytem() method and connecting the ISessionApplication to itself. """

'itself' is ambiguous here. I *think* you mean "the session", but you could mean "the session application".

ret = self.session.requestReceived('subsystem', common.NS('bad')) self.assertFalse(ret) self.assertEquals(self.session.client, None) ret = self.session.requestReceived('subsystem', common.NS('echo') + 'abc')

The 'echo' subsystem was defined some few hundred lines back. It's worth reminding the reader that it's there.

self.assertTrue(ret) self.assertTrue(session.ISessionApplication.providedBy( self.session.sessionApplication))

I really dislike the way this combines an assertion with code that exercises the system-under-test. It makes it harder to understand the expected behaviour. You do it a lot in this file, and I'd really appreciate it if you could change the tests to not do it.

self.assertIdentical(self.session.sessionApplication, self.session.applicationFactory.subsystem) self.assertFalse(self.session.sessionApplication.hasClosed) self.assertEquals(self.session.sessionApplication.packetData, 'abc') self.assertEquals(self.session.sessionApplication.channel, self.session) def test_shell(self): """ Test that SSHSession handles shell requests by dispatching to the application factory's openShell() method and connecting the ISessionApplication to itself. """

Same comments on 'itself'.

self.assertTrue(self.session.requestReceived('shell', )) self.assertNotEquals(self.session.sessionApplication, None) self.assertIdentical(self.session.sessionApplication, self.session.applicationFactory.shell) self.assertEquals(self.session.sessionApplication.channel, self.session) self.assertFalse(self.session.sessionApplication.hasClosed) self.assertFalse(self.session.requestReceived('shell', )) # fail if we have a shell already errors = self.flushLoggedErrors()

errors = self.flushLoggedErrors(RuntimeError)

self.assertEquals(len(errors), 1) errors[0].trap(RuntimeError) def test_exec(self): """ Test that SSHSession handles command requests by dispatching to the the application factory's execCommand method and connecting the ISessionApplication to itself. """

Ditto 'itself'.

self.assertTrue(self.session.requestReceived('exec', common.NS('good'))) self.assertNotEquals(self.session.sessionApplication, None) self.assertIdentical(self.session.sessionApplication, self.session.applicationFactory.command) self.assertEquals(self.session.sessionApplication.channel, self.session) self.assertEquals(self.session.sessionApplication.command, 'good') self.assertFalse(self.session.sessionApplication.hasClosed) self.assertFalse(self.session.requestReceived('exec', common.NS('bad'))) errors = self.flushLoggedErrors()

errors = self.flushLoggedErrors(RuntimeError)

self.assertEquals(len(errors), 1) errors[0].trap(RuntimeError) def test_ptyRequest(self): """ Test that SSHSession handles PTY requests by dispatching to the application factory's getPTY method. """ term = 'conch' windowSize = (80, 25, 0, 0) modes = [(0, 1), (2, 3)] ret = self.session.requestReceived('pty_req', common.NS(term) + '\x00\x00\x00\x19' + '\x00\x00\x00\x50' + '\x00\x00\x00\x00' * 2 + common.NS('\x00\x00\x00\x00\x01' '\x02\x00\x00\x00\x03'))

Formatting this with many more line breaks would make it a lot clearer.

Also, if you really cared about the reader, you'd make it obvious that '25' is inextricably bound up with '\x00\x00\x00\x19'.

self.assertTrue(ret) self.assertEquals(self.session.applicationFactory.term, term) self.assertEquals(self.session.applicationFactory.windowSize, windowSize)

Line too long, please wrap.

self.assertEquals(self.session.applicationFactory.modes, modes) self.assertFalse(self.session.requestReceived('pty_req', )) errors = self.flushLoggedErrors()

errors = self.flushLoggedErrors(RuntimeError)

self.assertEquals(len(errors), 1) errors[0].trap(struct.error) def test_windowChange(self): """ Test that SSHSession handles window size change requests by dispatching to the application factory's windowChanged method. """ windowSize = (1, 1, 1, 1) ret = self.session.requestReceived('window_change', '\x00\x00\x00\x01' * 4) self.assertTrue(ret) self.assertEquals(self.session.applicationFactory.windowSize, windowSize) self.assertFalse(self.session.requestReceived('window_change', )) errors = self.flushLoggedErrors()

errors = self.flushLoggedErrors(RuntimeError)

self.assertEquals(len(errors), 1) errors[0].trap(struct.error) class ClientSessionTestCase(unittest.TestCase): """ Test methods that use SSHSession as a client. """ def setUp(self): self.conn = StubConnection() self.session = session.SSHSession(remoteWindow=131072, remoteMaxPacket=32768, conn=self.conn) def test_getPty(self): """ Test that getPty sends the correct request. """ d = self.session.getPty('term', (80, 25, 0, 0), [(0, 1), (2, 3)], True) def check(value): self.assertEquals(self.conn.requests[self.session], [('pty-req', common.NS('term') + '\x00\x00\x00\x19' + '\x00\x00\x00\x50' + '\x00\x00\x00\x00' * 2 + common.NS('\x00\x00\x00\x00\x01\x02\x00\x00\x00\x03'), True)])

Formatting this with many more line breaks would make it a lot clearer.

Also, if you really cared about the reader, you'd make it obvious that '25' is inextricably bound up with '\x00\x00\x00\x19'.

self.assertEquals(self.session.getPty('term', (80, 25, 0, 0), []), None) d.addCallback(check) return d def test_changeWindowSize(self): """ Test that windowChange sends the correct request. """ d = self.session.changeWindowSize((80, 25, 0, 0), True) def check(value): self.assertEquals(self.conn.requests[self.session], [('window-change', '\x00\x00\x00\x19\x00\x00\x00\x50' + '\x00\x00\x00\x00' * 2, True)])

Newlines and hex.

self.assertEquals(self.session.changeWindowSize((80, 25, 0, 0)), None) d.addCallback(check) return d def test_openSubsystem(self): """ Test that openSubsystem sends the correct request. """

What's correct? I don't know.

d = self.session.openSubsystem('echo', 'data', True) def check(value): self.assertEquals(self.conn.requests[self.session], [('subsystem', common.NS('echo') + 'data', True)]) self.assertEquals(self.session.openSubsystem('echo', 'data'), None) d.addCallback(check) return d def test_openShell(self): """ Test that openShell sends the correct request. """

What's correct? I don't know.

d = self.session.openShell(True) def check(value): self.assertEquals(self.conn.requests[self.session], [('shell', , True)]) self.assertEquals(self.session.openShell(), None) d.addCallback(check) return d def test_execCommand(self): """ Test that execCommand sentds the correct request. """

What's correct? I don't know.

Also, 'sends' not 'sentds'.

d = self.session.execCommand('true', True) def check(value): self.assertEquals(self.conn.requests[self.session], [('exec', common.NS('true'), True)]) self.assertEquals(self.session.execCommand('true'), None) d.addCallback(check) return d

OK, that was epic.

It'd be great if you could send a diff containing the changes that you made in response to this review.

Thanks again, jml

[jml]

Ugh, sorry, I should have previewed. One moment.

[jml]

Hi Thomas & Paul,

This is a review of branches/session-2710-5.

This diff is over 3000 lines long. Although sometimes work can't be broken up, you should make an effort. The time it takes to review a branch scales non-linearly with the size of the diff.

Before you get into the review, I want to thank both of you for taking the time to do this work. Conch more than any other part of Twisted will benefit from better tests and being easier to test.

There's going to be another review round-trip and there *are* bits of the diff that I have serious issues with. Still, the patch as a whole shows clear thinking and a strong desire to separate concerns that were previously mixed together. You're attention to backwards compatibility is also greatly appreciated.

Because I'm coming to this branch cold, I'm going to be doing a full review. Personally, I hate it when an old branch of mine is subject to a fresh review by a fresh reviewer -- it means a lot of decisions get questioned again and feels like a lot of work little or no reason. It's also likely that the new reviewer will contradict the old reviewer.

I'm going to try as hard as I can to focus only on code cleanliness, and to assume that you and the previous reviewers knew what they were doing. What that means is I'll flag everything that's wrong and everything that I don't get, and I'll try to distinguish between the two.

I'd very much appreciate it if you could respond to each of my points, even if it is to say "no, I don't think I'll do that". The priority now is getting this branch landed. If you reply point-wise, my re-review can probably be done trivially.

Also, feel free to grab me on IRC or on skype ('blackjml') if you want to discuss any of these points.

> Index: twisted/conch/test/test_filetransfer.py
> ===================================================================
> --- twisted/conch/test/test_filetransfer.py	(revision 23210)
> +++ twisted/conch/test/test_filetransfer.py	(working copy)
> @@ -56,11 +56,6 @@
>          return os.path.join(os.getcwd(), self.homeDir)
>  
>  
> -class ConchSessionForTestAvatar:
> -
> -    def __init__(self, avatar):
> -        self.avatar = avatar
> -
>  if unix:
>      if not hasattr(unix, 'SFTPServerForUnixConchUser'):
>          # unix should either be a fully working module, or None.  I'm not sure
> @@ -463,7 +458,7 @@
>          self.interceptConnectionLost(sftpServer)
>  
>          # close session
> -        testSession.closeReceived()
> +        testSession.closed()
>

XXX I don't know what this is for. Let's see if I find out.

>          self.assertSFTPConnectionLost()
>  
> Index: twisted/conch/scripts/conch.py
> ===================================================================
> --- twisted/conch/scripts/conch.py	(revision 23210)
> +++ twisted/conch/scripts/conch.py	(working copy)
> @@ -8,20 +8,21 @@
>  
>  #""" Implementation module for the `conch` command.
>  #"""

Please either delete this comment, or uncomment the docstring and make it comply with Twisted conventions.

> -from twisted.conch.client import agent, connect, default, options
> +from twisted.conch.client import connect, default, options
>  from twisted.conch.error import ConchError
>  from twisted.conch.ssh import connection, common
>  from twisted.conch.ssh import session, forwarding, channel
> -from twisted.internet import reactor, stdio, defer, task
> +from twisted.internet import reactor, stdio, task
>  from twisted.python import log, usage
>  
> -import os, sys, getpass, struct, tty, fcntl, base64, signal, stat, errno
> +import os, sys, getpass, struct, tty, fcntl, signal, errno
>  
> +
>  class ClientOptions(options.ConchOptions):
> -    
> +
>      synopsis = """Usage:   conch [options] host [command]
>  """
> -    
> +
>      optParameters = [['escape', 'e', '~'],
>                        ['localforward', 'L', None, 'listen-port:host:port   Forward local port to remote address'],
>                        ['remoteforward', 'R', None, 'listen-port:host:port
>      Forward remote port to local address'],

Please wrap these lines to fit in 79 columns.

> Index: twisted/conch/ssh/session.py
> ===================================================================
> --- twisted/conch/ssh/session.py	(revision 23210)
> +++ twisted/conch/ssh/session.py	(working copy)
> @@ -1,9 +1,7 @@
> -# -*- test-case-name: twisted.conch.test.test_conch -*-
> -# Copyright (c) 2001-2004 Twisted Matrix Laboratories.
> +# -*- test-case-name: twisted.conch.test.test_session -*-
> +# Copyright (c) 2001-2007 Twisted Matrix Laboratories.

It's 2008 now.

>  # See LICENSE for details.
>  
> -# 
> -
>  """
>  This module contains the implementation of SSHSession, which (by default)
>  allows access to a shell and a python interpreter over SSH.
> @@ -11,214 +9,890 @@
>  Maintainer: U{Paul Swartz<mailto:z3p@twistedmatrix.com>}
>  """
>  
> -import struct
> +import struct, warnings, signal, os, sys
>  
> +from zope.interface import implements
> +
>  from twisted.internet import protocol
> -from twisted.python import log
> -from twisted.conch.interfaces import ISession
> -from twisted.conch.ssh import common, channel
> +from twisted.python import components, log
> +from twisted.conch.interfaces import ISessionApplication
> +from twisted.conch.interfaces import ISessionApplicationFactory, ISession
> +from twisted.conch.ssh import common, channel, connection
>  
> +
> +# supportedSignals is a list of signals that every session channel is supposed
> +# to support.
> +supportedSignals = ["ABRT", "ALRM", "FPE", "HUP", "ILL", "INT", "KILL", "PIPE",
> +                    "QUIT", "SEGV", "TERM", "USR1", "USR2"]
> +
> +
> +
>  class SSHSession(channel.SSHChannel):
> +    """
> +    A channel implementing the server side of the 'session' channel.  This is
> +    the channel that a client requests when it wants a subsystem, shell, or
> +    command.
>  
> +    @ivar earlyData: data sent to this channel before a client is present.
> +    @type earlyData: C{str}
> +    @ivar earlyExtended: extended data sent to this channel before a client is
> +    present.
> +    @type earlyExtended: C{list}
> +    @ivar applicationFactory: an object to which requests for shells and such
> +        are dispatched.  It implements I{ISessionApplicationFactory}.
> +    @ivar sessionApplication: an object which handles interacting with the
> +        other side of the channel.  It implements I{ISessionApplication}.
> +    @ivar client: a C{ProcessProtocol} to which data sent to this channel is
> +        sent.  This variable is deprecated.
> +    @type client: L{protocol.ProcessProtocol}
> +    @ivar session: an object implementing L{ISession} to which requests for
> +        shells or commands are dispatched.  This variable is deprecated.
> +    """
> +
> +
> +    # set so that this object can be used as a client channel.
>      name = 'session'
> +
> +
> +    def __setattr__(self, k, v):
> +        """
> +        Trap the 'client' attribute, what used to be the old name (roughly) for
> +        'sessionApplication', and 'session', which triggers setting up
> +        _DeprecatedSSHSession as our application factory.
> +        """
> +        if k == 'client':
> +            # Someone is trying to inform us of an old-style client.  Clear the
> +            # buffers (because this would not have previously delivered any
> +            # data, only delivered subsequent data) and set the old-style
> +            # "client" object up as a new-style processProtocol.
> +            self.earlyData = ''
> +            self.earlyExtended = []
> +            self.setupSession(_TransportToProcessProtocol(v.transport))
> +        if k == 'session' and v is not None:
> +            # Someone is trying to inform us of an old-style session.  Wrap it
> +            # in a _DeprecatedSSHSession.
> +            self.applicationFactory = _DeprecatedSSHSession(self, v)
> +        self.__dict__[k] = v
> +

I saw the 'setattr' and had a moment of quiet terror. Now I see that it's there for backwards compatibility -- this code will make my life easier, so thanks :)

However, I think that you could and should implement this with properties.

> +
> +    def applicationFactory(self):
> +        """
> +        Produce an applicationFactory dynamically if one does not yet exist.
> +        """
> +        if self.avatar is not None:
> +            factoryCandidate = ISessionApplicationFactory(self.avatar, None)
> +            if factoryCandidate is None:
> +                # Maybe it implements the old version.
> +                oldStyle = ISession(self.avatar, None)
> +                if oldStyle is not None:
> +                    # See __setattr__ above.
> +                    self.session = oldStyle
> +                else:
> +                    # Maybe it doesn't implement either.  The test
> +                    # SFTP server doesn't implement a session, because
> +                    # subsystems were just looked up in the avatar.
> +                    # Use a _SubsystemOnlyApplicationFactory.
> +                    self.applicationFactory = _SubsystemOnlyApplicationFactory(
> +                        self)
> +            else:
> +                self.applicationFactory = factoryCandidate
> +            return self.applicationFactory
> +    applicationFactory = property(applicationFactory)
> +

This method doesn't do what it says it does. The check at the beginning is for whether 'self.avatar' is None. Calling this method multiple times will create an applicationFactory multiple times. Is this deliberate? If so, please update the docstring. If not, then I *think* you need to change the guard at the top of the function.

Also, it's not clear to me that applicationFactory should be None if no avatar is set. The docstring should state that this is the case and explain why.

> +
> +    def client(self):
> +        """
> +        This used to be the C{ProcessProtocol} that was connected to us.
> +        Since we're not connected to a C{ProcessProtocol} anymore, we
> +        fake it.
> +        """
> +        if isinstance(self.sessionApplication,
> +                      _ProcessProtocolToSessionApplication):
> +            return self.sessionApplication.processProtocol
> +        elif isinstance(self.sessionApplication,
> +                        SSHSessionProcessProtocolApplication):
> +            return self.sessionApplication.processProtocol
> +    client = property(client)
> +

You should extract the if k == 'client' bit out of setattr and make it the setter for this property.

Also, I think this should raise a DeprecationWarning.

> +
> +    # This used to be set in the older SSHSession implementation.
> +    session = None
> +

And this needs to have a getter. I'm sorry, but there's code out there that uses this.

> +
>      def __init__(self, *args, **kw):
>          channel.SSHChannel.__init__(self, *args, **kw)
> -        self.buf = ''
> -        self.client = None
> -        self.session = None
> +        self.earlyData = ''
> +        self.earlyExtended = []
> +        self.sessionApplication = None
>  
> +
> +    def setupSession(self, sessionApplication):
> +        """
> +        Connect us to the application.  We set ourselves as it's channel
> +        instance variable, and the application becomes our sessionApplication.
> +        If any data was sent early, send it now.
> +        """

Is this supposed to be public? If not, make it _setUpSession.

Is there already an API or compatibility constraint on the name? If not, make it setUpSession.

> +        # make sure we have an ISessionApplication
> +        sessionApplication = ISessionApplication(sessionApplication)
> +        sessionApplication.channel = self
> +        sessionApplication.connectionMade()
> +        # Maybe change this name too...

Please say why you think this name should maybe change.

> +        self.sessionApplication = sessionApplication
> +        if self.earlyData:
> +            b, self.earlyData = self.earlyData, None
> +            self.dataReceived(b)
> +        if self.earlyExtended:
> +            b, self.earlyExtended = self.earlyExtended, None
> +            for dataType, data in b:
> +                self.extReceived(dataType, data)
> +

I guess 'b' stands for 'bytes'. Could you please rename it?

> +
>      def request_subsystem(self, data):
> -        subsystem, ignored= common.getNS(data)
> +        """
> +        The remote side has requested a subsystem.  Payload::
> +            string  subsystem name
> +
> +        Try to get a subsystem object by calling our adapter's lookupSubsystem
> +        method.  If that method returns a subsystem, then connect to our
> +        client and return True.  Otherwise, return False.
> +        """

I'm surprised you don't return the subsystem. Why don't you?

> +        subsystem, rest = common.getNS(data)
>          log.msg('asking for subsystem "%s"' % subsystem)
> -        client = self.avatar.lookupSubsystem(subsystem, data)
> +        client = self.applicationFactory.lookupSubsystem(subsystem, rest)
>          if client:
> -            pp = SSHSessionProcessProtocol(self)
> -            proto = wrapProcessProtocol(pp)
> -            client.makeConnection(proto)
> -            pp.makeConnection(wrapProtocol(client))
> -            self.client = pp
> -            return 1
> +            self.setupSession(client)
> +            return True
>          else:
> -            log.msg('failed to get subsystem')
> -            return 0
> +            return False
>  
> +
>      def request_shell(self, data):
> +        """
> +        The remote side has requested a shell.  No payload.  Call the
> +        application factory's openShell() method; it returns something
> +        implementing I{ISessionApplication} that will become our
> +        application.  If there's no exception, return True.
> +        Otherwise, return False.
> +        """
>          log.msg('getting shell')
> -        if not self.session:
> -            self.session = ISession(self.avatar)
>          try:
> -            pp = SSHSessionProcessProtocol(self)
> -            self.session.openShell(pp)
> -        except:
> -            log.deferr()
> -            return 0
> +            self.setupSession(self.applicationFactory.openShell())
> +        except Exception:

This looks wrong. Why are you catching all exceptions here and not in other 'request_' methods?

Also, a bare except is the way to go, as long as you catch and reraise KeyboardInterrupt.

> +            log.err()
> +            return False
>          else:
> -            self.client = pp
> -            return 1
> +            return True
>  
> +
>      def request_exec(self, data):
> -        if not self.session:
> -            self.session = ISession(self.avatar)
> -        f,data = common.getNS(data)
> -        log.msg('executing command "%s"' % f)
> +        """
> +        The remote side has requested to execute a command.  Payload::
> +            string  command line
> +
> +        Call the application factory's execCommand method with the
> +        command line.  It should return something implementing
> +        I{ISessionApplication} that becomes our client.  If there's no
> +        exception, return True.  Otherwise, return False.
> +        """
> +        command, data = common.getNS(data)
> +        log.msg('executing command "%s"' % command)
>          try:
> -            pp = SSHSessionProcessProtocol(self)
> -            self.session.execCommand(pp, f)
> -        except:
> -            log.deferr()
> -            return 0
> +            self.setupSession(self.applicationFactory.execCommand(command))
> +        except Exception:
> +            log.err()
> +            return False

Same comment as before.

>          else:
> -            self.client = pp
> -            return 1
> +            return True
>  
> +
>      def request_pty_req(self, data):
> -        if not self.session:
> -            self.session = ISession(self.avatar)
> -        term, windowSize, modes = parseRequest_pty_req(data)
> -        log.msg('pty request: %s %s' % (term, windowSize))
> +        """
> +        The remote side has requested a psuedoterminal (PTY).  Payload::
> +            string  terminal name
> +            uint32  columns
> +            uint32  rows
> +            uint32  xpixels
> +            uint32  ypixels
> +            string  modes
> +
> +        Modes is::
> +            0 or more of::
> +                byte    mode number
> +                uint32  mode value
> +
> +        Call the application factory's getPTY method.  If no exception
> +        is raised, return True.  Otherwise, return False.
> +        """

Thanks for including these docstrings, btw. They are a real help.

>          try:
> -            self.session.getPty(term, windowSize, modes)
> -        except:
> +            term, windowSize, modes = parseRequest_pty_req(data)
> +            log.msg('pty request: %s %s' % (term, windowSize))
> +            self.applicationFactory.getPTY(term, windowSize, modes)
> +        except Exception:
>              log.err()
> -            return 0
> +            return False
>          else:
> -            return 1
> +            return True
>  
> +
>      def request_window_change(self, data):
> -        if not self.session:
> -            self.session = ISession(self.avatar)
> -        winSize = parseRequest_window_change(data)
> +        """
> +        The remote side has changed the window size.  Payload::
> +            uint32  columns
> +            uint32  rows
> +            uint32  xpixels
> +            uint32  ypixels
> +
> +
> +        Call the application factory's windowChanged method.  If no
> +        exception is raised, return True.  Otherwise, return False.
> +        """
>          try:
> -            self.session.windowChanged(winSize)
> -        except:
> +            winSize = parseRequest_window_change(data)
> +            self.applicationFactory.windowChanged(winSize)
> +        except Exception:
>              log.msg('error changing window size')
>              log.err()
> -            return 0
> +            return False
>          else:
> -            return 1
> +            return True
>

OK, I'm beginning to suspect you just missed out the except clause in request_subsytem. Please make sure you add it and switch to bare excepts.

> +
>      def dataReceived(self, data):
> -        if not self.client:
> -            #self.conn.sendClose(self)
> -            self.buf += data
> +        """
> +        We got data from the remote side.  If we have an application,
> +        send the data to it.  Otherwise, buffer the data.
> +
> +        @type data: C{str}
> +        """
> +        if self.sessionApplication is None:
> +            self.earlyData += data

Perhaps StringIO would be more appropriate than a string for earlyData. If you think so, whack an XXX comment somewhere in this file saying so -- this branch is already too big. Be sure to put your name and the date on the comment.

>              return
> -        self.client.transport.write(data)
> +        self.sessionApplication.dataReceived(data)
>  
> +
>      def extReceived(self, dataType, data):
> -        if dataType == connection.EXTENDED_DATA_STDERR:
> -            if self.client and hasattr(self.client.transport, 'writeErr'):
> -                self.client.transport.writeErr(data)
> +        """
> +        We got extended data from the remote side.  If we have an
> +        application, send the data to it.  Otherwise, buffer the data.
> +
> +        @type dataType: C{int}
> +        @type data: C{str}
> +        """

You know, I'd always wondered what this method was for. Thanks for adding the docstring :)

> +        if self.sessionApplication is not None:
> +            self.sessionApplication.extendedDataReceived(dataType, data)
>          else:
> -            log.msg('weird extended data: %s'%dataType)
> +            self.earlyExtended.append((dataType, data))
>  
> +
>      def eofReceived(self):
> -        if self.session:
> -            self.session.eofReceived()
> -        elif self.client:
> -            self.conn.sendClose(self)
> +        """
> +        The remote side has closed its write side.  If we have an
> +        application factory, notify it.  If we have an application,
> +        notify it.
> +        """
> +        if self.applicationFactory is not None:
> +            self.applicationFactory.eofReceived()
> +        if self.sessionApplication is not None:
> +            self.sessionApplication.eofReceived()
>  
> +
>      def closed(self):
> -        if self.session:
> -            self.session.closed()
> -        elif self.client:
> -            self.client.transport.loseConnection()
> +        """
> +        The channel is closed.  If we have an application factory,
> +        notify it.  If we have an application, tell it the connection
> +        is lost.
> +        """

It *might* be worth mentioning here that just because the channel is closed, that doesn't mean that the SSH connection to the client is closed.

Just a thought.

> +        if self.applicationFactory is not None:
> +            self.applicationFactory.closed()
> +        if self.sessionApplication is not None:
> +            self.sessionApplication.closed()
>  
> -    #def closeReceived(self):
> -    #    self.loseConnection() # don't know what to do with this
>  
> -    def loseConnection(self):
> -        if self.client:
> -            self.client.transport.loseConnection()
> -        channel.SSHChannel.loseConnection(self)
> +    def closeReceived(self):
> +        """
> +        The remote side has requested that we no longer send data.  If
> +        we have an application factory, notify it.  If we have an
> +        application, notify it.
> +        """
>

Good docstring.

> -class _ProtocolWrapper(protocol.ProcessProtocol):
> +        if self.applicationFactory is not None:
> +            self.applicationFactory.closeReceived()
> +        if self.sessionApplication is not None:
> +            self.sessionApplication.closeReceived()
> +
> +
> +    # client methods
> +

'client methods' is ambiguous. Please say what they are used for, why they are different to the previous methods and, if you think it's appropriate, why they are in this class at all.

> +
> +    def getPty(self, term, windowSize, modes, wantReply=False):
> +        """
> +        Request a PTY from the other side.
> +
> +        @param term: the type of terminal (e.g. xterm)
> +        @type term: C{str}
> +        @param windowSize: the size of the window: (rows, cols, xpixels,
> +                           ypixels)
> +        @type windowSize: C{tuple}
> +        @param modes: termanal modes; a list of (modeNumber, modeValue) pairs.

It's spelled 'terminal'. It might be worth putting in a 'see also', so people who want to know what modeNumber and modeValue mean can find out.

> +        @type modes: C{list}
> +        @param wantReply: True if we want a reply to this request.
> +        @type wantReply: C{bool}
> +
> +        @returns: if wantReply, a Deferred that will be called back when
> +                  the request has succeeded or failed; else, None.
> +        @rtype: C{Deferred}/C{None}
> +        """
> +        data = packRequest_pty_req(term, windowSize, modes)
> +        return self.conn.sendRequest(self, 'pty-req', data,
> +                wantReply=wantReply)
> +
> +
> +    def openSubsystem(self, subsystem, data='', wantReply=False):
> +        """
> +        Request that a subsystem be opened on the other side.
> +
> +        @param subsystem: the name of the subsystem
> +        @type subsystem: C{str}
> +        @param data: any extra data to send with the request
> +        @type data: C{str}
> +        @param wantReply: True if we want a reply to this request.
> +        @type wantReply: C{bool}
> +
> +        @returns: if wantReply, a Deferred that will be called back when
> +                  the request has succeeded or failed; else, None.
> +        @rtype: C{Deferred}/C{None}
> +        """
> +        return self.conn.sendRequest(self, 'subsystem', common.NS(subsystem) +
> +                data, wantReply=wantReply)
> +
> +
> +    def openShell(self, wantReply=False):
> +        """
> +        Request that a shell be opened on the other side.
> +
> +        @param wantReply: True if we want a reply to this request.
> +        @type wantReply: C{bool}
> +
> +        @returns: if wantReply, a Deferred that will be called back when
> +                  the request has succeeded or failed; else, None.
> +        @rtype: C{Deferred}/C{None}
> +        """
> +        return self.conn.sendRequest(self, 'shell', '', wantReply=wantReply)
> +
> +
> +    def execCommand(self, command, wantReply=False):
> +        """
> +        Request that a command be executed on the other side.
> +
> +        @param command: the command to execute
> +        @type command: C{str}
> +        @param wantReply: True if we want a reply to this request.
> +        @type wantReply: C{bool}
> +
> +        @returns: if wantReply, a Deferred that will be called back when
> +                  the request has succeeded or failed; else, None.
> +        @rtype: C{Deferred}/C{None}
> +        """
> +        return self.conn.sendRequest(self, 'exec', common.NS(command),
> +                wantReply=wantReply)
> +
> +
> +    def changeWindowSize(self, windowSize, wantReply=False):
> +        """
> +        Inform the other side that the local terminal size has changed.
> +
> +        @param windowSize: the new size of the window: (rows, cols, xpixels,
> +                           ypixels)
> +        @type windowSize: C{tuple}
> +        @param wantReply: True if we want a reply to this request.
> +        @type wantReply: C{bool}
> +
> +        @returns: if wantReply, a Deferred that will be called back when
> +                  the request has succeeded or fails; else, None.
> +        @rtype: C{Deferred}/C{None}
> +        """
> +        data = packRequest_window_change(windowSize)
> +        return self.conn.sendRequest(self, 'window-change', data,
> +                wantReply=wantReply)
> +

I'm guessing that this wantReply business is for backwards compat? It seems a little strange otherwise.

> +
> +
> +class _SubsystemOnlyApplicationFactory:
>      """
> +    An application factory which which is only good for looking up a
> +    subsystem.  It is used when there is not an ISession adapter
> +    defined for the avatar.  Its use is deprecated.
> +    """

If it's deprecated then please make sure it issues a deprecation warning when used. See twisted.python.deprecate.

> +    implements(ISessionApplicationFactory)
> +
> +
> +    def __init__(self, sessionChannel):
> +        self.sessionChannel = sessionChannel
> +
> +
> +    def lookupSubsystem(self, subsystem, data):
> +        """
> +        The previous ConchUser avatar had subsystems looked up through
> +        the avatar instead of through the ISession adapter.  To be backwards
> +        compatible, try to look up the subsystem through the avatar.
> +        """
> +        self.sessionChannel.earlyData = ''
> +        self.sessionChannel.earlyExtended = []
> +        client = self.sessionChannel.avatar.lookupSubsystem(subsystem,
> +                                              common.NS(subsystem) + data)
> +        return wrapProcessProtocol(client)
> +
> +
> +    def eofReceived(self):
> +        self.sessionChannel.loseConnection()
> +
> +
> +    def closeReceived(self):
> +        """
> +        The old closeReceived method did not exist, so we dispatch to our
> +        parent's method.
> +        """
> +        channel.SSHChannel.closeReceived(self.sessionChannel)
> +
> +
> +    def closed(self):
> +        """
> +        The old implementation of this method didn't do anything, so we
> +        don't do anything either.
> +        """
> +
> +
> +
> +class _DeprecatedSSHSession(_SubsystemOnlyApplicationFactory):
> +    """
> +    This class brings the deprecated functionality of the old SSHSession
> +    into a single place.
> +    """
> +
> +
> +    def __init__(self, sessionChannel, oldISessionProvider):
> +        _SubsystemOnlyApplicationFactory.__init__(self, sessionChannel)
> +        self.oldISessionProvider = oldISessionProvider
> +
> +
> +    def getPTY(self, term, windowSize, modes):
> +        """
> +        The name of this method used to be getPty.
> +        """
> +        return self.oldISessionProvider.getPty(term, windowSize, modes)
> +
> +
> +    def openShell(self):
> +        """
> +        The old openShell interface passed in a ProcessProtocol which would be
> +        connected to the shell.
> +        """
> +        pp = SSHSessionProcessProtocol(self.sessionChannel)
> +        self.oldISessionProvider.openShell(pp)
> +        return SSHSessionProcessProtocolApplication(pp)
> +
> +
> +    def execCommand(self, command):
> +        """
> +        The old execCommand interface passed in a ProcessProtocol which would
> +        be connected to the command.
> +        """
> +        pp = SSHSessionProcessProtocol(self.sessionChannel)
> +        self.oldISessionProvider.execCommand(pp, command)
> +        return SSHSessionProcessProtocolApplication(pp)
> +
> +
> +    def eofReceived(self):
> +        """
> +        The old eofReceived method closed the session connection.
> +        """
> +        self.oldISessionProvider.eofReceived()
> +        self.sessionChannel.conn.sendClose(self.sessionChannel)
> +
> +
> +    def closed(self):
> +        """
> +        The old closed method closed the client's transport.
> +        """
> +        if self.sessionChannel.sessionApplication is not None:
> +            self.sessionChannel.sessionApplication.processProtocol.transport.loseConnection()
> +        self.oldISessionProvider.closed()
> +
> +
> +    def windowChanged(self, newWindowSize):
> +        """
> +        Just pass this method through.
> +        """
> +        self.oldISessionProvider.windowChanged(newWindowSize)
> +
> +
> +
> +class _ProtocolToProcessProtocol(protocol.ProcessProtocol):
> +    """
>      This class wraps a L{Protocol} instance in a L{ProcessProtocol} instance.
> +
> +    @ivar proto: the C{Protocol} we're wrapping.
>      """

This belongs in twisted.internet, not twisted.conch. Please add a XXX comment saying so (with name and the date).

> +
> +
>      def __init__(self, proto):
>          self.proto = proto
>  
> -    def connectionMade(self): self.proto.connectionMade()
>  
> -    def outReceived(self, data): self.proto.dataReceived(data)
> +    def __getattr__(self, attr):
> +        """
> +        This class did not previously exist, so some uses expect this object
> +        to implement the Protocol interface.  To handle this case, we pass
> +        requests through to the wrapped object.
> +        """
> +        return getattr(self.proto, attr)
>

Is there a reason for not using proxyForInterfaces?

> -    def processEnded(self, reason): self.proto.connectionLost(reason)
>  
> -class _DummyTransport:
> +    def __setattr__(self, attr, value):
> +        """
> +        See the documentation for __getattr__.
> +        """
> +        if attr in ('proto', 'transport'):
> +            self.__dict__[attr] = value
> +        else:
> +            setattr(self.proto, attr, value)
>  
> -    def __init__(self, proto):
> -        self.proto = proto
>  
> -    def dataReceived(self, data):
> -        self.proto.transport.write(data)
> +    def connectionMade(self):
> +        """
> +        Connect our C{Protocol} to our transport.
> +        """
> +        self.proto.makeConnection(self.transport)
> +        self.transport.proto = self
>  
> -    def write(self, data):
> +
> +    def outReceived(self, data):
> +        """
> +        Give the data to the C{Protocol}s dataReceived method.
> +        """
>          self.proto.dataReceived(data)
>  
> -    def writeSequence(self, seq):
> -        self.write(''.join(seq))
>  
> -    def loseConnection(self):
> -        self.proto.connectionLost(protocol.connectionDone)
> +    def processEnded(self, reason):
> +        """
> +        Give the C{Failure} to the C{Protocol}s connectionLost method.
> +        """
> +        self.proto.connectionLost(reason)
>  
> +
> +
>  def wrapProcessProtocol(inst):
> +    """
> +    If we're passed a C{Protocol}, wrap it in a C{ProcessProtocol}.
> +    Otherwise, just return what we were passed.
> +    """
>      if isinstance(inst, protocol.Protocol):
> -        return _ProtocolWrapper(inst)
> +        return _ProtocolToProcessProtocol(inst)
>      else:
>          return inst
>

Can you use adapters for this instead?

If not, add a XXX comment saying that you *should* use adapters, and ideally, file a bug about it.

> -def wrapProtocol(proto):
> -    return _DummyTransport(proto)
>  
> +
> +class _TransportToProcessProtocol(protocol.ProcessProtocol):
> +    """
> +    This wraps something implementing I{ITransport} in a
> +    C{ProcessProtocol} because that's how the old interface worked.
> +

Please say specifically which object you mean by "the old interface".

Given that this is backwards compatibility code, it would be good to say in which version of Twisted this was added. That way, we'll know when we can remove it.

> +    @ivar applicationTransport: the ITransport we're wrapping.
> +    """
> +
> +
> +    def __init__(self, applicationTransport):
> +        self.applicationTransport = applicationTransport
> +
> +
> +    def outReceived(self, data):
> +        """
> +        When we get data, write it to our transport.
> +        """
> +        self.applicationTransport.write(data)
> +
> +
> +    def errReceived(self, errData):
> +        """
> +        When we get extended data, give it to the writeErr method of
> +        our transport.
> +        """
> +        # XXX need to test when writeErr isn't available
> +        self.applicationTransport.writeErr(errData)
> +
> +
> +    def processEnded(self, reason):
> +        """
> +        When we're told the process ended, tell the transport to drop
> +        the connection.  Yes, this loses data.
> +        """
> +        self.applicationTransport.loseConnection()
> +
> +
> +
> +class _ProcessProtocolToSessionApplication:
> +    """
> +    This adapts a C{ProcessProtocol} to an ISessionApplication.
> +
> +    @ivar processProtocol: the C{ProcessProtocol} we're adapting.
> +    """
> +

It would be good to explain in the docstring why you need this adapter.

Same comments on including the version apply.

> +
> +    implements(ISessionApplication)
> +
> +
> +    def __init__(self, processProtocol):
> +        self.processProtocol = processProtocol
> +
> +
> +    def connectionMade(self):
> +        """
> +        Connect the C{ProcessProtocol} to the channel.
> +        """
> +        self.processProtocol.makeConnection(self.channel)
> +
> +
> +    def dataReceived(self, data):
> +        """
> +        Give the data to the C{ProcessProtocol}
> +        """
> +        self.processProtocol.outReceived(data)
> +
> +
> +    def extendedDataReceived(self, type, data):
> +        """
> +        If the extended data came from standard error, give it to the
> +        C{ProcessProtocol}.  Otherwise drop it on the floor.
> +        """
> +        if type == connection.EXTENDED_DATA_STDERR:
> +            self.processProtocol.errReceived(data)
> +
> +
> +    def eofReceived(self):
> +        """
> +        Tell the C{ProcessProtocol} that no more data will be sent to
> +        standard input.
> +        """
> +        self.processProtocol.inConnectionLost()
> +
> +
> +    def closeReceived(self):
> +        """
> +        Tell the C{ProcessProtocol} that it shouldn't send any more data.
> +        """
> +        self.processProtocol.outConnectionLost()
> +        self.processProtocol.errConnectionLost()
> +
> +
> +    def closed(self):
> +        """
> +        Tell the C{ProcessProtocol} that the process ended.
> +
> +        TODO: catch request_exit_status to give a better error message.
> +        """
> +        self.processProtocol.processEnded(protocol.connectionDone)
> +
> +
> +
> +components.registerAdapter(_ProcessProtocolToSessionApplication,
> +                           protocol.ProcessProtocol, ISessionApplication)
> +
> +
> +
>  class SSHSessionProcessProtocol(protocol.ProcessProtocol):
> +    """
> +    This is a C{ProcessProtocol} that is used to connect to old-style
> +    ISession implementations.  An instance of this is passed to
> +    openShell() and execCommand(), and then it's wrapped using
> +    SSHSessionProcessProtocolApplication.
> +    """
>  
> -#    __implements__ = I
> +
> +    _signalNames = None
> +

Please add a comment explaining the structure of this variable.

> +
>      def __init__(self, session):
>          self.session = session
>  
> -    def connectionMade(self):
> -        if self.session.buf:
> -            self.transport.write(self.session.buf)
> -            self.session.buf = None
>  
> +    def _getSignalName(self, signum):
> +        """
> +        Get a signal name given a signal number.
> +        """
> +        if self._signalNames is None:
> +            self.__class__._signalNames = {}
> +            # make sure that the POSIX ones are the defaults
> +            for signame in supportedSignals:
> +                signame = 'SIG' + signame
> +                sigvalue = getattr(signal, signame, None)
> +                if sigvalue is not None:
> +                    self._signalNames[sigvalue] = signame
> +            for k, v in signal.__dict__.items():
> +                if k.startswith('SIG') and not k.startswith('SIG_'):
> +                    if v not in self._signalNames:
> +                        self._signalNames[v] = k + '@' + sys.platform
> +        return self._signalNames[signum]
> +

So, figuring this out, _signalNames maps from signal numbers to signal names.

The names are either the regular names (e.g. 'SIGKILL') or they are names with platforms at the end (e.g. 'SIGCHLD@linux'). They are in the latter if they aren't in 'supportedSignals', which appears to be a global.

The 'SIG_' guard is there because the Python signal module has variables that start with 'SIG_' that aren't the names of signals.

OK, I think I've got it. Here are the recommendations:

• Make supportedSignals 'SUPPORTED_SIGNALS'.
• If 'supported' really means 'all platforms', then say so.
• The self.class thing smells like premature optimization. Just use self.

> +
>      def outReceived(self, data):
> +        """
> +        Sent data from standard out over the channel.
> +        """
>          self.session.write(data)
>  
> +
>      def errReceived(self, err):
> +        """
> +        Send data from standard error as extended data of type
> +        EXTENDED_DATA_STDERR.
> +        """
>          self.session.writeExtended(connection.EXTENDED_DATA_STDERR, err)
>  
> +
>      def inConnectionLost(self):
> +        """
> +        If we're told the incoming connection has been lost, send an EOF
> +        over the channel.
> +        """
>          self.session.conn.sendEOF(self.session)
>  
> -    def connectionLost(self, reason = None):
> -        self.session.loseConnection()
>  
> -    def processEnded(self, reason = None):
> -        if reason and hasattr(reason.value, 'exitCode'):
> -            log.msg('exitCode: %s' % repr(reason.value.exitCode))
> -            self.session.conn.sendRequest(self.session, 'exit-status', struct.pack('!L', reason.value.exitCode))
> +    def outConnectionLost(self):
> +        """
> +        If we're running as an subsystem, close the connection.
> +        """
> +        if self.session.session is None: # we're running as an old-style
> +                                         # subsystem
> +            self.session.loseConnection()
> +
> +
> +    def processEnded(self, reason=None):
> +        """
> +        When we are told the process ended, try to notify the other side about
> +        how the process ended using the exit-signal or exit-status requests.
> +        Also, close the channel.
> +        """
> +        if reason is not None and hasattr(reason.value, 'exitCode'):
> +            err = reason.value
> +            if err.signal is not None:
> +                signame = self._getSignalName(err.signal)
> +                log.msg('exitSignal: %s' % (signame,))
> +                if hasattr(os, 'WCOREDUMP') and os.WCOREDUMP(err.status):
> +                    coreDumped = 1
> +                else:
> +                    coreDumped = 0

If core was dumped, the log should say so. This is very useful information for debugging.

> +                self.session.conn.sendRequest(self.session, 'exit-signal',
> +                        common.NS(signame[3:]) + chr(coreDumped) +
> +                        common.NS('') + common.NS(''))
> +            elif err.exitCode is not None:
> +                log.msg('exitCode: %r' % (err.exitCode,))
> +                self.session.conn.sendRequest(self.session, 'exit-status',
> +                        struct.pack('>L', err.exitCode))

What if 'err' has no exitCode and no signal?

>          self.session.loseConnection()
>  
> -    # transport stuff (we are also a transport!)
>  
> +    # also a transport :(

I share your sadness, but not your understanding. Why must this be a transport? What can we do about it? Please explain these things in the comment.

>      def write(self, data):
> +        """
> +        If we're used like a transport, just send the data to the channel.
> +        """
>          self.session.write(data)
>  
> -    def writeSequence(self, seq):
> -        self.session.write(''.join(seq))
>  
>      def loseConnection(self):
> +        """
> +        If we're use like a transport, send the close message.
> +        """

'used', not 'use'.

>          self.session.loseConnection()
>  
> -class SSHSessionClient(protocol.Protocol):
>  
> +
> +class SSHSessionProcessProtocolApplication:
> +    """
> +    Another layer of wrapping to make the old-style ISession
> +    implemention work.  This adapts SSHSessionProcessProtocol to
> +    ISessionApplication.
> +
> +    @ivar processProtocol: the C{SSHSessionProcessProtocol} we're adapting.
> +    """
> +

Gosh there are a lot of these. I think it would be worth giving a summary in the module-level docstring. I'm starting to find it all a bit confusing.

> +
> +    implements(ISessionApplication)
> +
> +
> +    def __init__(self, processProtocol):
> +        self.processProtocol = processProtocol
> +
> +
> +    def connectionMade(self):
> +        """
> +        Don't need to do anything because SSHSessionProcessProtocol's
> +        transport doesn't do anything with this.
> +        """
> +
> +
>      def dataReceived(self, data):
> -        if self.transport:
> -            self.transport.write(data)
> +        """
> +        When we get data, write it to the SSHSessionProcessProtocol's
> +        transport.
> +        """
> +        self.processProtocol.transport.write(data)
>  
> +
> +    def extendedDataReceived(self, dataType, data):
> +        """
> +        If we get extended data from standard error and the transport
> +        has a writeErr method, pass the data along.
> +        """
> +        if not hasattr(self.processProtocol.transport, 'writeErr'):
> +            return
> +        if dataType == connection.EXTENDED_DATA_STDERR:
> +            self.processProtocol.transport.writeErr(data)
> +
> +
> +    def eofReceived(self):
> +        """
> +        Don't need to implement this because
> +        SSHSessionProcessProtocol's transport doesn't do anything with
> +        this.
> +        """
> +
> +
> +    def closeReceived(self):
> +        """
> +        Don't need to implement this because
> +        SSHSessionProcessProtocol's transport doesn't do anything with
> +        this.
> +        """
> +
> +
> +    def closed(self):
> +        """
> +        Don't need to implement this because
> +        SSHSessionProcessProtocol's transport doesn't do anything with
> +        this.
> +        """
> +

No adapter registration?

> +
> +
> +class SSHSessionClient(protocol.Protocol):
> +    """
> +    A class the conch command-line client uses to connect the channel
> +    to standard output.  Deprecated.
> +    """
> +    def dataReceived(self, data):
> +        """
> +        Send data to the transport.
> +        """
> +        self.transport.write(data)
> +

If it's deprecated, raise a warning on construction.

> +
> +
>  # methods factored out to make live easier on server writers
>  def parseRequest_pty_req(data):
> -    """Parse the data from a pty-req request into usable data.
> +    """
> +    Parse the data from a pty-req request into usable data.
>  
>      @returns: a tuple of (terminal type, (rows, cols, xpixel, ypixel), modes)
>      """

Please say where 'terminal type' and 'modes' come from.

By the way, there's no harm at all in referring to the RFC number and section in a docstring, especially for a protocol as complex as SSH.

> @@ -226,30 +900,48 @@
>      cols, rows, xpixel, ypixel = struct.unpack('>4L', rest[: 16])
>      modes, ignored= common.getNS(rest[16:])
>      winSize = (rows, cols, xpixel, ypixel)
> -    modes = [(ord(modes[i]), struct.unpack('>L', modes[i+1: i+5])[0]) for i in range(0, len(modes)-1, 5)]
> +    modes = [(ord(modes[i]), struct.unpack('>L', modes[i+1: i+5])[0]) for i in
> +            range(0, len(modes)-1, 5)]

There should be no space after the colon, and spaces around the '-' in 'len(modes)-1'.

>      return term, winSize, modes
>  
> +
> +
>  def packRequest_pty_req(term, (rows, cols, xpixel, ypixel), modes):
> -    """Pack a pty-req request so that it is suitable for sending.
> +    """
> +    Pack a pty-req request so that it is suitable for sending.
>  
>      NOTE: modes must be packed before being sent here.
>      """
>      termPacked = common.NS(term)
>      winSizePacked = struct.pack('>4L', cols, rows, xpixel, ypixel)
> -    modesPacked = common.NS(modes) # depend on the client packing modes
> +    if not isinstance(modes, str):
> +        modes = ''.join([chr(m[0]) + struct.pack('>L', m[1]) for m in modes])
> +    else:
> +        warnings.warn("packRequest_pty_req should be packing the modes.",
> +                      DeprecationWarning, stacklevel=2)

There's an API for this now: twisted.python.deprecate.

> +    modesPacked = common.NS(modes)
>      return termPacked + winSizePacked + modesPacked
>  
> +
> +
>  def parseRequest_window_change(data):
> -    """Parse the data from a window-change request into usuable data.
> +    """
> +    Parse the data from a window-change request into usuable data.
>  
>      @returns: a tuple of (rows, cols, xpixel, ypixel)
>      """
>      cols, rows, xpixel, ypixel = struct.unpack('>4L', data)
>      return rows, cols, xpixel, ypixel
>  
> +
> +
>  def packRequest_window_change((rows, cols, xpixel, ypixel)):
> -    """Pack a window-change request so that it is suitable for sending.
>      """
> +    Pack a window-change request so that it is suitable for sending.
> +    """
>      return struct.pack('>4L', cols, rows, xpixel, ypixel)
>  
> -import connection
> +
> +
> +__all__ = ['SSHSession', 'packRequest_window_change', 'packRequest_pty_req',
> +           'parseRequest_window_change', 'parseRequest_pty_req']

I think the world would be a happier place if this were at the top of the file.

> Index: twisted/conch/avatar.py
> ===================================================================
> --- twisted/conch/avatar.py	(revision 23210)
> +++ twisted/conch/avatar.py	(working copy)
> @@ -1,9 +1,11 @@
>  # -*- test-case-name: twisted.conch.test.test_conch -*-
> -from interfaces import IConchUser
> -from error import ConchError
> -from ssh.connection import OPEN_UNKNOWN_CHANNEL_TYPE
> -from twisted.python import log
> +# Copyright (c) 2007 Twisted Matrix Laboratories.
> +# See LICENSE for details.
> +

2008 :)

>  from zope import interface
> +from twisted.conch.interfaces import IConchUser
> +from twisted.conch.error import ConchError
> +from twisted.conch.ssh.connection import OPEN_UNKNOWN_CHANNEL_TYPE
>  
>  class ConchUser:
>      interface.implements(IConchUser)
> @@ -17,12 +19,16 @@
>          if not klass:
>              raise ConchError(OPEN_UNKNOWN_CHANNEL_TYPE, "unknown channel")
>          else:
> -            return klass(remoteWindow = windowSize, 
> -                         remoteMaxPacket = maxPacket, 
> +            return klass(remoteWindow=windowSize,
> +                         remoteMaxPacket=maxPacket,
>                           data=data, avatar=self)
>  
>      def lookupSubsystem(self, subsystem, data):
> -        log.msg(repr(self.subsystemLookup))
> +        """
> +        This is deprecated in favor of ISession.lookupSubsystem.  However,
> +        because there is already a warning in SSHSession.request_subsystem,
> +        we do not warn again here.
> +        """
>          klass = self.subsystemLookup.get(subsystem, None)
>          if not klass:
>              return False
> Index: twisted/conch/interfaces.py
> ===================================================================
> --- twisted/conch/interfaces.py	(revision 23210)
> +++ twisted/conch/interfaces.py	(working copy)
> @@ -4,7 +4,7 @@
>  from zope.interface import Interface
>  
>  class IConchUser(Interface):
> -    """A user who has been authenticated to Cred through Conch.  This is 
> +    """A user who has been authenticated to Cred through Conch.  This is
>      the interface between the SSH connection and the user.
>  
>      @ivar conn: The SSHConnection object for this user.
> @@ -30,26 +30,24 @@
>          @rtype:             subclass of L{SSHChannel}/C{tuple}
>          """
>  
> -    def lookupSubsystem(subsystem, data):
> -        """
> -        The other side requested a subsystem.
> -        subsystem is the name of the subsystem being requested.
> -        data is any other packet data (often nothing).
> -        
> -        We return a L{Protocol}.
> -        """
>  
> +
>      def gotGlobalRequest(requestType, data):
>          """
>          A global request was sent from the other side.
> -        
> +
>          By default, this dispatches to a method 'channel_channelType' with any
> -        non-alphanumerics in the channelType replace with _'s.  If it cannot 
> -        find a suitable method, it returns an OPEN_UNKNOWN_CHANNEL_TYPE error. 
> +        non-alphanumerics in the channelType replace with _'s.  If it cannot
> +        find a suitable method, it returns an OPEN_UNKNOWN_CHANNEL_TYPE error.
>          The method is called with arguments of windowSize, maxPacket, data.
>          """
>  
> +
>  class ISession(Interface):
> +    """
> +    The old, deprecated interface for implementing SSH session applications.
> +    Please don't use this for anything.
> +    """
>

Please say which version it was deprecated in.

>      def getPty(term, windowSize, modes):
>          """
> @@ -82,13 +80,143 @@
>          """
>          Called when the other side has indicated no more data will be sent.
>          """
> -        
> +
>      def closed():
>          """
>          Called when the session is closed.
>          """
>  
>  
> +
> +
> +class ISessionApplicationFactory(Interface):
> +    """
> +    The new, good interface for implementing an SSH session.  Use this to
> +    implement the application side of an SSH server session.
> +
> +    @ivar channel: the L{SSHChannel} object to which this application is
> +    connected.  Use this attribute to send data, etc.
> +    """
> +
> +    def getPTY(term, windowSize, modes):
> +        """
> +        Get a psuedo-terminal for use by a shell or command.
> +
> +        If a psuedo-terminal is not available, or the request otherwise
> +        fails, raise an exception.
> +        """
> +

"pseudo", not "psuedo".

> +
> +    def lookupSubsystem(subsystem, data):
> +        """
> +        The other side requested a subsystem.

Blank line.

> +        @param subsystem: the name of the subsystem being requested.
> +        @param data: any other packet data (often nothing).
> +
> +        @return: something that implements I{IConchApplication}
> +        @raises: If the subsystem is unavailable, raise an exception.
> +        """
> +
> +
> +    def openShell():
> +        """
> +        The other side requested a shell.
> +
> +        @return: something that implementes I{IConchApplication}
> +        @raises: If the subsystem is unavailable, raise an exception.
> +        """
> +
> +
> +    def execCommand(command):
> +        """
> +        The other side requested a shell.
> +
> +        @param command: the command the other side wants us to execute
> +        @type command: C{str}
> +
> +        @return: something that implementes I{IConchApplication}
> +        @raises: If the subsystem is unavailable, raise an exception.
> +        """
> +
> +
> +    def windowChanged(newWindowSize):
> +        """
> +        Called when the size of the remote screen has changed.
> +        """
> +
> +
> +    def eofReceived():
> +        """
> +        Called when the other side has indicated no more data will be sent.
> +        """
> +
> +
> +    def closeReceived():
> +        """
> +        Called when the other side has indicated it wants no more data.
> +        """
> +
> +
> +    def closed():
> +        """
> +        Called when the session channel is closed.
> +        """
> +
> +

One more blank line please.

> +class ISessionApplication(Interface):
> +    """
> +    Objects which are returned by an ISessionApplicationFactory implementor
> +    should implement this class.  It receives data from the channel and can
> +    send it back.
> +
> +    @ivar channel: the C{SSHSession} to which this application is connected.
> +    """
> +
> +
> +    def connectionMade():
> +        """
> +        Called when this application is connected to the channel.
> +        """
> +
> +
> +    def dataReceived(data):
> +        """
> +        Called when data is received from the other side.
> +
> +        @type data: C{str}
> +        """
> +
> +
> +    def extendedDataReceived(extendedDataType, extendedData):
> +        """
> +        Called when extended data is received from the other side.
> +
> +        @param extendedDataType: the type of extended data.  Typically, this
> +            is connection.EXTENDED_DATA_STDERR.
> +        @type extendedDataType: C{int}
> +        @type extendedData: C{str}
> +        """
> +
> +
> +    def eofReceived():
> +        """
> +        Called when the other side has said it will not send any more data.
> +        """
> +
> +
> +    def closeReceived():
> +        """
> +        Called when the other side has said it will not accept any more data.
> +        """
> +
> +
> +    def closed():
> +        """
> +        Called when the application is closed on both sides.
> +        """
> +
> +
> +
>  class ISFTPServer(Interface):
>      """
>      The only attribute of this class is "avatar".  It is the avatar
> @@ -350,5 +478,3 @@
>          @param attrs: a dictionary in the same format as the attrs argument to
>          L{openFile}.
>          """
> -
> -
> Index: twisted/conch/unix.py
> ===================================================================
> --- twisted/conch/unix.py	(revision 23210)
> +++ twisted/conch/unix.py	(working copy)
> @@ -5,19 +5,20 @@
>  from twisted.python import components, log
>  from twisted.internet.error import ProcessExitedAlready
>  from zope import interface
> -from ssh import session, forwarding, filetransfer
> -from ssh.filetransfer import FXF_READ, FXF_WRITE, FXF_APPEND, FXF_CREAT, FXF_TRUNC, FXF_EXCL
> +from twisted.conch.ssh import session, forwarding, filetransfer
> +from twisted.conch.ssh.filetransfer import FXF_READ, FXF_WRITE, FXF_APPEND
> +from twisted.conch.ssh.filetransfer import FXF_CREAT, FXF_TRUNC, FXF_EXCL
>  from twisted.conch.ls import lsLine
>  
> -from avatar import ConchUser
> -from error import ConchError
> -from interfaces import ISession, ISFTPServer, ISFTPFile
> +from twisted.conch.avatar import ConchUser
> +from twisted.conch.error import ConchError
> +from twisted.conch.interfaces import ISession, ISFTPServer, ISFTPFile
> +from twisted.conch import ttymodes
>  
>  import struct, os, time, socket
>  import fcntl, tty
>  import pwd, grp
>  import pty
> -import ttymodes
>  
>  try:
>      import utmp
> @@ -48,9 +49,6 @@
>                  {"session": session.SSHSession,
>                   "direct-tcpip": forwarding.openConnectForwardingClient})
>  
> -        self.subsystemLookup.update(
> -                {"sftp": filetransfer.FileTransferServer})
> -
>      def getUserGroupId(self):
>          return self.pwdData[2:4]
>  
> @@ -67,10 +65,10 @@
>          hostToBind, portToBind = forwarding.unpackGlobal_tcpip_forward(data)
>          from twisted.internet import reactor
>          try: listener = self._runAsUser(
> -                            reactor.listenTCP, portToBind, 
> +                            reactor.listenTCP, portToBind,
>                              forwarding.SSHListenForwardingFactory(self.conn,
>                                  (hostToBind, portToBind),
> -                                forwarding.SSHListenServerForwardingChannel), 
> +                                forwarding.SSHListenServerForwardingChannel),
>                              interface = hostToBind)
>          except:
>              return 0
> @@ -95,7 +93,8 @@
>          # remove all listeners
>          for listener in self.listeners.itervalues():
>              self._runAsUser(listener.stopListening)
> -        log.msg('avatar %s logging out (%i)' % (self.username, len(self.listeners)))
> +        log.msg('avatar %s logging out (%i)' % (self.username,
> +            len(self.listeners)))
>  
>      def _runAsUser(self, f, *args, **kw):
>          euid = os.geteuid()
> @@ -160,7 +159,6 @@
>          b = utmp.UtmpRecord(utmp.WTMP_FILE)
>          b.pututline(entry)
>          b.endutent()
> -                            
>  
>      def getPty(self, term, windowSize, modes):
>          self.environ['TERM'] = term
> @@ -168,9 +166,13 @@
>          self.modes = modes
>          master, slave = pty.openpty()
>          ttyname = os.ttyname(slave)
> -        self.environ['SSH_TTY'] = ttyname 
> +        self.environ['SSH_TTY'] = ttyname
>          self.ptyTuple = (master, slave, ttyname)
>  
> +    def lookupSubsystem(subsystem, data):
> +        if subsystem == 'sftp':
> +            return filetransfer.FileTransferServer(data)
> +
>      def openShell(self, proto):
>          from twisted.internet import reactor
>          if not self.ptyTuple: # we didn't get a pty-req
> @@ -191,7 +193,7 @@
>                    shell, ['-%s' % shellExec], self.environ, homeDir, uid, gid,
>                     usePTY = self.ptyTuple)
>          self.addUTMPEntry()
> -        fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ, 
> +        fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ,
>                      struct.pack('4H', *self.winSize))
>          if self.modes:
>              self.setModes()
> @@ -232,7 +234,7 @@
>          finally:
>              os.setegid(egid)
>              os.seteuid(euid)
> -        
> +
>      def setModes(self):
>          pty = self.pty
>          attr = tty.tcgetattr(pty.fileno())
> @@ -276,7 +278,7 @@
>  
>      def windowChanged(self, winSize):
>          self.winSize = winSize
> -        fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ, 
> +        fcntl.ioctl(self.pty.fileno(), tty.TIOCSWINSZ,
>                          struct.pack('4H', *self.winSize))
>  
>      def _writeHack(self, data):

Here the diff ends, what follows is the added test file.

> === twisted/conch/test/test_session.py ===
> # Copyright (c) 2007-2008 Twisted Matrix Laboratories.
> # See LICENSE for details.
> 
> """
> Tests for the 'session' channel implementation in twisted.conch.ssh.session.
> """
>

This is a good place to refer to the RFC.

> import os, signal, sys, struct
> 
> from zope.interface import implements
> 
> from twisted.conch.ssh import common, connection, session
> from twisted.internet import defer, protocol, error
> from twisted.python import components, failure
> from twisted.trial import unittest
> 
> 
> 
> class SubsystemOnlyAvatar(object):
>     """
>     A stub class representing an avatar that is only useful for
>     getting a subsystem.
>     """
> 
> 
>     def lookupSubsystem(self, name, data):
>         """
>         If the other side requests the 'subsystem' subsystem, allow it by
>         returning a MockProcessProtocol to implement it.  Otherwise, fail.
>         """
>         if name == 'subsystem':
>             return MockProcessProtocol()
>

When I read 'fail', I think 'raise some sort of error', not 'return None'. The docstring would be more helpful if it explained the impact of returning None in this case.

> 
> 
> class StubOldAvatar(object):
>     """
>     A stub class representing the avatar representing the authenticated user.
>     """
>

Please explain why it's 'Old'.

> 
>     def lookupSubsystem(self, name, data):
>         """
>         If the user requests the TestSubsystem subsystem, connect them
>         to our MockProcessProtocol.  If they request the protocol
>         subsystem, connect them to a MockProtocol.
>         """
>         if name == 'TestSubsystem':
>             self.subsystem = MockProcessProtocol()
>             self.subsystem.packetData = data
>             return self.subsystem
>         elif name == 'protocol':
>             self.subsystem = MockProtocol()
>             self.subsystem.packetData = data
>             return self.subsystem
>

And if they request neither, what happens?

> 
> 
> class StubSessionForStubOldAvatar(object):
>     """
>     A stub ISession implementation for our StubOldAvatar.
> 
>     @ivar avatar: the C{StubOldAvatar} we are adapting.
>     @ivar ptyRequest: if present, the terminal, window size, and modes passed
>         to the getPty method.
>     @ivar windowChange: if present, the window size passed to the
>         windowChangned method.
>     @ivar shellProtocol: if present, the C{SSHSessionProcessProtocol} passed
>         to the openShell method.
>     @ivar shellTransport: if present, the C{EchoTransport} connected to
>         shellProtocol.
>     @ivar execProtocol: if present, the C{SSHSessionProcessProtocol} passed
>         to the execCommand method.
>     @ivar execTransport: if present, the C{EchoTransport} connected to
>         execProtocol.
>     @ivar execCommandLine: if present, the command line passed to the
>         execCommand method.
>     @ivar gotEOF: if present, an EOF message was received.
>     @ivar gotClosed: if present, a closed message was received.
>     """
>

I guess you have all of those here so you can manipulate them from tests.

Looking down, I guessed wrong. These are like a log of the things that have been done to this session object. It's worth making a general comment about this in the docstring.

You should also mention that certain calls to certain methods will trigger errors.

Note that a more usual way of doing this is to have a 'log' or 'calls' attribute that appends information about the calls to a list. No need to change it though.

> 
>     implements(session.ISession)
> 
> 
>     def __init__(self, avatar):
>         """
>         Store the avatar we're adapting.
>         """
>         self.avatar = avatar
>         self.shellProtocol = None
> 
> 
>     def getPty(self, terminal, window, modes):
>         """
>         If the terminal is 'bad', fail.  Otherwise, store the information in
>         the ptyRequest variable.
>         """
>         if terminal != 'bad':
>             self.ptyRequest = (terminal, window, modes)
>         else:
>             raise RuntimeError('not getting a pty')
> 
> 
>     def windowChanged(self, window):
>         """
>         If all the window sizes are 0, fail.  Otherwise, store the size in the
>         windowChange variable.
>         """
>         if window == (0, 0, 0, 0):
>             raise RuntimeError('not changing the window size')
>         else:
>             self.windowChange = window
> 
> 
>     def openShell(self, pp):
>         """
>         If we have gotten a shell request before, fail.  Otherwise, store the
>         process protocol in the shellProtocol variable, connect it to the
>         EchoTransport and store that as shellTransport.
>         """
>         if self.shellProtocol is not None:
>             raise RuntimeError('not getting a shell this time')
>         else:
>             self.shellProtocol = pp
>             self.shellTransport = EchoTransport(pp)
> 
> 
>     def execCommand(self, pp, command):
>         """
>         If the command is 'true', store the command, the process protocol, and
>         the transport we connect to the process protocol.  Otherwise, just
>         store the command and raise an error.
>         """
>         self.execCommandLine = command
>         if command == 'true':
>             self.execProtocol = pp
>         elif command[:4] == 'echo':
>             self.execProtocol = pp
>             self.execTransport = EchoTransport(pp)
>             pp.outReceived(command[5:])
>         else:
>             raise RuntimeError('not getting a command')
> 
> 
>     def eofReceived(self):
>         """
>         Note that EOF has been received.
>         """
>         self.gotEOF = True
> 
> 
>     def closed(self):
>         """
>         Note that close has been received.
>         """
>         self.gotClosed = True
> 
> 
> 
> components.registerAdapter(StubSessionForStubOldAvatar, StubOldAvatar,
>         session.ISession)
> 
> 
> 
> class StubNewAvatar(object):
>     """
>     A stub avatar.  It does not need any methods, as it is only used as the
>     object StubSessionForStubNewAvatar adapts.
>     """
> 
> 
> 
> class StubApplicationFactoryForStubNewAvatar:
>     """
>     A stub ISessionApplicationFactory implementation for our StubNewAvatar.
> 
>     @ivar avatar: the C{StubOldAvatar} we are adapting.
>     @ivar inConnectionOpen: C{True} if the input side is open.
>     @ivar outConnectionOpen: C{True} if the output side is open.
>     @ivar ended: C{True} if the session has ended.
>     @ivar subsystem: if present, the C{MockApplication} that is the
>         current subsystem.  It has a packetData ivar which is the C{str} of
>         data passed in the subsystem request.
>     @ivar term: if present, the terminal name passed to getPty
>     @ivar windowSize: if present, the window size passed to getPty or
>         windowChanged
>     @ivar modes: if present, the terminal modes passed to getPty
>     @ivar command: if present, the C{MockApplication} that is the
>         current command.  It has a command ivar which is the C{str} giving
>         the command's name.
>     @ivar shell: if present, the C{MockApplication} that is the current
>         shell.
>     """
> 
> 
>     implements(session.ISessionApplicationFactory)
> 
> 
>     def __init__(self, avatar):
>         self.avatar = avatar
>         self.inConnectionOpen = True
>         self.outConnectionOpen = True
>         self.ended = False
>         self.subsystem = None
>         self.command = None
>         self.shell = None
> 
> 
>     def lookupSubsystem(self, subsystem, data):
>         """
>         Request a subsystem.  If one has been requested already, raise an
>         exception.  Otherwise, set self.subsystem to a MockApplication and
>         return it.
>         """
>         if self.subsystem is not None:
>             raise ValueError('already opened a subsystem')
>         if subsystem == 'echo':
>             self.subsystem = MockApplication()
>             self.subsystem.packetData = data
>             return self.subsystem
> 
> 
>     def getPTY(self, term, windowSize, modes):
>         """
>         Request a psuedoterminal.  Store the data passed to us.
>         """

"pseudo", not "psuedo".

>         self.term = term
>         self.windowSize = windowSize
>         self.modes = modes
> 
> 
>     def windowChanged(self, newSize):
>         """
>         The window size has changed.  Store the data.
>         """
>         self.windowSize = newSize
> 
> 
>     def execCommand(self, command):
>         """
>         Request a command.  If one has been requested already, raise an
>         exception.  Otherwise, set self.command to a MockApplication and
>         return it.
>         """
> 
>         if self.command is not None:
>             raise RuntimeError('already executed a command')
>         self.command = MockApplication()
>         self.command.command = command
>         return self.command
> 
> 
>     def openShell(self):
>         """
>         Request a shell.  If one has been requested already, raise an
>         exception.  Otherwise, set self.shell to a MockApplication and
>         return it.
>         """
> 
>         if self.shell is not None:
>             raise RuntimeError('already opened a shell')
>         self.shell = MockApplication()
>         return self.shell
> 
> 
>     def eofReceived(self):
>         """
>         Close the input side.
>         """
>         self.inConnectionOpen = False
> 
> 
>     def closeReceived(self):
>         """
>         Close the output side.
>         """
>         self.outConnectionOpen = False
> 
> 
>     def closed(self):
>         """
>         End the session.
>         """
>         self.ended = True
>

I see you've taken the same approach here. The same comments apply.

> 
> 
> components.registerAdapter(StubApplicationFactoryForStubNewAvatar,
>                            StubNewAvatar,
>                            session.ISessionApplicationFactory)
> 
> 
> 
> class MockApplication:
>     """
>     A mock ISessionApplication.  This is the new interface that
>     clients of SSHSession should implement.
> 
>     @ivar data: a C{list} of C{str} passed to our dataReceived.
>     @ivar extendedData: a C{list} of C{tuple} of (C{int}, C{str})
>         passed to our extendedDataReceived.
>     @ivar hasClosed: a C{bool} indicating whether the application is closed.
>     @ivar gotEOF: if present, we have received an EOF from the other side.
>     @ivar gotClose: if present, we have received a close message from the other
>        side
>     """
> 
> 
>     implements(session.ISessionApplication)
> 
> 
>     def connectionMade(self):
>         """
>         Called when the application is connected to the other side.
>         Initialize our instance variables.
>         """
>         self.data = []
>         self.extendedData = []
>         self.hasClosed = False
> 
> 
>     def dataReceived(self, data):
>         """
>         We got some data.  Store it, and echo it back with a tilde appended.
>         """

Why?

>         self.data.append(data)
>         self.channel.write(data + '~')
> 
> 
>     def extendedDataReceived(self, type, data):
>         """
>         We got some extended data.  Store it, and echo it back with an
>         incremented data type and with a tilde appended to the data.
>         """

Again, please say _why_ you are appending a tilde.

>         self.extendedData.append((type, data))
>         self.channel.writeExtended(type + 1, data + '~')
> 
> 
>     def eofReceived(self):
>         """
>         Note that we received an EOF.
>         """
>         self.gotEOF = True
> 
> 
>     def closeReceived(self):
>         """
>         Note that we received a close message.
>         """
>         self.gotClose = True
> 
> 
>     def closed(self):
>         """
>         Note that this application is closed.
>         """
>         self.hasClosed = True
> 
> 
> 
> class MockApplicationSendingOnConnection(MockApplication):
>     """
>     This is a a simple subclass of MockApplication which sends some
>     data when it is connected.
>     """
> 
> 
>     def connectionMade(self):
>         """
>         Write an introduction when we are connected.
>         """
>         MockApplication.connectionMade(self)
>         self.channel.write('intro')
> 
> 
> 
> class MockProcessProtocol(protocol.ProcessProtocol):
>     """
>     A mock ProcessProtocol which echoes back data sent to it and
>     appends a tilde.
>

I'm beginning to suspect a tilde fetish. Those sleek curves, that wavy form, the way it reminds me of home...

Say why you are adding a tilde. What's the intent?

>     @ivar data: a C{str} of data received.
>     @ivar err: a C{str} of error data received.
>     @ivar inConnectionOpen: True if the input side is open.
>     @ivar outConnectionOpen: True if the output side is open.
>     @ivar errConnectionOpen: True if the error side is open.
>     @ivar ended: False if the protocol has not ended, a C{Failure} if the
>         process has ended.
>     """
>     packetData = ''
> 
> 
>     def connectionMade(self):
>         """
>         Set up variables.
>         """
>         self.data = ''
>         self.err = ''
>         self.inConnectionOpen = True
>         self.outConnectionOpen = True
>         self.errConnectionOpen = True
>         self.ended = False
>         if self.packetData:
>             self.outReceived(self.packetData)
> 
> 
>     def outReceived(self, data):
>         """
>         Data was received.  Store it and echo it back with a tilde.
>         """
>         self.data += data
>         self.transport.write(data + '~')
> 
> 
>     def errReceived(self, data):
>         """
>         Error data was received.  Store it and echo it back backwards.
>         """
>         self.err += data
>         self.transport.write(data[::-1])
> 
> 
>     def inConnectionLost(self):
>         """
>         Close the input side.
>         """
>         self.inConnectionOpen = False
> 
> 
>     def outConnectionLost(self):
>         """
>         close the output side.
>         """

'Close', not 'close'.

>         self.outConnectionOpen = False
> 
> 
>     def errConnectionLost(self):
>         """
>         Close the error side.
>         """
>         self.errConnectionOpen = False
> 
> 
>     def processEnded(self, reason):
>         """
>         End the process and store the reason.
>         """
>         self.ended = reason
> 
> 
> 
> class EchoTransport:
>     """
>     A transport for a ProcessProtocol which echos data that is sent to it with
>     a Window newline (\\r\\n) appended to it.  If a null byte is in the data,

The normal way to say this is 'CR LF'.

>     disconnect.  When we are asked to disconnect, disconnect the C{ProcessProtocol}
>     with a 0 exit code.
> 
>     @ivar proto: the C{ProcessProtocol} connected to us.
>     @ivar data: a C{str} of data written to us.
>     """
> 
> 
>     def __init__(self, p):
>         """
>         Initialize our instance variables.
> 
>         @param p: a C{ProcessProtocol} to connect to ourself.
>         """
>         self.proto = p
>         p.makeConnection(self)
>         self.closed = False
>         self.data = ''
>

If you feel like it, rename 'p' to 'processProtocol'.

> 
>     def write(self, data):
>         """
>         We got some data.  Give it back to our C{ProcessProtocol} with
>         a newline attached.  Disconnect if there's a null byte.
>         """
>         self.data += data
>         self.proto.outReceived(data)
>         self.proto.outReceived('\r\n')
>         if '\x00' in data: # mimic 'exit' for the shell test
>             self.loseConnection()
> 
> 
>     def loseConnection(self):
>         """
>         If we're asked to disconnect (and we haven't already) shut down
>         the C{ProcessProtocol} with a 0 exit code.
>         """
>         if self.closed: return

Put a newline after the colon.

>         self.closed = 1
>         self.proto.inConnectionLost()
>         self.proto.outConnectionLost()
>         self.proto.errConnectionLost()
>         self.proto.processEnded(failure.Failure(
>                 error.ProcessTerminated(0, None, None)))
> 
> 
> 
> class MockProtocol(protocol.Protocol):
>     """
>     A sample Process which stores the data passed to it.
>

This isn't a 'Process'. Perhaps you mean 'protocol'?

>     @ivar data: a C{str} of the data passed to us.
>     @ivar open: True if the channel is open.
>     @ivar reason: if not None, the reason the protocol was closed.
>     """
>     packetData = ''
>

packetData lacks documentation. What is it? Why is it there?

> 
>     def connectionMade(self):
>         """
>         Set up the instance variables.
>         """

Not the most helpful docstring in the world. Nothing obviously better springs to mind. I see you use this in other places too.

>         self.data = ''
>         self.open = True
>         self.reason = None
>         if self.packetData:
>             self.dataReceived(self.packetData)
> 
> 
>     def dataReceived(self, data):
>         """
>         Store the received data.
>         """
>         self.data += data
>         self.transport.write(data + '~')
>

Hey look! It's that tilde again!

> 
>     def connectionLost(self, reason):
>         """
>         Close the protocol and store the reason.
>         """
>         self.open = False
>         self.reason = reason
> 
>

At this point I'm really starting to look forward to reading the tests.

> 
> class StubConnection:
>     """
>     A stub for twisted.conch.ssh.connection.SSHConnection.  Record the data
>     that channels send, and when they try to close the connection.
> 
>     @ivar data: a C{dict} mapping C{SSHChannel}s to a C{list} of C{str} of data
>        they sent.
>     @ivar extData: a C{dict} mapping C{SSHChannel}s to a C{list} of C{tuple} of
>        (C{int}, C{str}) of extended data they sent.
>     @ivar requests: a C{dict} mapping C{SSHChannel}s to a C{list} of C{tuple} of
>        (C{str}, C{str}) of channel requests they made.
>     @ivar eofs: a C{dict} mapping C{SSHChannel}s to C{true} if they have sent an
>        EOF.
>     @ivar closes: a C{dict} mapping C{SSHChannel}s to C{true} if they have sent a
>        close.
>     """
> 
> 
>     def __init__(self):
>         """
>         Initialize our instance variables.
>         """
>         self.data = {}
>         self.extData = {}
>         self.requests = {}
>         self.eofs = {}
>         self.closes = {}
> 
> 
>     def logPrefix(self):
>         """
>         Return our logging prefix.
>         """
>         return "MockConnection"
> 
> 
>     def sendData(self, channel, data):
>         """
>         Record the sent data.
>         """
>         self.data.setdefault(channel, []).append(data)
> 
> 
>     def sendExtendedData(self, channel, type, data):
>         """
>         Record the sent extended data.
>         """
>         self.extData.setdefault(channel, []).append((type, data))
> 
> 
>     def sendRequest(self, channel, request, data, wantReply=False):
>         """
>         Record the sent channel request.
>         """
>         self.requests.setdefault(channel, []).append((request, data,
>             wantReply))
>         if wantReply:
>             return defer.succeed(None)
> 
> 
>     def sendEOF(self, channel):
>         """
>         Record the sent EOF.
>         """
>         self.eofs[channel] = True
> 
> 
>     def sendClose(self, channel):
>         """
>         Record the sent close.
>         """
>         self.closes[channel] = True
> 
> 
> 
> class StubTransport:
>     """
>     A stub transport which records the data written.
> 
>     @ivar buf: the data sent to the transport.
>     @type buf: C{str}
> 
>     @ivar err: the extended data sent to the transport.
>     @type err: C{str}
> 
>     @ivar close: flags indicating if the transport has been closed.
>     @type close: C{bool}
>     """
> 
>     buf = ''
>     err = ''
>     close = False
> 
> 
>     def write(self, data):
>         """
>         Record data in the buffer.
>         """
>         self.buf += data
> 
> 
>     def writeErr(self, data):
>         """
>         Record the extended data in the buffer.
>         """
>         self.err += data
>

It records extended data and it's called 'writeErr'? That seems weird. Can you please explain the discrepancy in the docstring.

> 
>     def loseConnection(self):
>         """
>         Note that the connection was closed.
>         """
>         self.close = True
> 
> 
> 
> class StubClient(object):
>     """
>     A stub class representing the client to a SSHSession.
> 
>     @ivar transport: A C{StubTransport} object which keeps track of the data
>         passed to it.
>     """
> 
> 
>     def __init__(self):
>         self.transport = StubTransport()
> 
> 
> 
> class OldSessionInterfaceTestCase(unittest.TestCase):
>     """
>     Tests for the old SSHSession class interface.  This interface is not ideal,
>     but it is tested in order to maintain backwards compatibility.
>     """
>

Thanks for the docstring, it helps.

> 
>     def setUp(self):
>         """
>         Make an SSHSession object to test.  Give the chanel some window
>         so that it's allowed to send packets.
>         """

"channel", not "chanel".

If the numbers are arbitrary, say so. If they aren't, say why they were chosen.

>         self.session = session.SSHSession(remoteWindow=500,
>                 remoteMaxPacket=100, conn=StubConnection(),
>                 avatar=StubOldAvatar())
> 
> 
>     def test_init(self):
>         """
>         Test that SSHSession initializes its buffer (buf), client, and
>         ISession adapter.  The avatar should not need to be adaptable to an
>         ISession immediately.
>         """
>         s = session.SSHSession(avatar=object) # use object because it doesn't
>                                               # have an adapter

I don't like this style of comment, and it doesn't appear much in the rest of Twisted. It's no big deal though.

A comment here that applies to all of your other tests:

Don't say "Test that" in the docstring of a test. We know it's a test. Saying "test that" or "check that" or "foo should bar" encourages one to write docstrings that speak in terms of implemention.

Instead, describe the behaviour. "SSHSession initializes its buffer, ...".

>         self.assertEquals(s.buf, '')
>         self.assertIdentical(s.client, None)
>         self.assertIdentical(s.session, None)
> 
> 
>     def _testGetsSession(self):
>         """
>         Check that a function will cause SSHSession to generate a session.
> 
>         We return a C{tuple} of a C{SSHSession} and aC{Deferred} which expects

Add a space after 'a'.

>         to be called back with a function and arguments to be called.  After
>         the function is called, the C{SSHSession} should have a session
>         attribute which provides I{ISession}.
>         """

This would be a good spot to say *why* you need this helper.

>         s = session.SSHSession()
>         d = defer.Deferred()
>         def check(fargs):

Say "functionAndArgs", please. For me.

Actually, maybe it's better to assume it's a nullary function and then use lambdas when you need to invoke it.

>             f = fargs[0]
>             args = fargs[1:]
>             s.avatar = StubOldAvatar()
>             self.assertIdentical(s.session, None)
>             f(*args)

At this point I'm curious, what sort of function do we expect here?

>             self.assertTrue(session.ISession.providedBy(s.session))
>         d.addCallback(check)
>         return s, d
>

This is one weird function.

> 
>     def test_requestShellGetsSession(self):
>         """
>         If an ISession adapter isn't already present, request_shell should get
>         one.
>         """
>         s, d = self._testGetsSession()
>         return d.callback((s.requestReceived, 'shell', ''))
> 
> 
>     def test_requestExecGetsSession(self):
>         """
>         If an ISession adapter isn't already present, request_exec should get
>         one.
>         """
>         s, d = self._testGetsSession()
>         return d.callback((s.requestReceived, 'exec', common.NS('true')))
> 
> 
>     def test_requestPtyReqGetsSession(self):
>         """
>         If an ISession adapter isn't already present, request_pty_req should
>         get one.
>         """
>         s, d = self._testGetsSession()
>         return d.callback((s.requestReceived, 'pty_req',
>             session.packRequest_pty_req(
>             'term', (0, 0, 0, 0), [])))
> 
> 
>     def test_requestWindowChangeGetsSession(self):
>         """
>         If an ISession adapter isn't already present, request_window_change
>         should get one.
>         """
>         s, d = self._testGetsSession()
>         return d.callback((s.requestReceived, 'window_change',
>             session.packRequest_window_change((1, 1, 1, 1))))
>

This is a long, long way from idiomatic Twisted code.

 http://www.osnews.com/images/comics/wtfm.jpg

Looking back at _testGetsSession, everything up to and including the assertIdentical line could easily go into setUp. Then, the tests could simply call the methods they are testing and then do the assertion manually.

From all I can see, the Deferred is a red herring.

> 
>     def test_client(self):
>         """
>         Test that SSHSession passes data and extended data along to a client.
>         """
>         self.session.dataReceived('1')
>         self.session.extReceived(connection.EXTENDED_DATA_STDERR, '3')
>         # these don't get sent as part of the client interface
>         self.session.client = StubClient()
>         self.session.dataReceived('2')
>         self.session.extReceived(connection.EXTENDED_DATA_STDERR, '4')
>         self.assertEquals(self.session.client.transport.buf, '2')
>         self.assertEquals(self.session.client.transport.err, '4')
>         self.session.eofReceived()
>         self.assertTrue(self.session.conn.closes[self.session])
>         self.session.closed()
>         self.assertTrue(self.session.client.transport.close)
>         self.session.client.transport.close = False
>

Please split this into three tests, one for each assert block.

> 
>     def test_lookupSubsystem(self):
>         """
>         When a client requests a subsystem, the SSHSession object should get
>         the subsystem by calling avatar.lookupSubsystem, and attach it as
>         the client.
>         """
>         self.session.session = None # old SSHSession didn't have this attribute
>         self.session.dataReceived('1')
>         self.session.extReceived(connection.EXTENDED_DATA_STDERR, '2')
>         self.assertFalse(self.session.requestReceived(
>                 'subsystem', common.NS('BadSubsystem')))
>         self.assertIdentical(self.session.client, None)

Please add a comment explaining why you have this assertion.

>         self.assertTrue(self.session.requestReceived(
>                 'subsystem', common.NS('TestSubsystem') + 'data'))
>         self.assertIsInstance(self.session.client, protocol.ProcessProtocol)
>         self.assertIdentical(self.session.sessionApplication.processProtocol,
>                              self.session.avatar.subsystem)
> 
>         self.assertEquals(self.session.conn.data[self.session],
>                 ['\x00\x00\x00\x0dTestSubsystemdata~'])
>         self.session.dataReceived('more data')
>         self.assertEquals(self.session.conn.data[self.session][-1],
>                 'more data~')
>         self.session.closeReceived()
>         self.assertTrue(self.session.conn.closes[self.session])
>

This test is too big. Please split it up.

> 
>     def test_lookupSubsystemProtocol(self):
>         """
>         Test that lookupSubsystem correctly handles being returned a
>         Protocol.
>         """

Please define "correctly" in the docstring.

>         self.session.session = None # old SSHSession didn't have this attribute
>         self.session.dataReceived('1')
>         self.session.extReceived(connection.EXTENDED_DATA_STDERR, '2')
>         self.assertIdentical(self.session.client, None)
>         self.assertTrue(self.session.requestReceived(
>                 'subsystem', common.NS('protocol') + 'data'))
>         self.assertIsInstance(self.session.client,
>                               protocol.ProcessProtocol)
>         self.assertIdentical(
>             self.session.sessionApplication.processProtocol.proto,
>                              self.session.avatar.subsystem)
>         self.assertEquals(self.session.sessionApplication.processProtocol.connectionLost,
>                              self.session.sessionApplication.processProtocol.proto.connectionLost)
>         self.session.sessionApplication.processProtocol.foo = 'foo'
>         self.assertEquals(self.session.sessionApplication.processProtocol.proto.foo,
>                           'foo')
> 
>         self.assertIdentical(
>             self.session.sessionApplication.processProtocol.transport.proto,
>             self.session.sessionApplication.processProtocol)
> 
>         self.assertEquals(self.session.conn.data[self.session],
>                 ['\x00\x00\x00\x08protocoldata~'])
>         self.session.dataReceived('more data')
>         self.assertEquals(self.session.conn.data[self.session][-1],
>                 'more data~')
>         self.session.closeReceived()
>         self.assertTrue(self.session.conn.closes[self.session])
>         self.session.closed()
>         self.assertEquals(self.session.sessionApplication.processProtocol.proto.reason,
>                           protocol.connectionDone)
>

Please split this test up.

> 
>     def test_lookupSubsystemDoesNotNeedISession(self):
>         """
>         Previously, if one only wanted to implement a subsystem, an
>         ISession adapter wasn't needed.
>         """
>         s = session.SSHSession(avatar=SubsystemOnlyAvatar(),
>                                conn=StubConnection())
>         self.assertTrue(s.request_subsystem(common.NS('subsystem') + 'data'))
>         self.assertNotIdentical(s.applicationFactory, None)
>         self.assertIdentical(s.conn.closes.get(s), None)
>         s.eofReceived()
>         self.assertTrue(s.conn.closes.get(s))
>         # these should not raise errors
>         s.closeReceived()
>         s.closed()
>

Testing negatives is always tricky. I think you could explain better exactly how you *aren't* using ISession here, and highlight the bits that you think should blow up.

681 lines to go. Time for breakfast.

> 
>     def test_requestShell(self):
>         """
>         When a client requests a shell, the SSHSession object should get
>         the shell by getting an ISession adapter for the avatar, then
>         calling openShell() with a ProcessProtocol to attach.
>         """
>         # gets a shell the first time
>         self.assertTrue(self.session.requestReceived('shell', ''))
>         self.assertIsInstance(self.session.session,
>                               StubSessionForStubOldAvatar)
>         self.assertIsInstance(self.session.client,
>                               session.SSHSessionProcessProtocol)
>         self.assertIdentical(self.session.session.shellProtocol,
>                 self.session.client)
>         # doesn't get a shell the second time
>         self.assertFalse(self.session.requestReceived('shell', ''))
>         errors = self.flushLoggedErrors()

You should specify the type of error that you are flushing here, so that you don't accidentally flush a real error.

>         self.assertEquals(len(errors), 1)
>         errors[0].trap(RuntimeError)
>

Part of the difficulty in reviewing this is that there are so many assertions in these tests, which means they are testing many things.

If you can think of one, please extract custom assert* methods that can express your intent at a higher level.

> 
>     def test_requestShellWithData(self):
>         """
>         When a client executes a shell, it should be able to give pass data back
>         and forth.
>         """

"back and forth" is usually followed by "between X and Y". Please say where the data is going to and coming from.

>         self.assertTrue(self.session.requestReceived('shell', ''))
>         self.assertIsInstance(self.session.session,
>                               StubSessionForStubOldAvatar)
>         self.session.dataReceived('some data\x00')
>         self.assertEquals(self.session.session.shellTransport.data,
>                           'some data\x00')
>         self.assertEquals(self.session.conn.data[self.session],
>                           ['some data\x00', '\r\n'])
>         self.assertTrue(self.session.session.shellTransport.closed)
>         self.assertEquals(self.session.conn.requests[self.session],
>                           [('exit-status', '\x00\x00\x00\x00', False)])
> 
> 
>     def test_requestExec(self):
>         """
>         When a client requests a command, the SSHSession object should get
>         the command by getting an ISession adapter for the avatar, then
>         calling execCommand with a ProcessProtocol to attach and the
>         command line.
>         """
>         self.assertFalse(self.session.requestReceived('exec', common.NS('false')))
>         errors = self.flushLoggedErrors()

errors = self.flushLoggedErrors(RuntimeError)

>         self.assertEquals(len(errors), 1)
>         errors[0].trap(RuntimeError)
>         self.assertIdentical(self.session.client, None)
> 
>         self.assertTrue(self.session.requestReceived('exec', common.NS('true')))
>         self.assertIsInstance(self.session.session,
>                               StubSessionForStubOldAvatar)
>         self.assertIsInstance(self.session.client,
>                               session.SSHSessionProcessProtocol)
>         self.assertIdentical(self.session.session.execProtocol,
>                 self.session.client)
>         self.assertEquals(self.session.session.execCommandLine,
>                 'true')
>

Uhhh, that doesn't *really* execute 'true' on the command line, does it?

If it does, please replace it with something that will work on all platforms (e.g. '%s -c pass' % sys.executable). If it doesn't please replace it with something obviously bogus.

> 
>     def test_requestExecWithData(self):
>         """
>         When a client executes a command, it should be able to give pass data back
>         and forth.  Setting the remoteWindowLeft to 4 tests that the C{SSHChannel}
>         buf instance variable isn't overriden by SSHSession.
>         """

Why is 4 special? What is it about setting remoteWindowLeft that affects SSHChannel.buf?

>         self.session.remoteWindowLeft = 4
>         self.assertTrue(self.session.requestReceived('exec', common.NS('echo hello')))
>         self.assertIsInstance(self.session.session,
>                               StubSessionForStubOldAvatar)
>         self.session.dataReceived('some data')
>         self.assertEquals(self.session.session.execTransport.data, 'some data')
>         self.session.addWindowBytes(20)

Why 20?

You know, it's entirely OK to do something like:

arbitraryInteger = 20 self.session.addWindowBytes(arbitraryInteger)

>         self.assertEquals(self.session.conn.data[self.session],
>                           ['hell', 'osome data\r\n'])
>         self.session.eofReceived()
>         self.session.closeReceived()
>         self.session.closed()
>         self.assertTrue(self.session.session.execTransport.closed)
>         self.assertEquals(self.session.conn.requests[self.session],
>                           [('exit-status', '\x00\x00\x00\x00', False)])
> 
> 
>     def test_requestPty(self):
>         """
>         When a client requests a PTY, the SSHSession object should make
>         the request by getting an ISession adapter for the avatar, then
>         calling getPty with the terminal type, the window size, and any modes
>         the client gave us.
>         """
>         self.assertFalse(
>                 self.session.requestReceived('pty_req',
>             session.packRequest_pty_req('bad', (1, 2, 3, 4), [])))
>         self.assertIsInstance(self.session.session,
>                               StubSessionForStubOldAvatar)
>         errors = self.flushLoggedErrors()

errors = self.flushLoggedErrors(RuntimeError)

>         self.assertEquals(len(errors), 1)
>         errors[0].trap(RuntimeError)

This is begging for a custom assertion. It's perfectly OK to add one here, and then later we can move it into trial.

>         self.assertTrue(self.session.requestReceived('pty_req',
>             session.packRequest_pty_req('good', (1, 2, 3, 4), [])))
>         self.assertEquals(self.session.session.ptyRequest,
>                 ('good', (1, 2, 3, 4), []))
>

I can't remember, do 'good' and 'bad' activate specific behaviours on the stub objects? If they do, add a comment saying so.

> 
>     def test_requestWindowChange(self):
>         """
>         When the client requests to change the window size, the SSHSession
>         object should make the request by getting an ISession adapter for the
>         avatar, then calling windowChanged with the new window size.
>         """
>         self.assertFalse(self.session.requestReceived('window_change',
>             session.packRequest_window_change((0, 0, 0, 0))))
>         errors = self.flushLoggedErrors()
>         self.assertEquals(len(errors), 1)
>         errors[0].trap(RuntimeError)
>         self.assertIsInstance(self.session.session,
>                               StubSessionForStubOldAvatar)

You do this a lot. I'm guessing it's because requestReceived should set session. Why not test that in one place and then remove the assertion from all your other tests?

>         self.assertTrue(self.session.requestReceived('window_change',
>             session.packRequest_window_change((1, 2, 3, 4))))
>         self.assertEquals(self.session.session.windowChange,
>                 (1, 2, 3, 4))
> 
> 
>     def test_eofReceived(self):
>         """
>         When an EOF is received and a ISession adapter is present, it should
>         be notified of the EOF message.
>         """
>         self.session.session = session.ISession(self.session.avatar)
>         self.session.eofReceived()
>         self.assertTrue(self.session.session.gotEOF)
> 
> 
>     def test_closeReceived(self):
>         """
>         When a close is received, the session should send a close message.
>         """
>         self.assertIdentical(self.session.closeReceived(), None)
>         self.assertTrue(self.session.conn.closes[self.session])
> 
> 
>     def test_closed(self):
>         """
>         When a close is received and a ISession adapter is present, it should
>         be notified of the close message.
>         """
>         self.session.session = session.ISession(self.session.avatar)
>         self.session.closed()
>         self.assertTrue(self.session.session.gotClosed)
> 
> 
> 
> class TestHelpers(unittest.TestCase):
>     """
>     Tests for the 4 helper functions: parseRequest_* and packRequest_*.
>     """
> 
> 
>     def test_parseRequest_pty_req(self):
>         """
>         The payload of a pty-req message is::
>             string  terminal
>             uint32  columns
>             uint32  rows
>             uint32  x pixels
>             uint32  y pixels
>             string  modes
> 
>         Modes are::
>             byte    mode number
>             uint32  mode value
>         """
>         self.assertEquals(session.parseRequest_pty_req(common.NS('xterm') +
>                 '\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00\x00\x03\x00\x00\x00'
>                 '\x04\x00\x00\x00\x05\x05\x00\x00\x00\x06'),
>                 ('xterm', (2, 1, 3, 4), [(5, 6)]))
>

If you are happy to use NS to convert 'xterm', you should be OK with using a similar helper to convert the Python integers into their wire format. Please do this.

Alternatively, format the byte string differently:

request = (common.NS('xterm') +
           '\x00\x00\x00\x01' +
           '\x00\x00\x00\x02' +
           '\x00\x00\x00\x03' +
           '\x00\x00\x00\x04' +
           '\x00\x00\x00\x05' +
           '\x00\x00\x00\x06')
self.assertEquals(session.parseRequest_pty_req(request),
                  ('xterm', (2, 1, 3, 4), [(5, 6)]))

> 
>     def test_packRequest_pty_req(self):
>         """
>         See test_parseRequest_pty_req for the payload format.
>         """
>         self.assertEquals(self.assertWarns(DeprecationWarning,
>             "packRequest_pty_req should be packing the modes.",
>             unittest.__file__, session.packRequest_pty_req, 'xterm',
>             (2, 1, 3, 4), '\x05\x00\x00\x00\x06'),
>             common.NS('xterm') + '\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00'
>             '\x00\x03\x00\x00\x00\x04\x00\x00\x00\x05\x05\x00\x00\x00\x06')
>

There's no reason to nest these calls so much. Local variables are as cheap as chips.

Use TestCase.callDeprecated instead of assertWarns.

> 
>     def test_parseRequest_window_change(self):
>         """
>         The payload of a window_change request is::
>             uint32  columns
>             uint32  rows
>             uint32  x pixels
>             uint32  y pixels
>         """
>         self.assertEquals(session.parseRequest_window_change('\x00\x00\x00\x01'
>             '\x00\x00\x00\x02\x00\x00\x00\x03\x00\x00\x00\x04'), (2, 1, 3, 4))
>

Same comments as for the first test.

> 
>     def test_packRequest_window_change(self):
>         """
>         See test_parseRequest_window_change for the payload format.
>         """
>         self.assertEquals(session.packRequest_window_change((2, 1, 3, 4)),
>             '\x00\x00\x00\x01\x00\x00\x00\x02\x00\x00\x00\x03\x00\x00\x00\x04')
>

Same comments as for the first test.

> 
> 
> class SSHSessionProcessProtocolTestCase(unittest.TestCase):
>     """
>     SSHSessionProcessProtocol is an obsolete class used as a ProcessProtocol
>     for executed programs.  It has has a couple transport methods, namely
>     write() and loseConnection()
>     """
>

'couple of transport methods'.

> 
>     def setUp(self):
>         self.session = session.SSHSession(conn=StubConnection(),
>                 remoteWindow=500, remoteMaxPacket=100)
>         self.transport = StubTransport()
>         self.pp = session.SSHSessionProcessProtocol(self.session)
>         self.pp.makeConnection(self.transport)
> 
> 
>     def test_init(self):
>         """
>         SSHSessionProcessProtocol should set self.session to the session passed
>         to the __init__ method.
>         """
>         self.assertEquals(self.pp.session, self.session)
> 
> 
>     def test_getSignalName(self):
>         """
>         _getSignalName should return the name of a signal when given the
>         signal number.  Also, it should only generate the signal dictionary
>         once.
>         """
>         signal.SIGTwistedTest = signal.NSIG + 1 # value can't exist normally
>         # Force reinitialization of signals
>         self.pp.__class__._signalNames = None
>         try:
>             for signame in session.supportedSignals:
>                 signame = 'SIG' + signame
>                 value = getattr(signal, signame)
>                 self.assertEquals(self.pp._getSignalName(value),
>                         signame, "%i: %s != %s" % (value,
>                             self.pp._getSignalName(value), signame))
>             self.assertEquals(self.pp._getSignalName(signal.SIGTwistedTest),
>                 'SIGTwistedTest@' + sys.platform)
>             newPP = session.SSHSessionProcessProtocol(self.session)
>             self.assertNotIdentical(newPP._signalNames, None)
>             self.assertIdentical(newPP._signalNames, self.pp._signalNames)

This tests three things, and should be three tests.

>         finally:
>             del signal.SIGTwistedTest
>             # Force reinitialization of signals
>             self.pp.__class__._signalNames = None
>

See, that just goes to show that storing it on the class is a bad idea.

>     if getattr(signal, 'SIGALRM', None) is None:
>         test_getSignalName.skip = "Not all signals available"
> 
> 
>     def test_outReceived(self):
>         """
>         When data is passed to the outReceived method, it should be sent to
>         the session's write method.
>         """
>         self.pp.outReceived('test data')
>         self.assertEquals(self.session.conn.data[self.session],
>                 ['test data'])
> 
> 
>     def test_write(self):
>         """
>         When data is passed to the write method, it should be sent to the
>         session channel's write method.
>         """
>         self.pp.write('test data')
>         self.assertEquals(self.session.conn.data[self.session],
>                 ['test data'])
> 
> 
>     def test_errReceived(self):
>         """
>         When data is passed to the errReceived method, it should be sent to
>         the session's writeExtended method.
>         """
>         self.pp.errReceived('test data')
>         self.assertEquals(self.session.conn.extData[self.session],
>                 [(1, 'test data')])
> 
> 
>     def test_inConnectionLost(self):
>         """
>         When inConnectionLost is called, it should send an EOF message,
>         """
>         self.pp.inConnectionLost()
>         self.assertTrue(self.session.conn.eofs[self.session])
> 
> 
>     def test_outConnectionLost(self):
>         """
>         When outConnectionLost is called and there is no ISession adapter,
>         the connection should be closed,
>         """
>         self.pp.outConnectionLost()
>         self.assertTrue(self.session.conn.closes[self.session])
> 
> 
>     def test_loseConnection(self):
>         """
>         When loseConnection() is called, it should call loseConnection
>         on the session channel.
>         """
>         self.pp.loseConnection()
>         self.assertTrue(self.session.conn.closes[self.session])
> 
> 
>     def test_processEndedWithExitCode(self):
>         """
>         When processEnded is called, if there is an exit code in the reason
>         it should be sent in an exit-status method.  The connection should be
>         closed.
>         """
>         self.pp.processEnded(failure.Failure(error.ProcessDone(None)))
>         self.assertEquals(self.session.conn.requests[self.session],
>                 [('exit-status', '\x00' * 4, False)])

Explain where the magic number comes from.

>         self.assertTrue(self.session.conn.closes[self.session])
> 
> 
>     def test_processEndedWithExitSignalCoreDump(self):
>         """
>         When processEnded is called, if there is an exit signal in the reason
>         it should be sent in an exit-signal message.  The connection should be
>         closed.
>         """
>         self.pp.processEnded(failure.Failure(error.ProcessTerminated(None,
>             signal.SIGTERM, 128)))
>         self.assertEqual(self.session.conn.requests[self.session],
>                 [('exit-signal', common.NS('TERM') + '\x01' + common.NS('') +
>                     common.NS(''), False)])

Two magic numbers here. Please explain where they come from.

(Hint: coreDumped = '\x01' would go a long way)

>         self.assertTrue(self.session.conn.closes[self.session])
> 
> 
>     def test_processEndedWithExitSignalNoCoreDump(self):
>         """
>         When processEnded is called, if there is an exit signal in the
>         reason it should be sent in an exit-signal message.  If no
>         core was dumped, don't set the core-dump bit.
>         """
>         self.pp.processEnded(
>             failure.Failure(error.ProcessTerminated(None, signal.SIGTERM, 0)))
>         self.assertEqual(self.session.conn.requests[self.session],
>                          [('exit-signal', common.NS('TERM') + '\x00' +
>                            common.NS('') + common.NS(''), False)])
>         self.assertTrue(self.session.conn.closes[self.session])
>

Magic number again.

>     if not hasattr(os, 'WCOREDUMP'):
>         skipMsg = "can't run this w/o os.WCOREDUMP"
>         test_processEndedWithExitSignalCoreDump.skip = skipMsg
>         test_processEndedWithExitSignalNoCoreDump.skip = skipMsg
> 
> 
> 
> class SSHSessionProcessProtocolApplicationTestCase(unittest.TestCase):
>     """
>     SSHSessionProcessProtocolApplicationTestCase is an class used to
>     connect to a SSHSessionProcessProtocol as an application.
>     """
> 
> 
>     def test_dataReceived(self):
>         """
>         When data is received, it should be sent to the transport.
>         """
>         client = StubClient()
>         app = session.SSHSessionProcessProtocolApplication(client)
>         app.dataReceived('test data')
>         self.assertEquals(client.transport.buf, 'test data')
>

This is a beautiful test. More like this please.

> 
>     def test_extendedDataReceived(self):
>         """
>         When extended data of the type EXTENDED_DATA_STDERR is
>         received, it should be passed along to SSHSessionProcessProtocol's
>         transport.writeErr.
>         """
>         transport = StubTransport()
>         pp = MockProcessProtocol()
>         pp.makeConnection(transport)
>         app = session.SSHSessionProcessProtocolApplication(pp)
>         app.extendedDataReceived(255, "ignore this")

What does 255 mean?

What does this line test?

>         app.extendedDataReceived(connection.EXTENDED_DATA_STDERR, "data")
>         self.assertEquals(transport.err, "data")
>         # should not raise an error if writeErr doesn't exist
>         app.extendedDataReceived(connection.EXTENDED_DATA_STDERR, "more")
>

Huh what? You haven't done anything to writeErr. And if you did, it should go into another test.

Also, "should not raise an error" is a bad thing to test for. It's much easier and clearer to test for what a thing should do, rather than what it should not do.

> 
> 
> class SSHSessionClientTestCase(unittest.TestCase):
>     """
>     SSHSessionClient is an obsolete class used to connect standard IO to
>     an SSHSession.
>     """
> 
> 
>     def test_dataReceived(self):
>         """
>         When data is received, it should be sent to the transport.
>         """
>         client = session.SSHSessionClient()
>         client.transport = StubTransport()
>         client.dataReceived('test data')
>         self.assertEquals(client.transport.buf, 'test data')
> 
> 
> 
> class ServerSessionTestCase(unittest.TestCase):
>     """
>     Tests that verify server functionality of SSHSession.
>     """
> 
> 
>     def setUp(self):
>         self.conn = StubConnection()
>         self.avatar = StubNewAvatar()
>         self.session = session.SSHSession(remoteWindow=131072,
>                 remoteMaxPacket=32768, conn=self.conn,
>                 avatar=self.avatar)
> 
> 
>     def test_init(self):
>         """
>         Test that the session is initialized correctly,
>         """

'Correctly' is a weasel word in tests. Of course you are testing that something happens correctly. Tell us what 'correctly' means.

>         self.assertEquals(self.session.client, None)
>         self.assertTrue(session.ISessionApplicationFactory.providedBy(
>                 self.session.applicationFactory),
>                         str(self.session.applicationFactory))
> 
> 
>     def test_client(self):
>         """
>         Test that the session performs correctly when it has a client
>         application.
>         """

This docstring is unclear. "correctly", as mentioned before, adds no meaning.

Looking at the test below, it would be easier to write a docstring if you were testing fewer things.

>         client = MockApplication()
>         self.session.dataReceived('1')
>         self.session.extReceived(connection.EXTENDED_DATA_STDERR, '2')
>         self.session.extReceived(2, '3')
>         self.session.setupSession(client)
>         self.session.dataReceived('out')
>         self.assertEquals(client.data, ['1', 'out'])
>         self.assertEquals(self.session.conn.data[self.session],
>                           ['1~', 'out~'])
>         self.session.extReceived(connection.EXTENDED_DATA_STDERR, 'err')
>         self.assertEquals(client.extendedData,
>                           [(connection.EXTENDED_DATA_STDERR, '2'),
>                            (2, '3'),
>                            (connection.EXTENDED_DATA_STDERR, 'err')])
>         self.assertEquals(self.session.conn.extData[self.session],
>                           [(connection.EXTENDED_DATA_STDERR + 1, '2~'),
>                            (2 + 1, '3~'),
>                            (connection.EXTENDED_DATA_STDERR + 1, 'err~')])
>         self.session.eofReceived()
>         self.assertTrue(client.gotEOF)
>         self.assertFalse(client.hasClosed, 'closed() called during EOF')
>         self.session.closeReceived()
>         self.assertTrue(client.gotClose)
>         self.assertFalse(client.hasClosed,
>                          'closed() called during closeReceived')
>         self.session.closed()
>         self.assertTrue(client.hasClosed, 'closed() not called')
>

This test tests so many things. Please split it up as much as possible.

> 
>     def test_applicationFactory(self):
>         """
>         Test that SSHSession handles dispatching to applicationFactory correctly.
>         """
>         self.session.eofReceived()
>         self.assertFalse(self.session.applicationFactory.inConnectionOpen)
>         self.session.closeReceived()
>         self.assertFalse(self.session.applicationFactory.outConnectionOpen)
>         self.session.closed()
>         self.assertTrue(self.session.applicationFactory.ended)
>

What does correctly mean?

I think this test would be clearer if you used the more traditional 'call logging' style of mock object, rather than one with many custom booleans.

> 
>     def test_subsystem(self):
>         """
>         Test that SSHSession handles subsystem requests by dispatching to the
>         application factory's requestSubsytem() method and connecting the
>         ISessionApplication to itself.
>         """

'itself' is ambiguous here. I *think* you mean "the session", but you could mean "the session application".

>         ret = self.session.requestReceived('subsystem', common.NS('bad'))
>         self.assertFalse(ret)
>         self.assertEquals(self.session.client, None)
>         ret = self.session.requestReceived('subsystem',
>                 common.NS('echo') + 'abc')

The 'echo' subsystem was defined some few hundred lines back. It's worth reminding the reader that it's there.

>         self.assertTrue(ret)
>         self.assertTrue(session.ISessionApplication.providedBy(
>                 self.session.sessionApplication))

I really dislike the way this combines an assertion with code that exercises the system-under-test. It makes it harder to understand the expected behaviour. You do it a lot in this file, and I'd really appreciate it if you could change the tests to not do it.

>         self.assertIdentical(self.session.sessionApplication,
>                 self.session.applicationFactory.subsystem)
>         self.assertFalse(self.session.sessionApplication.hasClosed)
>         self.assertEquals(self.session.sessionApplication.packetData, 'abc')
>         self.assertEquals(self.session.sessionApplication.channel,
>                 self.session)
> 
> 
>     def test_shell(self):
>         """
>         Test that SSHSession handles shell requests by dispatching to the
>         application factory's openShell() method and connecting the
>         ISessionApplication to itself.
>         """

Same comments on 'itself'.

>         self.assertTrue(self.session.requestReceived('shell', ''))
>         self.assertNotEquals(self.session.sessionApplication, None)
>         self.assertIdentical(self.session.sessionApplication,
>                 self.session.applicationFactory.shell)
>         self.assertEquals(self.session.sessionApplication.channel,
>                 self.session)
>         self.assertFalse(self.session.sessionApplication.hasClosed)
>         self.assertFalse(self.session.requestReceived('shell', ''))
>         # fail if we have a shell already
>         errors = self.flushLoggedErrors()

errors = self.flushLoggedErrors(RuntimeError)

>         self.assertEquals(len(errors), 1)
>         errors[0].trap(RuntimeError)
> 
> 
>     def test_exec(self):
>         """
>         Test that SSHSession handles command requests by dispatching to the
>         the application factory's execCommand method and connecting the
>         ISessionApplication to itself.
>         """

Ditto 'itself'.

>         self.assertTrue(self.session.requestReceived('exec',
>             common.NS('good')))
>         self.assertNotEquals(self.session.sessionApplication, None)
>         self.assertIdentical(self.session.sessionApplication,
>                              self.session.applicationFactory.command)
>         self.assertEquals(self.session.sessionApplication.channel,
>                 self.session)
>         self.assertEquals(self.session.sessionApplication.command, 'good')
>         self.assertFalse(self.session.sessionApplication.hasClosed)
>         self.assertFalse(self.session.requestReceived('exec',
>             common.NS('bad')))
>         errors = self.flushLoggedErrors()

errors = self.flushLoggedErrors(RuntimeError)

>         self.assertEquals(len(errors), 1)
>         errors[0].trap(RuntimeError)
> 
> 
>     def test_ptyRequest(self):
>         """
>         Test that SSHSession handles PTY requests by dispatching to the
>         application factory's getPTY method.
>         """
>         term = 'conch'
>         windowSize = (80, 25, 0, 0)
>         modes = [(0, 1), (2, 3)]
>         ret = self.session.requestReceived('pty_req',
>                 common.NS(term) +
>                 '\x00\x00\x00\x19' + '\x00\x00\x00\x50' +
>                 '\x00\x00\x00\x00' * 2 + common.NS('\x00\x00\x00\x00\x01'
>                     '\x02\x00\x00\x00\x03'))

Formatting this with many more line breaks would make it a lot clearer.

Also, if you really cared about the reader, you'd make it obvious that '25' is inextricably bound up with '\x00\x00\x00\x19'.

>         self.assertTrue(ret)
>         self.assertEquals(self.session.applicationFactory.term, term)
>         self.assertEquals(self.session.applicationFactory.windowSize, windowSize)

Line too long, please wrap.

>         self.assertEquals(self.session.applicationFactory.modes, modes)
>         self.assertFalse(self.session.requestReceived('pty_req', ''))
>         errors = self.flushLoggedErrors()

errors = self.flushLoggedErrors(RuntimeError)

>         self.assertEquals(len(errors), 1)
>         errors[0].trap(struct.error)
> 
> 
>     def test_windowChange(self):
>         """
>         Test that SSHSession handles window size change requests by dispatching
>         to the application factory's windowChanged method.
>         """
>         windowSize = (1, 1, 1, 1)
>         ret = self.session.requestReceived('window_change',
>                 '\x00\x00\x00\x01' * 4)
>         self.assertTrue(ret)
>         self.assertEquals(self.session.applicationFactory.windowSize, windowSize)
>         self.assertFalse(self.session.requestReceived('window_change', ''))
>         errors = self.flushLoggedErrors()

errors = self.flushLoggedErrors(RuntimeError)

>         self.assertEquals(len(errors), 1)
>         errors[0].trap(struct.error)
> 
> 
> 
> class ClientSessionTestCase(unittest.TestCase):
>     """
>     Test methods that use SSHSession as a client.
>     """
> 
> 
>     def setUp(self):
>         self.conn = StubConnection()
>         self.session = session.SSHSession(remoteWindow=131072,
>                 remoteMaxPacket=32768, conn=self.conn)
> 
> 
>     def test_getPty(self):
>         """
>         Test that getPty sends the correct request.
>         """
>         d = self.session.getPty('term', (80, 25, 0, 0), [(0, 1), (2, 3)],
>                 True)
>         def check(value):
>             self.assertEquals(self.conn.requests[self.session],
>                     [('pty-req', common.NS('term') + '\x00\x00\x00\x19' +
>                         '\x00\x00\x00\x50' + '\x00\x00\x00\x00' * 2 +
>                         common.NS('\x00\x00\x00\x00\x01\x02\x00\x00\x00\x03'),
>                         True)])

Formatting this with many more line breaks would make it a lot clearer.

Also, if you really cared about the reader, you'd make it obvious that '25' is inextricably bound up with '\x00\x00\x00\x19'.

>             self.assertEquals(self.session.getPty('term', (80, 25, 0, 0), []),
>                     None)
>         d.addCallback(check)
>         return d
> 
> 
>     def test_changeWindowSize(self):
>         """
>         Test that windowChange sends the correct request.
>         """
>         d = self.session.changeWindowSize((80, 25, 0, 0), True)
>         def check(value):
>             self.assertEquals(self.conn.requests[self.session],
>                     [('window-change', '\x00\x00\x00\x19\x00\x00\x00\x50' +
>                         '\x00\x00\x00\x00' * 2, True)])

Newlines and hex.

>             self.assertEquals(self.session.changeWindowSize((80, 25, 0, 0)),
>                     None)
>         d.addCallback(check)
>         return d
> 
> 
>     def test_openSubsystem(self):
>         """
>         Test that openSubsystem sends the correct request.
>         """

What's correct? I don't know.

>         d = self.session.openSubsystem('echo', 'data', True)
>         def check(value):
>             self.assertEquals(self.conn.requests[self.session],
>                     [('subsystem', common.NS('echo') + 'data', True)])
>             self.assertEquals(self.session.openSubsystem('echo', 'data'),
>                     None)
>         d.addCallback(check)
>         return d
> 
> 
>     def test_openShell(self):
>         """
>         Test that openShell sends the correct request.
>         """

What's correct? I don't know.

>         d = self.session.openShell(True)
>         def check(value):
>             self.assertEquals(self.conn.requests[self.session],
>                     [('shell', '', True)])
>             self.assertEquals(self.session.openShell(), None)
>         d.addCallback(check)
>         return d
> 
> 
>     def test_execCommand(self):
>         """
>         Test that execCommand sentds the correct request.
>         """

What's correct? I don't know.

Also, 'sends' not 'sentds'.

>         d = self.session.execCommand('true', True)
>         def check(value):
>             self.assertEquals(self.conn.requests[self.session],
>                     [('exec', common.NS('true'), True)])
>             self.assertEquals(self.session.execCommand('true'), None)
>         d.addCallback(check)
>         return d

OK, that was epic.

It'd be great if you could send a diff containing the changes that you made in response to this review.

Thanks again, jml

[jml]

jml's review, attached to make replying easier

[therve]

I've looked at it, but I'm basically unable to address most points. FWIW, one problem is that SSHSession is currently an old style class, so the properties actually don't work. You have to make it new style first.

[z3p]

FWIW, the properties actually do work like I want them to. Each one sets self.[attribute name] so it doesn't need a getter. I should add a comment to that effect.

[therve]

At least, the applicationFactory property is not tested.

[z3p]

(In [23375]) checkpointing after the sprint

Refs #2710

[z3p]

I think this branch is ready for review again.

jml: Some comments on your review (thanks for the v. thorough review, BTW) are attached, as is the diff of the changes I made to address your comments.

[z3p]

comments to jml's review

[z3p]

changes made to this branch to address jml's review

[therve]

I just note 2 problems I've spotted with the branch merged to trunk:

• the conch client prints a big traceback when at logout:

  File "/home/therve/Projets/Twisted/trunk/twisted/conch/client/direct.py", line 64, in connectionLost
  d.addCallback(lambda x:
  exceptions.AttributeError: 'NoneType' object has no attribute 'addCallback'
  

• the sftp server run with 'twistd conch' doesn't work

[exarkun]

• avatar.py
  • still no module docstring (mentioned in earlier review)
• interfaces.py
  • still some trailing whitespace (in lookupSubsystem)
  • also no module docstring
  • several ISessionApplicationFactory methods document that an exception may be raised, but not what exception
  • several method docstrings also misspell implements
  • the type of newWindowSize in ISessionApplicationFactory.windowChanged is unspecified
  • how does ISessionApplication.channel get set? (Ah, I see, it gets jammed on externally.) Note that connectionMade isn't part of IProtocol - IProtocol.makeConnection(transport) is the interface.
• twisted/conch/ssh/session.py
  • some trailing whitespace
  • SSHSession.applicationFactory is documented with an I{} that should be an L{}, I think.
  • Some XXX left in SSHSession.__setattr__.
  • log.err() in request_subsystem could use a _why argument
  • ditto for request_shell, request_exec, request_pty_req (mentioned in earlier review)
  • Are there tests for special handling of KeyboardInterrupt?

Getting closer, I think. The backwards compatibility code is making a lot more sense to me now, in particular. I'd suggest going over the earlier reviews and making sure you addressed all the points from them.

[thijs]

Adding patch that removes the remaining relative imports

[therve]

Same #2682, the review is not on a subpatch.

[z3p]

(In [24601]) Branching to 'session-2710-6'

[z3p]

Ready for review in session-2710-6.

[therve]

Build results are here:  http://buildbot.twistedmatrix.com/boxes-supported?branch=branches/session-2710-6

[exarkun]

There seems to be some problem with the test suite.

[z3p]

(In [25053]) Branching to 'session-2710-7'

[z3p]

Warnings being printed in the ManholeLoopbackSSH tests are causing an infinite recursion:

exarkun: It calls the deprecated function exarkun: A warning is emitted exarkun: It gets turned into a log event exarkun: trial's log observer turns it into a print exarkun: The test is for code that overrides sys.stdout, so the print goes to the override exarkun: The override tries to write it to the connection exarkun: It calls the deprecated function exarkun: etc

The changes in #3487 (flushWarnings) seem to resolve the hang.

[z3p]

I believe that this branch is ready for review.

[therve]

Erm, sorry to say that, but there is a conflict with trunk, and several tests are failing, mainly in OldSessionInterfaceTestCase and OldSessionWithNoAvatarTestCase.

There are also several mentions of Twisted 8.2, which shoudl be replaced by 9.0.

Thanks, and sorry again for the review delay.

[z3p]

(In [26169]) Branching to 'session-2710-8'

[z3p]

What mentions of 8.2? The only things I noticed were deprecations.

Otherwise, this is ready for review.

[therve]

Right, the deprecations are mentioning 8.2, but 8.2 is out, so it should be replaced by 9.0.

[z3p]

But that code has still been deprecated since 8.2; how else will we know what release we can remove the code in?

[therve]

No, it has not been deprecated since 8.2 if the branch hasn't been merged? I mean, the deprecations are new in this branch right? The deprecations should specify the version of the next release. It seems I may miss something or be confused, or you are.

[z3p]

Oh, right. I'm confused :) Fixed, and ready for review.

[z3p]

Forgot to mark this as 'review' before. Also, I think this fixes #3444.

[therve]

1) The tests still refer to 8.2 (thus failing).

2) _wrapWithAssertWarns (both of them), test_lookupSubsystemProtocol, should be on 80 columns.

3) assertRequestRaisedError checks that we got one error, but doesn't check the error raised.

4) Please update copyrights to 2009.

5) SSHSessionProcessProtocol.write and SSHSessionProcessProtocol.loseConnection still mention 8.2 in the deprecation message.

Thanks!

[z3p]

(In [26459]) 80 columns

Refs #2710

[z3p]

(In [26460]) Twisted 9.0

Refs #2710

[z3p]

(In [26461]) copyrights

Refs #2710

[z3p]

(In [26462]) more Twisted 9.0

Refs #2710

[z3p]

(In [26463]) more Twisted 9.0

Refs #2710

[z3p]

Re #3: We don't care what the exception is; we just care that it's logged. That's why the test just checks for the number of exceptions and not their type.

I've addressed the other comments; I believe it's ready for review again.

[glyph]

In advance of this review, I apologize if I'm retreading any old ground here. The conversation on this ticket is so massive that it borders on incomprehensibility.

I also apologize that I haven't separated this into sections ("must address", "address at your own discretion", "file a separate ticket"), so please feel free to do so yourself. Test-coverage and compatibility issues are the most important (obviously).

If I mention something that's been addressed in a previous review discussion, you are free to ignore it (although I'd appreciate an indication of its prior addressed-ness).

1. These are untested, according to trial --coverage twisted.conch.test.test_session (trying to restrict myself to new code here):
   1. The RuntimeError raised from twisted.conch.session.SSHSession.applicationFactory
   2. request_subsystem's logged error in its except clause. _why is intended to be a private name for that argument, by the way, to encourage it to be passed positionally: I'd prefer log.err(Failure(), "...") as a calling convention.
   3. Both outReceived and processEnded on _ProtocolToProcessProtocol
2. I tried the example script from #3444. Turns out it relied on session.wrapProtocol, which was removed without deprecation. So please put that back.
   1. When I restored that (and _DummyTransport), I still got "short" as the output from the example. So I don't think it fixes the problem. On the other hand, the example is very difficult to read. I think that you should probably make a separate branch for that.
3. In epytext, C{} is "code", but L{} is link. You want to be using L{} most of the time; it provides users with a working link to the documentation for the code you're referring to. C{} just makes the text appear monospaced.
4. Pydoctor understands @since, so you should use "@since: 9.0" instead of "This class was added in Twisted 9.0".
5. twisted.conch.ssh.session._ProtocolToProcessProtocol:
   1. I don't really like the __getattr__ / __setattr__ magic. What uses depend on it implementing IProtocol? If this usage is deprecated, I'd rather the methods be explicitly enumerated, and to emit deprecation warnings, since this sort of confused maybe-it's-a-protocol-maybe-it's-a-process-protocol problem that we really don't need more of.
   2. This has an XXX in its docstring. The actual implementation provided here shouldn't be in twisted.internet, since it is confused about whether it's a ProcessProtocol or a Protocol.
      1. I believe it wraps an IProtocol provider, not a Protocol instance. The docstring should say so.
      2. Similarly, __getattr__'s docstring mentions "the Protocol interface".
      3. If you can get rid of the aforementioned __getattr__ mess, the XXX should really be obeyed. If not, you should file a new ticket.
6. twisted.conch.ssh.session._TransportToProcessProtocol:
   1. As I mentioned above, L{} should be used for links, not I{} (italics). At first I was confused about what ITransport you were talking about, since ITransport doesn't include writeErr. If you wrote it as a link which pydoctor could resolve it would be clearer.
      1. writeErr should be more clearly documented as just for compatibility. There are presumably better ways to get this now.
   2. Don't use hasattr. It has the unfortunate side-effect of swallowing all exceptions and not reporting them, so it can be very confusing to debug it in conjunction with dynamic properties. getattr(x, 'y', None) is None has the same effect, and doesn't have this side-effect.
   3. There should be a ticket for this.
   4. This should be in twisted.internet.
      1. There should probably be a subclass here, of course, which does the writeErr stuff.
   5. It should be public.
   6. It should have its own tests.
7. twisted.conch.ssh.session.SSHSession.__setattr__.__doc__ might want to say something about old-style classes. It took me a minute to realize why you weren't just making a property.
8. twisted.conch.ssh.session.SSHSession.client:
   1. Shouldn't this emit a DeprecationWarning?
   2. The docstring for this function should really just refer to the @ivar.
9. I lost the battle on first-person docstrings. There's now an official recommendation in the coding standard (under "Docstrings") which suggests that, i.e. rather than:

   This used to be the C{ProcessProtocol} that was connected to us.
   

   you should say:

   Before Twisted 9.0, C{client} was the L{ProcessProtocol} that was connected to this L{SSHSession}.
   

   It's a bit more verbose, but also clearer and more consistent with Twisted's other documentation.
10. twisted.conch.ssh.session.SSHSession._setUpSession:  http://angryflower.com/bobsqu.gif
11. Given that so much of this code is compatibility code, it would be really helpful if you moved as much of it as possible to twisted.conch.ssh._session_compat or something. ISessionApplication and ISessionApplicationFactory are a big step forward, but it's very hard to read twisted.conch.ssh.session and get an idea for the most recent / public code.

Otherwise this is looking pretty good. It's come a long way.

[therve]

(In [26785]) Branching to 'session-2710-9'

[therve]

For the record, I've fixed from the previous review: 1, 3, 4, 9, 10.

[z3p]

2 addressed in r26884

[z3p]

Re 5: The issue with the getattr stuff is that subsystems that used Protocols (in Conch, the SFTP server is an example) were not wrapped before. The SFTP unittests expect that they can modify that object and have it affect the Protocol; it modifies connectionLost and assumes that works, hence the test for that in test_session.

Re 6: Is this really something that anyone else is going to use? It would seem that it'll just clutter t.i.

Re 8: The DeprecationWarning is emitted when this attribute is set.

Re 11: I tried this before with DeprecatedSSHSession (see above) and it ended up getting backed out. Are we sure that it's a good idea this time?

The other documentation-y things are addressed in r26886.

[therve]

OK, all points seem to be handled. Let's push that one again!

[exarkun]

I started to review this. Here's what I came up with before I changed my mind (what comes after is more immediately relevant):

1. Regarding _ProtocolToProcessProtocol, this is an oft requested feature. No need to move the code for this ticket, but please file a new ticket and link to it in the XXX in that class's docstring.
2. Regarding the __getattr__ and __setattr__ of _ProtocolToProcessProtocol, can you file a ticket for deprecating this proxying and link to it from one of these methods?
3. The other parts of point 5 from glyph's most recent review are unaddressed.
4. The docstring for twisted.conch.interfaces.ISessionApplicationFactory.execCommand talks about shells, which I think is a copy/paste error.
5. ISessionApplicationFactory.channel is confusing in the same way ISession.conn was confusing. Where does it come from? (The implementation sets it, I know). A parallel to IProtocol.makeConnection would be much easier to understand, I think. Or... Is that attribute really real? I don't see that it actually exists anywhere.
6. Several docstrings for ISessionApplicationFactory methods talk about IConchApplication, which does not exist.
7. They also talk about objects which implement that interface, but they probably mean provide.

There is too much going on in this branch:

1. Test coverage for previously untested code is being added
2. Old interfaces are changing
3. New interfaces are being introduced to replace old interfaces
4. Certain application behaviors are being deprecated (don't call this, do define that, etc)
5. Fixes for various bugs encountered during development of the branch are included

The reasons this troubles me are that:

1. It makes the reviews unreasonably hard to do well. Since reviews end up not being spectacular, more work ends up being generated for the developer (mainly Paul).
2. The last big conch branch introduced some regressions. I'd like not to repeat that.
3. Things like point 5 from the review above go unnoticed for two years.

Probably there are other reasons, but hopefully I have conveyed the basic idea.

So what to do from now? Maybe someone else will take up the review of this branch and it can be merged (they should stake something of value on the branch not introducing any serious regressions though, ssh is serious business!). Alternatively, the various areas of improvement represented in the current branch can be identified (as I've started to do above), new tickets can be filed for those areas, and the issues can be resolved independently with smaller branches. There can also be some opportunity for discussion about the merits or demerits of the solution in this branch on those tickets, an opportunity which was largely missed here because the initial ticket description didn't cover much of what has happened in this branch, and because by the time the branch was first up for review, it was so big that the reviewer was probably overwhelmed by what was going on and overlooked considering why it was going on.

I'll promise to offer time to help out with this, either in the form of pair programming or realtime discussion.

[therve]

+1 on splitting the work. I already started in #2687.

[jml]

I agree with both of you. Marking as wontfix -- hope that's ok.