Merge lp:~facundo/ubuntuone-client/isolate-accessing-functions into lp:ubuntuone-client

[Facundo Batista]

Finish separating all user files access.

Finish separating all user files access.

As part of the plan to get all paths as Unicode internally, this branch isolates *everything* that touches user files into separated functions. This do not include syncdaemon controlled paths (for example, the file of version number for FSM, or the directory where the metadata is stored).

So, please be sure that nothing is missing in this regard while reviewing.

[Guillermo Gonzalez (verterok)]

Hi,

Only a few questions/comments:

1) What do you think about changing the name of the access function, to something more explicit e.g: is_readeable or can_read or...you get the idea ;-)

2) The stat_path is using os.lstat (in linux) but it's used to replace calls to os.stat, e.g: in the before the hashing loop in ubuntuone/syncdaemon/hash_queue.py (and also in local rescan). Is this change ok? should be using lstat or stat?

Beside those two issues, looks really good.

review: Needs Information

[Facundo Batista (facundo)]

1) I thought about using other name, but "access" is the name it always been used (not only in Python, also in the OS and other languages), and *today* is only to check readability (tomorrow it can grow other params).

2) All places I found we were using lstat/stat, we really should not follow symlinks or we don't have symlinks to follow.

Thanks!

[Guillermo Gonzalez (verterok)]

review: Approve

[Manuel de la Peña (mandel)]

Looks good to me, later we will have to add the new decorators in ubuntuone-dev-tools to ignore platform specific tests. I'll take care of that as soon as I start actively working on the code. For the lest everything looks great!

review: Approve

[Ubuntu One Auto Pilot (otto-pilot)]

Attempt to merge into lp:ubuntuone-client failed due to conflicts:

text conflict in ubuntuone/syncdaemon/filesystem_manager.py
text conflict in ubuntuone/syncdaemon/local_rescan.py

[lp:~facundo/ubuntuone-client/isolate-accessing-functions]

780. By Facundo Batista on 2011-01-05

Merged trunk in

[Guillermo Gonzalez (verterok)]

+1

review: Approve

=== modified file 'tests/platform/test_os_helper.py'
--- tests/platform/test_os_helper.py	2010-12-02 21:27:56 +0000
+++ tests/platform/test_os_helper.py	2011-01-05 15:16:24 +0000
@@ -15,22 +15,28 @@
 #
 # You should have received a copy of the GNU General Public License along
 # with this program.  If not, see <http://www.gnu.org/licenses/>.
+
 """Linux specific tests for the platform module."""
+
+import errno
 import os
 
 from contrib.testing.testcase import BaseTwistedTestCase
 from ubuntuone.platform.linux import (
-    set_dir_readonly,
-    set_file_readonly,
-    set_dir_readwrite,
-    set_file_readwrite,
+    access,
     allow_writes,
-    remove_file,
-    remove_dir,
-    path_exists,
+    listdir,
     make_dir,
     open_file,
+    path_exists,
+    remove_dir,
+    remove_file,
     rename,
+    set_dir_readonly,
+    set_dir_readwrite,
+    set_file_readonly,
+    set_file_readwrite,
+    stat_path,
 )
 
 
@@ -183,3 +189,66 @@
         self.assertFalse(os.path.exists(testdir1))
         self.assertTrue(os.path.exists(testdir2))
 
+    def test_listdir(self):
+        """Return a list of the files in a dir."""
+        open(os.path.join(self.basedir, 'foo'), 'w').close()
+        open(os.path.join(self.basedir, 'bar'), 'w').close()
+        l = listdir(self.basedir)
+        self.assertEqual(sorted(l), ['bar', 'foo', 'test_file'])
+
+    def test_access_rw(self):
+        """Test access on a file with full permission."""
+        self.assertTrue(access(self.testfile))
+
+    def test_access_ro(self):
+        """Test access on a file with read only permission."""
+        os.chmod(self.testfile, 0o444)
+        self.assertTrue(access(self.testfile))
+        os.chmod(self.testfile, 0o664)
+
+    def test_access_nothing(self):
+        """Test access on a file with no permission at all."""
+        os.chmod(self.testfile, 0o000)
+        self.assertFalse(access(self.testfile))
+        os.chmod(self.testfile, 0o664)
+
+    def test_stat_normal(self):
+        """Test on a normal file."""
+        self.assertEqual(os.stat(self.testfile), stat_path(self.testfile))
+
+    def test_stat_symlink(self):
+        """Test that it doesn't follow symlinks.
+
+        We compare the inode only (enough to see if it's returning info
+        from the link or the linked), as we can not compare the full stat
+        because the st_mode will be different.
+        """
+        link = os.path.join(self.basedir, 'foo')
+        os.symlink(self.testfile, link)
+        self.assertNotEqual(os.stat(link).st_ino, stat_path(link).st_ino)
+        self.assertEqual(os.lstat(link).st_ino, stat_path(link).st_ino)
+
+    def test_stat_no_path(self):
+        """Test that it raises proper error when no file is there."""
+        try:
+            return stat_path(os.path.join(self.basedir, 'nofile'))
+        except OSError, e:
+            self.assertEqual(e.errno, errno.ENOENT)
+
+    def test_path_exists_file_yes(self):
+        """The file is there."""
+        self.assertTrue(path_exists(self.testfile))
+
+    def test_path_exists_file_no(self):
+        """The file is not there."""
+        os.remove(self.testfile)
+        self.assertFalse(path_exists(self.testfile))
+
+    def test_path_exists_dir_yes(self):
+        """The dir is there."""
+        self.assertTrue(path_exists(self.basedir))
+
+    def test_path_exists_dir_no(self):
+        """The dir is not there."""
+        self.rmtree(self.basedir)
+        self.assertFalse(path_exists(self.basedir))

=== modified file 'ubuntuone/platform/linux/__init__.py'
--- ubuntuone/platform/linux/__init__.py	2010-12-15 18:36:41 +0000
+++ ubuntuone/platform/linux/__init__.py	2011-01-05 15:16:24 +0000
@@ -26,17 +26,20 @@
 from dbus.mainloop.glib import DBusGMainLoop
 
 from ubuntuone.platform.linux.os_helper import (
-    set_file_readonly,
-    set_file_readwrite,
-    set_dir_readonly,
-    set_dir_readwrite,
+    access,
     allow_writes,
-    remove_file,
-    remove_dir,
-    path_exists,
+    listdir,
     make_dir,
     open_file,
+    path_exists,
+    remove_dir,
+    remove_file,
     rename,
+    set_dir_readonly,
+    set_dir_readwrite,
+    set_file_readonly,
+    set_file_readwrite,
+    stat_path,
 )
 from ubuntuone.platform.linux.vm_helper import (create_shares_link,
     get_udf_path_name, get_udf_path, get_share_path, VMMetadataUpgraderMixIn)

=== modified file 'ubuntuone/platform/linux/os_helper.py'
--- ubuntuone/platform/linux/os_helper.py	2010-12-02 21:27:56 +0000
+++ ubuntuone/platform/linux/os_helper.py	2011-01-05 15:16:24 +0000
@@ -80,3 +80,16 @@
 def rename(path_from, path_to):
     """Rename a file or directory."""
     os.rename(path_from, path_to)
+
+def listdir(directory):
+    """List a directory."""
+    return os.listdir(directory)
+
+def access(path):
+    """Return if the path is at least readable."""
+    return os.access(path, os.R_OK)
+
+def stat_path(path):
+    """Return stat info about a path."""
+    return os.lstat(path)
+

=== modified file 'ubuntuone/syncdaemon/filesystem_manager.py'
--- ubuntuone/syncdaemon/filesystem_manager.py	2011-01-04 13:22:34 +0000
+++ ubuntuone/syncdaemon/filesystem_manager.py	2011-01-05 15:16:24 +0000
@@ -40,16 +40,17 @@
     MOVE_LIMBO_ROW_TYPE,
 )
 from ubuntuone.platform import (
+    make_dir,
+    open_file,
+    path_exists,
+    remove_dir,
+    remove_file,
+    rename,
     set_dir_readonly,
     set_dir_readwrite,
     set_file_readonly,
     set_file_readwrite,
-    path_exists,
-    remove_file,
-    remove_dir,
-    make_dir,
-    open_file,
-    rename,
+    stat_path,
 )
 
 METADATA_VERSION = "6"
@@ -837,7 +838,7 @@
         # we try to stat, if we fail, so what?
         #pylint: disable-msg=W0704
         try:
-            mdobj["stat"] = os.stat(path_to)  # needed if not the same FS
+            mdobj["stat"] = stat_path(path_to)  # needed if not the same FS
         except OSError:
             log_warning("Got an OSError while getting the stat of %r", path_to)
         self.fs[mdid] = mdobj
@@ -1003,7 +1004,7 @@
         # get the values
         mdobj = self.fs[mdid]
         partial_in_md = mdobj["info"]["is_partial"]
-        partial_in_disk = os.path.exists(self._get_partial_path(mdobj))
+        partial_in_disk = path_exists(self._get_partial_path(mdobj))
 
         # check and return
         if partial_in_md != partial_in_disk:
@@ -1042,7 +1043,7 @@
         path = self.get_abspath(mdobj['share_id'], mdobj['path'])
         with self._enable_share_write(share_id, os.path.dirname(path)):
             # if we don't have the dir yet, create it
-            if is_dir and not os.path.exists(path):
+            if is_dir and not path_exists(path):
                 self.eq.add_to_mute_filter("FS_DIR_CREATE", path=path)
                 make_dir(path)
 
@@ -1129,7 +1130,7 @@
         #pylint: disable-msg=W0704
         try:
             # don't alert EQ, partials are in other directory, not watched
-            os.remove(partial_path)
+            remove_file(partial_path)
         except OSError, e:
             # we only remove it if its there.
             m = "OSError %s when trying to remove partial_path %r"
@@ -1491,7 +1492,7 @@
     os.lstat is used as we don't support symlinks
     """
     try:
-        return os.lstat(path)
+        return stat_path(path)
     except OSError, e:
         if e.errno == errno.ENOENT:
             return None

=== modified file 'ubuntuone/syncdaemon/hash_queue.py'
--- ubuntuone/syncdaemon/hash_queue.py	2010-12-02 21:27:56 +0000
+++ ubuntuone/syncdaemon/hash_queue.py	2011-01-05 15:16:24 +0000
@@ -23,7 +23,6 @@
 import logging
 import threading
 import Queue
-import os
 
 from twisted.internet import reactor
 
@@ -32,6 +31,7 @@
 
 from ubuntuone.platform import (
     open_file,
+    stat_path,
 )
 
 class StopHashing(Exception):
@@ -112,7 +112,7 @@
         crc = 0
         size = 0
         try:
-            initial_stat = os.stat(path)
+            initial_stat = stat_path(path)
             with open_file(path) as fh:
                 while True:
                     # stop hashing if path_to_cancel == path or _stopped is True

=== modified file 'ubuntuone/syncdaemon/local_rescan.py'
--- ubuntuone/syncdaemon/local_rescan.py	2010-12-20 19:56:56 +0000
+++ ubuntuone/syncdaemon/local_rescan.py	2011-01-05 15:16:24 +0000
@@ -30,9 +30,12 @@
 
 from ubuntuone.syncdaemon.interfaces import IMarker
 from ubuntuone.platform import (
+    access,
+    listdir,
     path_exists,
     remove_file,
     rename,
+    stat_path,
 )
 
 class ScanTransactionDirty(Exception):
@@ -104,7 +107,7 @@
         fix the nodes states, we just remove them to don't let garbage behind.
         """
         try:
-            partials = os.listdir(self.fsm.partials_dir)
+            partials = listdir(self.fsm.partials_dir)
         except OSError, e:
             if e.errno != errno.ENOENT:
                 raise
@@ -114,7 +117,7 @@
         for partial in partials:
             partial_path = os.path.join(self.fsm.partials_dir, partial)
             log_debug("Removing old .partial: %r", partial_path)
-            os.remove(partial_path)
+            remove_file(partial_path)
 
     def _process_limbo(self):
         """Process the FSM limbos and send corresponding AQ orders."""
@@ -243,7 +246,7 @@
 
         for ancestor in volume.ancestors:
             # check that ancestor is still there
-            if not os.access(ancestor, os.R_OK):
+            if not access(ancestor):
                 log_info("Tree broken at path: %r", volume.path)
                 revert_watches()
                 return False
@@ -255,7 +258,7 @@
                 self.eq.add_watch(ancestor)
 
         # finally, check that UDF is ok in disk
-        if not os.access(volume.path, os.R_OK):
+        if not access(volume.path):
             revert_watches()
             return False
 
@@ -365,7 +368,7 @@
         """
         if oldstat is None:
             return True
-        newstat = os.stat(fullname)
+        newstat = stat_path(fullname)
         different = (newstat.st_ino != oldstat.st_ino or
                      newstat.st_size != oldstat.st_size or
                      newstat.st_mtime != oldstat.st_mtime)
@@ -614,18 +617,18 @@
             """The scan, really."""
 
             log_debug("scanning the dir %r", dirpath)
-            listdir = os.listdir(dirpath)
+            dircontent = listdir(dirpath)
 
             # get the info from disk
             dnames = []
             fnames = []
-            for something in listdir:
+            for something in dircontent:
                 fullname = os.path.join(dirpath, something)
                 stat_result = get_stat(fullname)
                 if stat_result is None or \
                    stat.S_ISLNK(stat_result.st_mode):
                     continue
-                elif not os.access(fullname, os.R_OK):
+                elif not access(fullname):
                     log_warning("Ignoring path as we don't have enough "
                                 "permissions to track it: %r", fullname)
                     continue

=== modified file 'ubuntuone/syncdaemon/sync.py'
--- ubuntuone/syncdaemon/sync.py	2010-12-20 19:56:56 +0000
+++ ubuntuone/syncdaemon/sync.py	2011-01-05 15:16:24 +0000
@@ -32,6 +32,9 @@
 from ubuntuone.syncdaemon.logger import DebugCapture
 from ubuntuone.syncdaemon.filesystem_manager import InconsistencyError
 from ubuntuone.syncdaemon.volume_manager import VolumeDoesNotExist
+from ubuntuone.platform import (
+    stat_path,
+)
 
 empty_hash = ""
 
@@ -307,7 +310,7 @@
 
     def validate_actual_data(self, path, oldstat):
         """Validates that the received info is not obsolete."""
-        newstat = os.stat(path)
+        newstat = stat_path(path)
         if newstat.st_ino != oldstat.st_ino or \
            newstat.st_size != oldstat.st_size or \
            newstat.st_mtime != oldstat.st_mtime: