tahoe-lafs/trac

new SFTP implementation #1037

New issue

Closed

opened 2010-05-12 05:41:11 +00:00 by daira · 26 comments

daira commented

2010-05-12 05:41:11 +00:00

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.

daira added the

labels 2010-05-12 05:41:11 +00:00

daira added this to the 1.7.0 milestone 2010-05-12 05:41:11 +00:00

daira commented

2010-05-12 06:04:02 +00:00

Author

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

sftpd.py

51 KiB

daira commented

2010-05-12 06:25:06 +00:00

Author

Also see #531 for tests.

francois commented

2010-05-13 10:52:19 +00:00

Owner

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

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

sftp-small-file.log

4.5 KiB

francois commented

2010-05-13 10:54:50 +00:00

Owner

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

daira commented

2010-05-14 05:12:30 +00:00

Author

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/issues/1037#issuecomment-378104): > 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

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

Owner

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

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

sftp-small-file-2.log

28 KiB

francois commented

2010-05-14 16:07:41 +00:00

Owner

Attachment nautilus-dialog.jpeg (43798 bytes) added

**Attachment** nautilus-dialog.jpeg (43798 bytes) added

nautilus-dialog.jpeg

43 KiB

zooko commented

2010-05-17 04:25:20 +00:00

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 self-assigned this 2010-05-17 04:25:20 +00:00

zooko commented

2010-05-17 04:30:39 +00:00

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

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

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.

daira commented

2010-05-17 05:09:26 +00:00

Author

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/issues/1037#issuecomment-378111): > 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

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

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/issues/1037#issuecomment-378113): > 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

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

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

I marked #531 as a duplicate of this ticket.

bj0 commented

2010-05-18 21:18:21 +00:00

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

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

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

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

daira commented

2010-06-10 17:27:53 +00:00

Author

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/issues/1037#issuecomment-378120): > * 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.

daira commented

2010-06-10 17:37:13 +00:00

Author

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:378107, 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:378107](/tahoe-lafs/trac/issues/1037#issuecomment-378107), not #1045.

zooko commented

2010-06-10 19:14:03 +00:00

I reviewed changeset:546c3d2ed471daac.

daira commented

2010-06-13 02:27:51 +00:00

Author

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.

Replying to [zooko](/tahoe-lafs/trac/issues/1037#issuecomment-378115): > 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.

daira commented

2010-06-13 02:56:21 +00:00

Author

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)

daira added the

r/fixed

label 2010-06-19 03:50:23 +00:00

daira closed this issue

2010-06-19 03:50:23 +00:00