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

[Andrew Bennetts]

John Arbash Meinel wrote:
> Andrew Bennetts wrote:
[...]
> 
> I like the overall refactoring. I think you might be missing a couple
> steps, but nothing critical at this point.

Right.  I could improve this further, but there are other things I want to work
on, and this is a pretty good improvement already.

> v-- In general, all of you SSHVendor children have a 'connect_sftp'
> method which connects to SSH and then wraps it with a SFTPClient.
> 
> I think it would make sense to break out the SSH connection, and then
> you don't have to re-implement it in the future. However, these changes
> can be done at a different time, when you actually go to use ssh
> connections.

I agree -- both about what to do, and when to do it :)

[...]
> > +
> > +    def connect_ssh(self, username, password, host, port, command):
> > +        """Make an SSH connection, and return a pipe-like object."""
> > +        raise NotImplementedError(self.connect_ssh)
> > +        
> 
> ^-- I realize you haven't implemented this yet. but it seems you would
> need more than a 'pipe-like' object. Either you need 2 pipes for
> bidirectional support, or you need a socket-like object. Or maybe even a
> real class.

Right.  I really only included this to indicate where this will fit in with
later work -- I'd rather not get hung up on the details now, they're likely to
change anyway.

I've added this to the docstring to make this clear:

        (This is currently unused, it's just here to indicate future directions
        for this code.)

[...]
> ^- loopback is fairly unique, but I think it could be:
> 
> def connect_sftp(self, username, password, host, port):
>     return SFTPClient(LoopbackSFTP(self.connect_ssh()))
> 
> def connect_ssh(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 sock

Right.  But again, I'd rather not worry about connect_ssh until it's actually
used.

> > +class ParamikoVendor(SSHVendor):
> > +    """Vendor that uses paramiko."""
> > +
> > +    def connect_sftp(self, username, password, host, port):
[...]
> 
> Everything up to here is actually connecting to SSH, only the
> 'open_sftp_client()' step is creating a sftp connection.
> Though I realize 'paramiko.Transport' is probably not a 'pipe like
> object'. So probably you would need a helper function on the class that
> returns a connected paramiko.Transport, and go from there.

Right.  Again, I'd like to leave this until connect_ssh is implemented.

[...]
> > +def os_specific_subprocess_params():
[...]
> > +        return {'preexec_fn': _ignore_sigint,
> > +                'close_fds': True,
> > +                }
> > +
> 
> -- You need an extra space here

Done, thanks.

> > +class SSHSubprocess(object):
> > +    """A socket-like object that talks to an ssh subprocess via pipes."""
> > +    # TODO: this class probably belongs in bzrlib/transport/ssh.py
> > +
> > +    def __init__(self, proc):
> > +        self.proc = proc
> > +
> 
> ...
> 
> Otherwise I'm happy to see the proper things moved into ssh.py
> I'm a little concerned that a short name like 'ssh.py' might conflict
> with another module in the future. But only a little.

Yeah, me too, but it's no worse than 'sftp' really.  Besides, bzr supports
renames :)

-Andrew.