new SFTP implementation #1037

Closed
opened 2010-05-12 05:41:11 +00:00 by davidsarah · 26 comments
davidsarah commented 2010-05-12 05:41:11 +00:00
Owner

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.
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.
tahoe-lafs added the
code-frontend
major
enhancement
1.6.1
labels 2010-05-12 05:41:11 +00:00
tahoe-lafs added this to the 1.7.0 milestone 2010-05-12 05:41:11 +00:00
davidsarah commented 2010-05-12 06:04:02 +00:00
Author
Owner

Attachment sftpd.py (52128 bytes) added

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

**Attachment** sftpd.py (52128 bytes) added Finished recording patch 'New SFTP implementation: mutable files, read/write support, streaming download, Unicode filena mes, and more
51 KiB
davidsarah commented 2010-05-12 06:25:06 +00:00
Author
Owner

Also see #531 for tests.

Also see #531 for tests.
francois commented 2010-05-13 10:52:19 +00:00
Author
Owner

Attachment sftp-small-file.log (4574 bytes) added

**Attachment** sftp-small-file.log (4574 bytes) added
francois commented 2010-05-13 10:54:50 +00:00
Author
Owner

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

This log file (sftp-small-file.log) was generated by copying a 18 byte file with Nautilus by SFTP. The upload stalled.
davidsarah commented 2010-05-14 05:12:30 +00:00
Author
Owner

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 .

Replying to [francois](/tahoe-lafs/trac-2024-07-25/issues/1037#issuecomment-118864): > 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 commented 2010-05-14 16:07:14 +00:00
Author
Owner

Replying to [davidsarah]comment:4:

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.

Replying to [davidsarah]comment:4: > 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.
francois commented 2010-05-14 16:07:30 +00:00
Author
Owner

Attachment sftp-small-file-2.log (28672 bytes) added

**Attachment** sftp-small-file-2.log (28672 bytes) added
francois commented 2010-05-14 16:07:41 +00:00
Author
Owner

Attachment nautilus-dialog.jpeg (43798 bytes) added

**Attachment** nautilus-dialog.jpeg (43798 bytes) added
zooko commented 2010-05-17 04:25:20 +00:00
Author
Owner

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.

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 commented 2010-05-17 04:30:39 +00:00
Author
Owner

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

Add to [test_openFile_read()](http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/browser/src/allmydata/test/test_sftp.py?rev=4295#L353) a test of what happens when you read 0 bytes when you were not already at EOF.
zooko commented 2010-05-17 04:45:05 +00:00
Author
Owner

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... :-))

[This test](http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/browser/src/allmydata/test/test_sftp.py?rev=4295#L340) 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 commented 2010-05-17 04:49:02 +00:00
Author
Owner

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

Likewise I don't see why [this attempt](http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/browser/src/allmydata/test/test_sftp.py?rev=4295#L564) to open `small_uri` with flags `FXF_WRITE` should fail.
davidsarah commented 2010-05-17 05:09:26 +00:00
Author
Owner

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.
Replying to [zooko](/tahoe-lafs/trac-2024-07-25/issues/1037#issuecomment-118870): > Likewise I don't see why [this attempt](http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/browser/src/allmydata/test/test_sftp.py?rev=4295#L564) 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 commented 2010-05-17 05:37:39 +00:00
Author
Owner

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

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 commented 2010-05-17 08:27:14 +00:00
Author
Owner

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.

Replying to [zooko](/tahoe-lafs/trac-2024-07-25/issues/1037#issuecomment-118872): > 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 commented 2010-05-18 15:31:28 +00:00
Author
Owner

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:

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](http://tahoe-lafs.org/trac/tahoe-lafs-ticket1037/browser/src/allmydata/test/test_sftp.py?rev=4295#L528) 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 commented 2010-05-18 21:03:38 +00:00
Author
Owner

I marked #531 as a duplicate of this ticket.

I marked #531 as a duplicate of this ticket.
Author
Owner

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

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 commented 2010-05-19 07:19:48 +00:00
Author
Owner

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.

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 commented 2010-05-20 05:34:48 +00:00
Author
Owner

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

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 commented 2010-05-30 01:56:14 +00:00
Author
Owner

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
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 commented 2010-06-10 17:27:53 +00:00
Author
Owner

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]source:src/allmydata/test/test_sftp.py?rev=4466#L611.

  • 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.

Replying to [zooko](/tahoe-lafs/trac-2024-07-25/issues/1037#issuecomment-118879): > * There isn't a test of: What if you setattr size past the end then close? This is now tested in [test_openFile_write]source:src/allmydata/test/test_sftp.py?rev=4466#L611. > * 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 commented 2010-06-10 17:37:13 +00:00
Author
Owner

Replying to [davidsarah]comment:19:

  • 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:118867, not #1045.

Replying to [davidsarah]comment:19: > > * 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:118867](/tahoe-lafs/trac-2024-07-25/issues/1037#issuecomment-118867), not #1045.
zooko commented 2010-06-10 19:14:03 +00:00
Author
Owner

I reviewed changeset:546c3d2ed471daac.

I reviewed changeset:546c3d2ed471daac.
davidsarah commented 2010-06-13 02:27:51 +00:00
Author
Owner
Replying to [zooko](/tahoe-lafs/trac-2024-07-25/issues/1037#issuecomment-118874): > 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 commented 2010-06-13 02:56:21 +00:00
Author
Owner

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

I suggest reviewing the patches since r4375 in three chunks, rather than individually: - r4375 to r4383: changes before adding the heisenfiles tables -- [sftpd.py](http://tahoe-lafs.org/trac/tahoe-lafs/changeset?new=4383%40src%2Fallmydata%2Ffrontends%2Fsftpd.py&old=4375%40src%2Fallmydata%2Ffrontends%2Fsftpd.py), [test_sftp.py](http://tahoe-lafs.org/trac/tahoe-lafs/changeset?new=4383%40src%2Fallmydata%2Ftest%2Ftest_sftp.py&old=4375%40src%2Fallmydata%2Ftest%2Ftest_sftp.py) - r4383 to r4452: functional changes to add the heisenfiles tables and support for OpenSSH extensions -- [sftpd.py](http://tahoe-lafs.org/trac/tahoe-lafs/changeset?new=4452%40src%2Fallmydata%2Ffrontends%2Fsftpd.py&old=4383%40src%2Fallmydata%2Ffrontends%2Fsftpd.py), [test_sftp.py](http://tahoe-lafs.org/trac/tahoe-lafs/changeset?new=4452%40src%2Fallmydata%2Ftest%2Ftest_sftp.py&old=4383%40src%2Fallmydata%2Ftest%2Ftest_sftp.py) - r4452 to r4476: mainly improvements to test coverage and assertions -- [sftpd.py](http://tahoe-lafs.org/trac/tahoe-lafs/changeset?new=4476%40src%2Fallmydata%2Ffrontends%2Fsftpd.py&old=4452%40src%2Fallmydata%2Ffrontends%2Fsftpd.py), [test_sftp.py](http://tahoe-lafs.org/trac/tahoe-lafs/changeset?new=4476%40src%2Fallmydata%2Ftest%2Ftest_sftp.py&old=4452%40src%2Fallmydata%2Ftest%2Ftest_sftp.py)
tahoe-lafs added the
fixed
label 2010-06-19 03:50:23 +00:00
davidsarah closed this issue 2010-06-19 03:50:23 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: tahoe-lafs/trac-2024-07-25#1037
No description provided.