SFTP: further small improvements to test coverage. Also ensure that after a test failure, later tests don't fail spuriously due to the checks for heisenfile leaks.

src/allmydata/frontends/sftpd.py

@@ -765,9 +765,6 @@ class GeneralSFTPFile(PrefixingLogMixin):
         d.addBoth(_done)
         return d
 
-    def get_metadata(self):
-        return self.metadata
-
     def readChunk(self, offset, length):
         request = ".readChunk(%r, %r)" % (offset, length)
         self.log(request, level=OPERATIONAL)
@@ -869,7 +866,7 @@ class GeneralSFTPFile(PrefixingLogMixin):
         def _close(ign):
             d2 = self.consumer.when_done()
             if self.filenode and self.filenode.is_mutable():
-                self.log("update mutable file %r childname=%r" % (self.filenode, childname), level=OPERATIONAL)
+                self.log("update mutable file %r childname=%r metadata=%r" % (self.filenode, childname, self.metadata), level=OPERATIONAL)
                 if self.metadata.get('no-write', False) and not self.filenode.is_readonly():
                     assert parent and childname, (parent, childname, self.metadata)
                     d2.addCallback(lambda ign: parent.set_metadata_for(childname, self.metadata))
@@ -946,7 +943,10 @@ class GeneralSFTPFile(PrefixingLogMixin):
         d = defer.Deferred()
         def _set(ign):
             if noisy: self.log("_set(%r) in %r" % (ign, request), level=NOISY)
-            if only_if_at and only_if_at != _direntry_for(self.parent, self.childname, self.filenode):
+            current_direntry = _direntry_for(self.parent, self.childname, self.filenode)
+            if only_if_at and only_if_at != current_direntry:
+                if noisy: self.log("not setting attributes: current_direntry=%r in %r" %
+                                   (current_direntry, request), level=NOISY)
                 return None
 
             now = time()
@@ -988,6 +988,9 @@ class Reason:
 
 all_heisenfiles = {}
 
+def _reload():
+    global all_heisenfiles
+    all_heisenfiles = {}
 
 class SFTPUserHandler(ConchUser, PrefixingLogMixin):
     implements(ISFTPServer)
@@ -1510,17 +1513,17 @@ class SFTPUserHandler(ConchUser, PrefixingLogMixin):
             # For the standard SSH_FXP_RENAME operation, overwrite=False.
             # We also support the posix-rename@openssh.com extension, which uses overwrite=True.
 
-            d2 = defer.fail(NoSuchChildError())
+            d2 = defer.succeed(None)
             if not overwrite:
                 d2.addCallback(lambda ign: to_parent.get(to_childname))
-            def _expect_fail(res):
+                def _expect_fail(res):
-                if not isinstance(res, Failure):
+                    if not isinstance(res, Failure):
-                    raise SFTPError(FX_PERMISSION_DENIED, "cannot rename to existing path " + to_userpath)
+                        raise SFTPError(FX_PERMISSION_DENIED, "cannot rename to existing path " + to_userpath)
 
-                # It is OK if we fail for errors other than NoSuchChildError, since that probably
+                    # It is OK if we fail for errors other than NoSuchChildError, since that probably
-                # indicates some problem accessing the destination directory.
+                    # indicates some problem accessing the destination directory.
-                res.trap(NoSuchChildError)
+                    res.trap(NoSuchChildError)
-            d2.addBoth(_expect_fail)
+                d2.addBoth(_expect_fail)
 
             # If there are heisenfiles to be written at the 'from' direntry, then ensure
             # they will now be written at the 'to' direntry instead.

src/allmydata/test/test_sftp.py

@@ -73,6 +73,7 @@ class Handler(GridTestMixin, ShouldFailMixin, ReallyEqualMixin, unittest.TestCas
         def _created_root(node):
             self.root = node
             self.root_uri = node.get_uri()
+            sftpd._reload()
             self.handler = sftpd.SFTPUserHandler(self.client, self.root, self.username)
         d.addCallback(_created_root)
         return d
@@ -828,6 +829,8 @@ class Handler(GridTestMixin, ShouldFailMixin, ReallyEqualMixin, unittest.TestCas
         d.addCallback(lambda node: download_to_data(node))
         d.addCallback(lambda data: self.failUnlessReallyEqual(data, "abcde56789"))
 
+        d.addCallback(lambda ign: self.root.set_node(u"mutable2", self.mutable))
+
         # test writing to a mutable file
         d.addCallback(lambda ign:
                       self.handler.openFile("mutable", sftp.FXF_WRITE, {}))
@@ -839,6 +842,7 @@ class Handler(GridTestMixin, ShouldFailMixin, ReallyEqualMixin, unittest.TestCas
         d.addCallback(lambda ign: self.root.get(u"mutable"))
         def _check_same_file(node):
             self.failUnless(node.is_mutable())
+            self.failIf(node.is_readonly())
             self.failUnlessReallyEqual(node.get_uri(), self.mutable_uri)
             return node.download_best_version()
         d.addCallback(_check_same_file)
@@ -852,13 +856,19 @@ class Handler(GridTestMixin, ShouldFailMixin, ReallyEqualMixin, unittest.TestCas
         d.addCallback(_check_same_file)
         d.addCallback(lambda data: self.failUnlessReallyEqual(data, "mutable new! contents"))
 
-        # ... and with a setAttrs call that diminishes the parent link to read-only
+        # ... and with a setAttrs call that diminishes the parent link to read-only, first by path
         d.addCallback(lambda ign:
                       self.handler.openFile("mutable", sftp.FXF_WRITE, {}))
         def _write_mutable_setattr(wf):
             d2 = wf.writeChunk(8, "read-only link from parent")
 
             d2.addCallback(lambda ign: self.handler.setAttrs("mutable", {'permissions': 0444}))
+
+            d2.addCallback(lambda ign: self.root.get(u"mutable"))
+            d2.addCallback(lambda node: self.failUnless(node.is_readonly()))
+
+            d2.addCallback(lambda ign: wf.getAttrs())
+            d2.addCallback(lambda attrs: self.failUnlessReallyEqual(attrs['permissions'], S_IFREG | 0666))
             d2.addCallback(lambda ign: self.handler.getAttrs("mutable", followLinks=0))
             d2.addCallback(lambda attrs: self.failUnlessReallyEqual(attrs['permissions'], S_IFREG | 0444))
 
@@ -875,6 +885,30 @@ class Handler(GridTestMixin, ShouldFailMixin, ReallyEqualMixin, unittest.TestCas
         d.addCallback(_check_readonly_file)
         d.addCallback(lambda data: self.failUnlessReallyEqual(data, "mutable read-only link from parent"))
 
+        # ... and then by handle
+        d.addCallback(lambda ign:
+                      self.handler.openFile("mutable2", sftp.FXF_WRITE, {}))
+        def _write_mutable2_setattr(wf):
+            d2 = wf.writeChunk(7, "2")
+
+            d2.addCallback(lambda ign: wf.setAttrs({'permissions': 0444, 'size': 8}))
+
+            # The link isn't made read-only until the file is closed.
+            d2.addCallback(lambda ign: self.root.get(u"mutable2"))
+            d2.addCallback(lambda node: self.failIf(node.is_readonly()))
+
+            d2.addCallback(lambda ign: wf.getAttrs())
+            d2.addCallback(lambda attrs: self.failUnlessReallyEqual(attrs['permissions'], S_IFREG | 0444))
+            d2.addCallback(lambda ign: self.handler.getAttrs("mutable2", followLinks=0))
+            d2.addCallback(lambda attrs: self.failUnlessReallyEqual(attrs['permissions'], S_IFREG | 0666))
+
+            d2.addCallback(lambda ign: wf.close())
+            return d2
+        d.addCallback(_write_mutable2_setattr)
+        d.addCallback(lambda ign: self.root.get(u"mutable2"))
+        d.addCallback(_check_readonly_file)  # from above
+        d.addCallback(lambda data: self.failUnlessReallyEqual(data, "mutable2"))
+
         # test READ | WRITE without CREAT or TRUNC
         d.addCallback(lambda ign:
                       self.handler.openFile("small", sftp.FXF_READ | sftp.FXF_WRITE, {}))