Merge lp:~verterok/ubuntuone-client/fix-597870 into lp:ubuntuone-client

[Guillermo Gonzalez]

Fix fileystem manager _delete_dir_tree method to support read-only shares.

Fix fileystem manager _delete_dir_tree method to support read-only shares.
This will avoid conflicts in read only shares, when a lot of changes are going on and e.g: we get a delete for a node and another node(s) are added to the parent.

[John O'Brien (jdobrien)]

looks good. tests pass

review: Approve

[dobey (dobey)]

review: Approve

=== modified file 'tests/syncdaemon/test_fsm.py'
--- tests/syncdaemon/test_fsm.py	2010-06-21 20:17:59 +0000
+++ tests/syncdaemon/test_fsm.py	2010-06-24 13:56:33 +0000
@@ -2365,7 +2365,7 @@
 
 
 class SharesTests(FSMTestCase):
-    """ Tests fsm with ro and rw shares. """
+    """Test fsm with ro and rw shares."""
 
     def tearDown(self):
         """ cleanup the mess """
@@ -2426,6 +2426,95 @@
         self.fsm.delete_file(testdir)
         self.assertFalse(os.path.exists(testdir))
 
+    def test_delete_non_empty_dir_ro_share(self):
+        """Test that fsm is able to delete a non-empty dir in a ro.share."""
+        share = self.create_share('ro_share', 'ro_share_name',  self.fsm,
+                                       self.shares_dir, access_level='View')
+        testdir = os.path.join(share.path, "path2")
+        mdid = self.fsm.create(testdir, share.volume_id, is_dir=True)
+        self.fsm.set_node_id(testdir, "uuid2")
+        self.fsm.create_partial('uuid2', share.volume_id)
+        fd = self.fsm.get_partial_for_writing('uuid2', share.volume_id)
+        fd.flush()
+        fd.close()
+        self.fsm.remove_partial('uuid2', share.volume_id)
+        self.fsm.upload_finished(mdid, self.fsm.get_by_mdid(mdid).local_hash)
+        # crete a file inside the testdir
+        testfile = os.path.join(testdir, "a_file")
+        mdid = self.fsm.create(testfile, share.volume_id, is_dir=False)
+        self.fsm.set_node_id(testfile, "uuid3")
+        self.fsm.create_partial('uuid3', share.volume_id)
+        fd = self.fsm.get_partial_for_writing('uuid3', share.volume_id)
+        fd.flush()
+        fd.close()
+        self.fsm.commit_partial('uuid3', share.volume_id, None)
+        self.fsm.upload_finished(mdid, self.fsm.get_by_mdid(mdid).local_hash)
+        self.assertTrue(os.path.exists(testdir))
+        self.assertTrue(os.path.exists(testfile))
+        self.fsm.delete_file(testdir)
+        self.assertFalse(os.path.exists(testdir))
+        self.assertFalse(os.path.exists(testfile))
+
+    def test_delete_non_empty_dir_rw_share(self):
+        """Test that fsm is able to delete a non-empty dir in a rw.share."""
+        share = self.create_share('rw_share', 'rw_share_name',  self.fsm,
+                                       self.shares_dir, access_level='Modify')
+        testdir = os.path.join(share.path, "path2")
+        mdid = self.fsm.create(testdir, share.volume_id, is_dir=True)
+        self.fsm.set_node_id(testdir, "uuid2")
+        self.fsm.create_partial('uuid2', share.volume_id)
+        fd = self.fsm.get_partial_for_writing('uuid2', share.volume_id)
+        fd.flush()
+        fd.close()
+        self.fsm.remove_partial('uuid2', share.volume_id)
+        self.fsm.upload_finished(mdid, self.fsm.get_by_mdid(mdid).local_hash)
+        # crete a file inside the testdir
+        testfile = os.path.join(testdir, "a_file")
+        mdid = self.fsm.create(testfile, share.volume_id, is_dir=False)
+        self.fsm.set_node_id(testfile, "uuid3")
+        self.fsm.create_partial('uuid3', share.volume_id)
+        fd = self.fsm.get_partial_for_writing('uuid3', share.volume_id)
+        fd.flush()
+        fd.close()
+        self.fsm.commit_partial('uuid3', share.volume_id, None)
+        self.fsm.upload_finished(mdid, self.fsm.get_by_mdid(mdid).local_hash)
+        self.assertTrue(os.path.exists(testdir))
+        self.assertTrue(os.path.exists(testfile))
+        self.fsm.delete_file(testdir)
+        self.assertFalse(os.path.exists(testdir))
+        self.assertFalse(os.path.exists(testfile))
+
+    def test_delete_non_empty_dir_bad_perms_rw_share(self):
+        """Test that fsm is able to delete a non-empty dir in a rw.share."""
+        share = self.create_share('rw_share', 'rw_share_name',  self.fsm,
+                                       self.shares_dir, access_level='Modify')
+        testdir = os.path.join(share.path, "path2")
+        mdid = self.fsm.create(testdir, share.volume_id, is_dir=True)
+        self.fsm.set_node_id(testdir, "uuid2")
+        self.fsm.create_partial('uuid2', share.volume_id)
+        fd = self.fsm.get_partial_for_writing('uuid2', share.volume_id)
+        fd.flush()
+        fd.close()
+        self.fsm.remove_partial('uuid2', share.volume_id)
+        self.fsm.upload_finished(mdid, self.fsm.get_by_mdid(mdid).local_hash)
+        # crete a file inside the testdir
+        testfile = os.path.join(testdir, "a_file")
+        mdid = self.fsm.create(testfile, share.volume_id, is_dir=False)
+        self.fsm.set_node_id(testfile, "uuid3")
+        self.fsm.create_partial('uuid3', share.volume_id)
+        fd = self.fsm.get_partial_for_writing('uuid3', share.volume_id)
+        fd.flush()
+        fd.close()
+        self.fsm.commit_partial('uuid3', share.volume_id, None)
+        self.fsm.upload_finished(mdid, self.fsm.get_by_mdid(mdid).local_hash)
+        self.assertTrue(os.path.exists(testdir))
+        self.assertTrue(os.path.exists(testfile))
+        # make the dir read-only
+        os.chmod(testdir, 0555)
+        self.assertRaises(OSError, self.fsm.delete_file, testdir)
+        self.assertTrue(os.path.exists(testdir))
+        self.assertTrue(os.path.exists(testfile))
+
     def test_delete_file_ro_share(self):
         """ Test that fsm is able to delete a file in a ro-share. """
         self.share = self.create_share('ro_share', 'ro_share_name',  self.fsm,
@@ -2493,7 +2582,6 @@
         self.fsm.commit_partial('uuid3', self.share.volume_id, None)
         self.assertTrue(os.path.exists(testfile))
 
-
     def test_share_and_root(self):
         """ Test the creation of a file with the same relative path in a share
         and in the root.

=== modified file 'ubuntuone/syncdaemon/filesystem_manager.py'
--- ubuntuone/syncdaemon/filesystem_manager.py	2010-06-21 20:17:59 +0000
+++ ubuntuone/syncdaemon/filesystem_manager.py	2010-06-24 13:56:33 +0000
@@ -724,7 +724,7 @@
             del self._idx_node_id[(mdobj["share_id"], mdobj["node_id"])]
         del self.fs[mdid]
 
-    def _delete_dir_tree(self, path):
+    def _delete_dir_tree(self, path, share_id):
         """Helper function to recursively remove a non-empty directory.
 
         Removes the dir if there aren't local changes, and returns True.
@@ -753,7 +753,21 @@
             filter_name = is_dir and "FS_DIR_DELETE" or "FS_FILE_DELETE"
             self.eq.add_to_mute_filter(filter_name, p)
             self.delete_metadata(p)
-        shutil.rmtree(path)
+
+        # get the share and if it's read-only
+        share = self._get_share(share_id)
+        is_ro_share = not share.can_write()
+        def handle_ro_share(func, subpath, exc_info):
+            """Handle rmtree EACCES error for ro-shares."""
+            if exc_info[0] == OSError and exc_info[1].errno == errno.EACCES \
+               and is_ro_share:
+                # use EnableShareWrite directly to avoid a lot of share
+                # lookups.
+                with EnableShareWrite(share, subpath):
+                    func(subpath)
+            else:
+                raise
+        shutil.rmtree(path, onerror=handle_ro_share)
         return True
 
     def delete_file(self, path):
@@ -778,7 +792,8 @@
         except OSError, e:
             self.eq.rm_from_mute_filter(expected_event, path)
             if e.errno == errno.ENOTEMPTY:
-                if not self._delete_dir_tree(path=path):
+                if not self._delete_dir_tree(path=path,
+                                             share_id=mdobj['share_id']):
                     raise
             else:
                 m = "OSError %s when trying to remove file/dir %r"