new SFTP implementation

[davidsarah]

This ticket is for review of my new SFTP implementation. It adds or improves the following features:

• support for all valid combinations of open flags (READ, WRITE, CREAT, TRUNC, APPEND, EXCL).

• Unicode paths, encoded as UTF-8. (This is dependent on SFTP clients' support for UTF-8, which may be a little sketchy.)

• getting file attributes, and setting attributes on an open handle.

• mutable files are handled correctly, including preserving the identity of the file when writing. (The previous SFTP implementation could only read and write immutable files, and would fail when listing a directory containing mutable files -- ticket #941.)

• the problem described in #645 should be fixed.

• latency reduction:
  • return success from an open request as soon as the parent directory has been read.
  • streaming reads, i.e. read the file as it is being downloaded (unfinished downloads are stopped when the file is closed or the SFTP session is logged out).
  • overwrite parts of a file as the original contents are being downloaded.

[davidsarah]

Finished recording patch 'New SFTP implementation: mutable files, read/write support, streaming download, Unicode filena mes, and more

[davidsarah]

Also see #531 for tests.

[francois]

This log file (sftp-small-file.log) was generated by copying a 18 byte file with Nautilus by SFTP. The upload stalled.

[davidsarah]

Replying to francois:

This log file (sftp-small-file.log) was generated by copying a 18 byte file with Nautilus by SFTP. The upload stalled.

francois, please undo the change to 'debug =' (to avoid conflicts), then 'darcs pull' on the ticket1037 branch and try this again. I've fixed #1038, and those changes may have affected this bug as well.

Note that the current code uses foolscap logging (although it can still use 'print' if you set 'use_foolscap_logging = False' on sftpd.py line 41). Instructions on how to view foolscap logs are in source:docs/logging.txt .

[francois]

Replying to davidsarah:

francois, please undo the change to 'debug =' (to avoid conflicts), then 'darcs pull' on the ticket1037 branch and try this again. I've fixed #1038, and those changes may have affected this bug as well.

I just gave it a try with Nautilus and the bug is still present. According to Nautilus, the upload operation is stalled and I could see an empty file stored on Tahoe. Please see attached log file and screenshot.

[zooko]

I'm reviewing, starting with the tests -- ​http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/browser/src/allmydata/test/test_sftp.py?rev=4295 .

Excellent work so far! I'm glad to see such thorough, feature-oriented tests. I noticed that you test that we return EOF error on a 0-length read if the pos is already at EOF, instead of returning an empty string on a 0-length read in that situation. I remember that we discussed this matter on IRC and that we settled on a good argument that this was preferable. However, I don't remember the argument! Please add it to the code -- ​http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/browser/src/allmydata/frontends/sftpd.py?rev=4295#L541 -- preferably in a docstring of def readChunk() if you remember it.

[zooko]

Add to ​test_openFile_read() a test of what happens when you read 0 bytes when you were not already at EOF.

[zooko]

​This test looks like it is asserting the wrong result -- the permissions on "small" should be 0660, not 0440. (Also it says FIXME next to it... :-))

[zooko]

Likewise I don't see why ​this attempt to open small_uri with flags FXF_WRITE should fail.

[davidsarah]

Replying to zooko:

Likewise I don't see why ​this attempt to open small_uri with flags FXF_WRITE should fail.

It should fail because:

• we don't have the parent of small_uri, since it was accessed by URI -- so we can't change its link in the parent;
• it's an immutable file, so we can't write to it directly.

[zooko]

David-Sarah think the problem reported by François has been fixed in the branch. Here are their instructions for testing the branch:

​http://tahoe-lafs.org/pipermail/tahoe-dev/2010-May/004339.html

[francois]

Replying to zooko:

David-Sarah think the problem reported by François has been fixed in the branch. Here are their instructions for testing the branch:

Yes, writing files with Nautilus is now working as expected.

[zooko]

Reviewing ​http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/browser/src/allmydata/test/test_sftp.py?rev=4295 and ​http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/browser/src/allmydata/frontends/sftpd.py?rev=4295 .

The following comments were made when I was under the mis-impression that we were following the latest draft spec. These comments are therefore wrong and irrelevant for the actual current code, but there may be some value in them:

• Can we add support for readLink and makeLink in the future?
• Please add a reference to which SFTP specification(s) we are following. (​http://tools.ietf.org/html/draft-ietf-secsh-filexfer-13? )
• On ​test_sftp.py line 528 how about if trying to open a file for writing when the filename is the empty string '' returns FX_INVALID_FILENAME (as defined in ​http://tools.ietf.org/html/draft-ietf-secsh-filexfer-13 ) instead of FX_NO_SUCH_FILE? (Note: twisted.conch.ssh.filetransfer doesn't define that symbol...)

[zooko]

I marked #531 as a duplicate of this ticket.

[bj0]

I did a couple quick tests of the SFTP interface from the ticket1037 branch:

I attached via sshfs. I could list and copy files to the mount, as well as read the files. When I tried to modify a file and save it, there was no error, but the resulting file was 0 bytes. I did this with an .odt file that I copied to the share using nautilus and edited using open office.

I tried the same thing using the nautilus "attach to server..." to connect. I was also able to copy the .odt to the directory and open it. When I tried to edit the file and save it, Open Office gave me 3 input/output errors. I closed Open Office and noticed the file size of the .odt (reported by nautilus) was about doubled. I opened the .odt again and it looked exactly the same. I repeated the edit attempt, and the file size (reported by nautilus) again doubled.

If there are any helpful logs I could attach let me know.

I'm doing this all on ubuntu 10.04 (on a laptop) connecting to a tahoe-lafs node on VM running ubuntu server 10.04

[zooko]

To test this feature get the current version of David-Sarah's branch:

darcs get --lazy http://allmydata.org/source/tahoe-lafs/ticket1037 tahoe-sftp

And follow the instructions to set up SFTP: ​http://tahoe-lafs.org/trac/tahoe-lafs/browser/docs/frontends/FTP-and-SFTP.txt , and then use your favorite SFTP client (especially if it is sshfs, gvfs or other FUSE-like integration layers that make the SFTP server look like a real POSIX filesystem) and report how it works.

[zooko]

Here is a report from the buildbot showing all builds which were specifically building the "ticket1037" branch:

​http://tahoe-lafs.org/buildbot/waterfall?show_events=true&branch=ticket1037&reload=none

[zooko]

Okay I've finished reviewing ​http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/browser/src/allmydata/test/test_sftp.py?rev=4295 and part of ​http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/browser/src/allmydata/frontends/sftpd.py?rev=4295 . Note that these are not the most recent versions of those files on the ticket1037 branch! My plan is to finish reviewing those versions and then review the patches that were added to the branch since that version.

• There isn't a test of: What if you setattr size past the end then close?
• Can you trunc in append mode with setattr? (What should it do—maybe return an error?)
• Can you extend in append mode with setattr? ...and then write more?

If you have a writable directory foo and a read-only child bar and you open foo/bar for write then you get PERMISSION DENIED, but if you have a writable directory foo and an immutable child bar and you open foo/bar for write then you succeed! This seems inconsistent and surprising. If foo/bar is a read-only file in a mutable directory then open("foo/bar").write('whee') will not change the result of open("foo/bar").read(), but if foo/bar is an immutable file then it will.

current behavior (inconsistent?):

• case a: writeable dir foo, writable child bar, open foo/bar for write->open it for write
• case b: writeable dir foo, readonly child bar, open foo/bar for write->permission denied
• case c: writeable dir foo, immutable child bar, open foo/bar for write->create a new one, copy in the contents of the old one, open it for write, when it is closed then unlink the old one and link the new one (immutable)

proposed new behavior:

• case a: writeable dir foo, writable child bar, open foo/bar for write->open it for write
• case b: writeable dir foo, readonly child bar, open foo/bar for write->permission denied
• case c: writeable dir foo, immutable child bar, open foo/bar for write->permission denied

[davidsarah]

Replying to zooko:

• There isn't a test of: What if you setattr size past the end then close?

This is now tested in test_openFile_write​.

• Can you trunc in append mode with setattr? (What should it do—maybe return an error?)

Good question. Currently you can truncate in FXF_APPEND mode (since it only affects the position of writes), but perhaps you shouldn't be able to. Being able to truncate would interfere with the optimization of append mode discussed in #1045.

• Can you extend in append mode with setattr? ...and then write more?

Yes. I'll add a unit test for this.

If you have a writable directory foo and a read-only child bar and you open foo/bar for write then you get PERMISSION DENIED, but if you have a writable directory foo and an immutable child bar and you open foo/bar for write then you succeed! This seems inconsistent and surprising.

This is discussed in #1063.

[davidsarah]

Replying to davidsarah:

• Can you trunc in append mode with setattr? (What should it do—maybe return an error?)

Good question. Currently you can truncate in FXF_APPEND mode (since it only affects the position of writes), but perhaps you shouldn't be able to. Being able to truncate would interfere with the optimization of append mode discussed in #1045.

ticket:1041#comment:6, not #1045.

[zooko]

I reviewed 546c3d2ed471daac.

[davidsarah]

Replying to zooko:

Reviewing ​http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/browser/src/allmydata/test/test_sftp.py?rev=4295 and ​http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/browser/src/allmydata/frontends/sftpd.py?rev=4295 .

Note that this corresponds to r4375 of trunk.

[davidsarah]

I suggest reviewing the patches since r4375 in three chunks, rather than individually:

• r4375 to r4383: changes before adding the heisenfiles tables -- ​sftpd.py, ​test_sftp.py

• r4383 to r4452: functional changes to add the heisenfiles tables and support for OpenSSH extensions -- ​sftpd.py, ​test_sftp.py

• r4452 to r4476: mainly improvements to test coverage and assertions -- ​sftpd.py, ​test_sftp.py