[MERGE] Refactor loopback/paramiko/openssh/ssh connection opening

[Martin Pool]

On 25 Aug 2006, Andrew Bennetts <andrew at canonical.com> wrote:
> Hi all,
> 
> This bundle gets rid of all the "if vendor == 'xxx'" statements scattered
> throughout bzrlib/transport/sftp.py, and instead moves all the connection
> establishing logic into a new file, bzrlib/transport/ssh.py.  Code that's
> purely about establishing an SSH connection has been moved out of sftp.py,
> leaving sftp.py with just SFTP logic.
> 
> In addition to the moved code, ssh.py defines a bunch of new classes inheriting
> from SSHVendor, which provides a consistent interface to establishing an SFTP
> connection, regardless of the method used to do this.  Soon it will also do a
> similar thing for opening pipes to a command run over SSH; the smart server will
> use this to run over SSH.
> 
> It's likely that I've missed adding some helpful docstrings, but overall I think
> this is much cleaner than what was there before.  I think some further tidying
> is possible (e.g. I think a bunch of the host key management code belongs in
> paramiko, and may in fact be there already), but nothing is worse than it was
> before, just moved :)

+1 for 0.11, but I have some suggestions for more cleanups.

> +class LoopbackVendor(SSHVendor):
> +    """SSH "vendor" that connects over a plain TCP socket, not SSH."""
> +    
> +    def connect_sftp(self, username, password, host, port):
> +        sock = socket.socket()
> +        try:
> +            sock.connect((host, port))
> +        except socket.error, e:
> +            raise ConnectionError('Unable to connect to SSH host %s:%s: %s'
> +                                  % (host, port, e))
> +        return SFTPClient(LoopbackSFTP(sock))
> +

I realize this is just moved code but seeing it here made me realize -
you probably want to try turning on TCP_NODELAY as we did in remote.py.
It made a big difference to test suite runtime there and probably will
here: just measure the walltime without and with.

> +def os_specific_subprocess_params():
> +    """Get O/S specific subprocess parameters."""

Could this move to an instance method of SubprocessVendor? 

It's also called from the code that runs 'ssh -V', but setting these
options is not really needed to get the version.

> +
> +_ssh_vendors = {}
> +
> +def register_ssh_vendor(name, vendor_class):
> +    """Register lazy-loaded SSH vendor class.""" 
> +    _ssh_vendors[name] = vendor_class
> +
> +register_ssh_vendor('loopback', ssh.LoopbackVendor)
> +register_ssh_vendor('paramiko', ssh.ParamikoVendor)
> +register_ssh_vendor('none', ssh.ParamikoVendor)
> +register_ssh_vendor('openssh', ssh.OpenSSHSubprocessVendor)
> +register_ssh_vendor('ssh', ssh.SSHCorpSubprocessVendor)
> +    

The registration and discovery of vendors should move to ssh.py too.

I wonder, would this be happier just having instances, since they're
stateless?  Not a big deal.

-- 
Martin