plugin: sftp transport

[Martin Pool]

On 20/10/05, John A Meinel <john at arbash-meinel.com> wrote:
> >> I know there has been some discussion (and I believe it was  decided)
> >> about
> >> using Twisted, which also provides sftp support. I'm guessing what
> >> you've
> >> done can probably be adapted for it. But I know one reason I wasn't
> >> persuing it heavily, is that I was waiting for Twisted to be  integrated
> >> first.
> >
> > A large part of it could probably be salvaged if we switch to  Twisted,
> > but I won't hold my breath for that.  Twisted is an event- based
> > framework, so it would mean changing a lot of the flow of bzr  to suit
> > twisted.  (This is one reason I prefer synchronous code over
> > event-based: Simpler is better.)
>
> Simpler, but not without loss of functionality. And it looks like we
> need to go async for performance reasons when downloading lots of code,
> and to enable nice responsive guis to be built. I'm only about 60%
> convinced, but it seems to be a general consensus that it is the least
> evil thing. :)

I'm still only about 75% convinced myself.  If we can acheive our
performance and function goals without using twisted that would make
me more comfortable.  Being purely synchronous is probably not enough
but perhaps it can be confined to one part of the code.

> Yeah. I don't really have a solution. Something like Tom's latest work
> on a transactional remote system, where you just have some directories
> that keep getting renamed to indicate whether you are locked or not. I
> don't think we can change things such that
> >
> >> It would be nice to have a directory with an __init__.py that also
> >> has the
> >> test_suite(), so that running "bzr selftest" could also test the sftp
> >> plugin.
> >
> >
> > Yeah, I did that now.  I guess it was too optimistic to think I could
> > keep it down to one file. :)
>
> Actually, you could put everything into one file. The plugin loader just
> looks for a "test_suite" function in the module that it loaded. And you
> could also put the test in the same module.
>
> I personally think it is cleaner to break things out, so that you don't
> have to see all the test code when you are looking at the implementation.
>
> Also, if you have a standalone "test_sftptransport.py" file, that can
> simply be copied into bzrlib/selftest/ and __init__.py updated to look
> for it, rather than copying and pasting the test code into another file.
>
> In other words, it lets me write code as though it was part of the core
> of bzr, and just have an __init__.py which handles the fact that it is a
> plugin.
>
> >
> >>> Oh yeah, this is mostly just ported from my last version, except
> >>> using the new Transport API (which has been cleaned up a lot --
> >>> thanks).
> >>
> >>
> >> I'm not really sure how the interface was cleaned up, perhaps I  moved
> >> the
> >> LocalTransport calls into a separate class. Or maybe I just missed
> >> someone
> >> else's work.
> >
> >
> > I just meant compared to my last patch, which was before your  Transport
> > work got in.  Using the old "store" interface to implement  new
> > transports had a lot of redundancy.  This way is much better.
> >
> >> I've looked over the code (briefly), and generally it looks good.  I'm a
> >> little curious why you have the global SYSTEM_HOST_KEYS loaded into
> >> save_host_keys(), but you don't use it.
> >
> >
> > Good point, too much cut-n-paste. :)
> >
> >> Also, I would think that in _sftp_connect() we would preferentially  use
> >> "BZR_HOST_KEYS" over using SYSTEM_HOST_KEYS, since we are BZR, but  this
> >> probably isn't a big deal.
> >
> >
> > *shrug* Makes sense to me, so I swapped them.
> >
> >> I also noticed that you never disconnect from the remote host. Now, I
> >> don't know if the disconnect automatically happens when the last
> >> reference
> >> to self._sftp goes away, which is perfectly fine.
> >
> >
> > Yeah, it's using python __del__ magick so that SFTP file objects can  be
> > casually discarded just like python's built-in file objects.  The  same
> > applies up the chain to the (paramiko) Transport.
> >
> >> We might also consider having a weakref dictionary of remote hosts
> >> that we
> >> are currently connected to, and their connection object. I know that I
> >> believe everything will be called through "clone()" but I suppose  it
> >> would
> >> be possible for a second connection to be attempted. A weakref
> >> dictionary
> >> lets you use the connection if it exists, but it also doesn't hang  on to
> >> it (letting it disconnect) when all real references go away.

Or maybe just hang on to the indefinitely, or until the connection drops.

--
Martin