Ticket #1106: sftp-comments.dpatch

Mon Jul 12 03:55:37 GMT Daylight Time 2010  david-sarah@jacaranda.org
  * SFTP: address some of the comments in zooko's review (#1106).

New patches:

[SFTP: address some of the comments in zooko's review (#1106).
david-sarah@jacaranda.org**20100712025537
 Ignore-this: c3921638a2d4f1de2a776ae78e4dc37e
] {
hunk ./src/allmydata/frontends/sftpd.py 81
 
 
 def _convert_error(res, request):
+    """If res is not a Failure, return it, otherwise reraise the appropriate
+    SFTPError."""
+
     if not isinstance(res, Failure):
         logged_res = res
         if isinstance(res, str): logged_res = "<data of length %r>" % (len(res),)
hunk ./src/allmydata/frontends/sftpd.py 94
     logmsg("RAISE %r %r" % (request, err.value), level=OPERATIONAL)
     try:
         if noisy: logmsg(traceback.format_exc(err.value), level=NOISY)
-    except:  # pragma: no cover
+    except Exception:  # pragma: no cover
         pass
 
     # The message argument to SFTPError must not reveal information that
hunk ./src/allmydata/frontends/sftpd.py 98
-    # might compromise anonymity.
+    # might compromise anonymity, if we are running over an anonymous network.
 
     if err.check(SFTPError):
         # original raiser of SFTPError has responsibility to ensure anonymity
hunk ./src/allmydata/frontends/sftpd.py 146
     st_gid = "tahoe"
     st_mtime = attrs.get("mtime", 0)
     st_mode = attrs["permissions"]
-    # TODO: check that clients are okay with this being a "?".
-    # (They should be because the longname is intended for human
-    # consumption.)
     st_size = attrs.get("size", "?")
     # We don't know how many links there really are to this object.
     st_nlink = 1
hunk ./src/allmydata/frontends/sftpd.py 392
             self.overwrite(self.current_size, "\x00" * (size - self.current_size))
         self.current_size = size
 
-        # invariant: self.download_size <= self.current_size
+        # make the invariant self.download_size <= self.current_size be true again
         if size < self.download_size:
             self.download_size = size
hunk ./src/allmydata/frontends/sftpd.py 395
+
         if self.downloaded >= self.download_size:
             self.finish()
 
hunk ./src/allmydata/frontends/sftpd.py 401
     def registerProducer(self, p, streaming):
         if noisy: self.log(".registerProducer(%r, streaming=%r)" % (p, streaming), level=NOISY)
+        if self.producer is not None:
+            raise RuntimeError("producer is already registered")
+
         self.producer = p
         if streaming:
             # call resumeProducing once to start things off
hunk ./src/allmydata/frontends/sftpd.py 477
                 return
             if noisy: self.log("MILESTONE %r %r" % (next, d), level=NOISY)
             heapq.heappop(self.milestones)
-            eventually_callback(d)(None)
+            eventually(d.callback, None)
 
         if milestone >= self.download_size:
             self.finish()
hunk ./src/allmydata/frontends/sftpd.py 550
         return self.done
 
     def finish(self):
+        """Called by the producer when it has finished producing, or when we have
+        received enough bytes, or as a result of a close. Defined by IFinishableConsumer."""
+
         while len(self.milestones) > 0:
             (next, d) = self.milestones[0]
             if noisy: self.log("MILESTONE FINISH %r %r" % (next, d), level=NOISY)
hunk ./src/allmydata/frontends/sftpd.py 560
             # The callback means that the milestone has been reached if
             # it is ever going to be. Note that the file may have been
             # truncated to before the milestone.
-            eventually_callback(d)(None)
+            eventually(d.callback, None)
 
     def close(self):
         if not self.is_closed:
hunk ./src/allmydata/frontends/sftpd.py 567
             self.is_closed = True
             try:
                 self.f.close()
-            except BaseException, e:
+            except Exception, e:
                 self.log("suppressed %r from close of temporary file %r" % (e, self.f), level=WEIRD)
         self.finish()
 
hunk ./src/allmydata/frontends/sftpd.py 582
     implements(ISFTPFile)
     """I represent a file handle to a particular file on an SFTP connection.
     I am used only for short immutable files opened in read-only mode.
-    The file contents are downloaded to memory when I am created."""
+    When I am created, the file contents start to be downloaded to memory.
+    self.async is used to delay read requests until the download has finished."""
 
     def __init__(self, userpath, filenode, metadata):
         PrefixingLogMixin.__init__(self, facility="tahoe.sftp", prefix=userpath)
hunk ./src/allmydata/frontends/sftpd.py 617
             # i.e. we respond with an EOF error iff offset is already at EOF.
 
             if offset >= len(data):
-                eventually_errback(d)(SFTPError(FX_EOF, "read at or past end of file"))
+                eventually(d.errback, SFTPError(FX_EOF, "read at or past end of file"))
             else:
hunk ./src/allmydata/frontends/sftpd.py 619
-                eventually_callback(d)(data[offset:min(offset+length, len(data))])
+                eventually(d.callback, data[offset:offset+length])  # truncated if offset+length > len(data)
             return data
         self.async.addCallbacks(_read, eventually_errback(d))
         d.addBoth(_convert_error, request)
hunk ./src/allmydata/frontends/sftpd.py 714
         else:
             assert IFileNode.providedBy(filenode), filenode
 
-            # TODO: use download interface described in #993 when implemented.
             if filenode.is_mutable():
                 self.async.addCallback(lambda ign: filenode.download_best_version())
                 def _downloaded(data):
hunk ./src/allmydata/frontends/sftpd.py 731
                     filenode.read(self.consumer, 0, None)
                 self.async.addCallback(_read)
 
-        eventually_callback(self.async)(None)
+        eventually(self.async.callback, None)
 
         if noisy: self.log("open done", level=NOISY)
         return self
hunk ./src/allmydata/frontends/sftpd.py 919
 
             # self.filenode might be None, but that's ok.
             attrs = _populate_attrs(self.filenode, self.metadata, size=self.consumer.get_current_size())
-            eventually_callback(d)(attrs)
+            eventually(d.callback, attrs)
             return None
         self.async.addCallbacks(_get, eventually_errback(d))
         d.addBoth(_convert_error, request)
hunk ./src/allmydata/frontends/sftpd.py 957
                 # TODO: should we refuse to truncate a file opened with FXF_APPEND?
                 # <http://allmydata.org/trac/tahoe-lafs/ticket/1037#comment:20>
                 self.consumer.set_current_size(size)
-            eventually_callback(d)(None)
+            eventually(d.callback, None)
             return None
         self.async.addCallbacks(_set, eventually_errback(d))
         d.addBoth(_convert_error, request)
}

Context:

[upcase_since_on_welcome
terrellrussell@gmail.com**20100708193903] 
[server_version_on_welcome_page.dpatch.txt
freestorm77@gmail.com**20100605191721
 Ignore-this: b450c76dc875f5ac8cca229a666cbd0a
 
 
 - The storage server version is 0 for all storage nodes in the Welcome Page
 
 
] 
[NEWS: add NEWS snippets about two recent patches
zooko@zooko.com**20100708162058
 Ignore-this: 6c9da6a0ad7351a960bdd60f81532899
] 
[directory_html_top_banner.dpatch
freestorm77@gmail.com**20100622205301
 Ignore-this: 1d770d975e0c414c996564774f049bca
 
 The div tag with the link "Return to Welcome page" on the directory.xhtml page is not correct
 
] 
[tahoe_css_toolbar.dpatch
freestorm77@gmail.com**20100622210046
 Ignore-this: 5b3ebb2e0f52bbba718a932f80c246c0
 
 CSS modification to be correctly diplayed with Internet Explorer 8
 
 The links on the top of page directory.xhtml are not diplayed in the same line as display with Firefox.
 
] 
[runnin_test_tahoe_css.dpatch
freestorm77@gmail.com**20100622214714
 Ignore-this: e0db73d68740aad09a7b9ae60a08c05c
 
 Runnin test for changes in tahoe.css file
 
] 
[runnin_test_directory_xhtml.dpatch
freestorm77@gmail.com**20100622201403
 Ignore-this: f8962463fce50b9466405cb59fe11d43
 
 Runnin test for diretory.xhtml top banner
 
] 
[stringutils.py: tolerate sys.stdout having no 'encoding' attribute.
david-sarah@jacaranda.org**20100626040817
 Ignore-this: f42cad81cef645ee38ac1df4660cc850
] 
[quickstart.html: python 2.5 -> 2.6 as recommended version
david-sarah@jacaranda.org**20100705175858
 Ignore-this: bc3a14645ea1d5435002966ae903199f
] 
[SFTP: don't call .stopProducing on the producer registered with OverwriteableFileConsumer (which breaks with warner's new downloader).
david-sarah@jacaranda.org**20100628231926
 Ignore-this: 131b7a5787bc85a9a356b5740d9d996f
] 
[docs/how_to_make_a_tahoe-lafs_release.txt: trivial correction, install.html should now be quickstart.html.
david-sarah@jacaranda.org**20100625223929
 Ignore-this: 99a5459cac51bd867cc11ad06927ff30
] 
[setup: in the Makefile, refuse to upload tarballs unless someone has passed the environment variable "BB_BRANCH" with value "trunk"
zooko@zooko.com**20100619034928
 Ignore-this: 276ddf9b6ad7ec79e27474862e0f7d6
] 
[trivial: tiny update to in-line comment
zooko@zooko.com**20100614045715
 Ignore-this: 10851b0ed2abfed542c97749e5d280bc
 (I'm actually committing this patch as a test of the new eager-annotation-computation of trac-darcs.)
] 
[docs: about.html link to home page early on, and be decentralized storage instead of cloud storage this time around
zooko@zooko.com**20100619065318
 Ignore-this: dc6db03f696e5b6d2848699e754d8053
] 
[docs: update about.html, especially to have a non-broken link to quickstart.html, and also to comment out the broken links to "for Paranoids" and "for Corporates"
zooko@zooko.com**20100619065124
 Ignore-this: e292c7f51c337a84ebfeb366fbd24d6c
] 
[TAG allmydata-tahoe-1.7.0
zooko@zooko.com**20100619052631
 Ignore-this: d21e27afe6d85e2e3ba6a3292ba2be1
] 
Patch bundle hash:
90a0f8f93d2ebf7fac7b5a8277dc7347be41ba38