#1037 closed enhancement (fixed)

new SFTP implementation

Reported by: davidsarah Owned by: zooko
Priority: major Milestone: 1.7.0
Component: code-frontend Version: 1.6.1
Keywords: sftp mutable unicode performance Cc:
Launchpad Bug:

Description

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.

Attachments (4)

sftpd.py (50.9 KB) - added by davidsarah at 2010-05-12T06:04:02Z.
Finished recording patch 'New SFTP implementation: mutable files, read/write support, streaming download, Unicode filena mes, and more
sftp-small-file.log (4.5 KB) - added by francois at 2010-05-13T10:52:19Z.
sftp-small-file-2.log (28.0 KB) - added by francois at 2010-05-14T16:07:30Z.
nautilus-dialog.jpeg (42.8 KB) - added by francois at 2010-05-14T16:07:41Z.

Download all attachments as: .zip

Change History (29)

Changed at 2010-05-12T06:04:02Z by davidsarah

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

comment:1 Changed at 2010-05-12T06:04:42Z by davidsarah

  • Keywords performance review-needed added

comment:2 Changed at 2010-05-12T06:25:06Z by davidsarah

Also see #531 for tests.

Changed at 2010-05-13T10:52:19Z by francois

comment:3 follow-up: Changed at 2010-05-13T10:54:50Z by francois

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

comment:4 in reply to: ↑ 3 ; follow-up: Changed at 2010-05-14T05:12:30Z by 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 .

comment:5 in reply to: ↑ 4 Changed at 2010-05-14T16:07:14Z by 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.

Changed at 2010-05-14T16:07:30Z by francois

Changed at 2010-05-14T16:07:41Z by francois

comment:6 Changed at 2010-05-17T04:25:20Z by zooko

  • Owner set to zooko
  • Status changed from new to assigned

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.

comment:7 Changed at 2010-05-17T04:30:39Z by zooko

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

comment:8 Changed at 2010-05-17T04:45:05Z by 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... :-))

comment:9 follow-up: Changed at 2010-05-17T04:49:02Z by zooko

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

comment:10 in reply to: ↑ 9 Changed at 2010-05-17T05:09:26Z by 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.

comment:11 follow-up: Changed at 2010-05-17T05:37:39Z by 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

comment:12 in reply to: ↑ 11 Changed at 2010-05-17T08:27:14Z by 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.

comment:13 follow-up: Changed at 2010-05-18T15:31:28Z by 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:

comment:14 Changed at 2010-05-18T21:03:38Z by zooko

I marked #531 as a duplicate of this ticket.

comment:15 Changed at 2010-05-18T21:18:21Z by 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

comment:16 Changed at 2010-05-19T07:19:48Z by 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.

comment:17 Changed at 2010-05-20T05:34:48Z by 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

comment:18 follow-up: Changed at 2010-05-30T01:56:14Z by 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

comment:19 in reply to: ↑ 18 ; follow-up: Changed at 2010-06-10T17:27:53Z by 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.

comment:20 in reply to: ↑ 19 Changed at 2010-06-10T17:37:13Z by 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.

comment:21 Changed at 2010-06-10T19:14:03Z by zooko

I reviewed 546c3d2ed471daac.

comment:23 Changed at 2010-06-13T02:56:21Z by davidsarah

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

comment:24 Changed at 2010-06-19T03:50:23Z by davidsarah

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:25 Changed at 2010-06-19T05:28:54Z by davidsarah

  • Keywords review-needed removed
Note: See TracTickets for help on using tickets.