Fix validation of marker dir objects

For marker directory objects, validate_object() always returned False.
This was because st_size from stat was being compared to Content-Length
stored in metadata. Unlike files, for directories st_size is always
4096. Hence the comparison would always be '4096 == 0' which would
fail.

This patch makes the following changes:
* Do size comparison of st_size and Content-Length only for files.
* Get rid of _is_dir everywhere. This will simplify things.

Change-Id: Ib75e06c4e3bce36bab11ce7d029ff327f33c3146
Signed-off-by: Prashanth Pai <ppai@redhat.com>
Reviewed-on: http://review.gluster.org/14423
Reviewed-by: Thiago da Silva <thiago@redhat.com>
Tested-by: Thiago da Silva <thiago@redhat.com>

4 files changed, 25 insertions, 31 deletions

diff --git a/gluster/swift/common/utils.py b/gluster/swift/common/utils.py
index 26e8c1b..70bf551 100644
--- a/gluster/swift/common/utils.py
+++ b/gluster/swift/common/utils.py
@@ -275,7 +275,7 @@ def validate_account(metadata):
     return False
 
 
-def validate_object(metadata, stat=None):
+def validate_object(metadata, statinfo=None):
     if not metadata:
         return False
 
@@ -287,11 +287,14 @@ def validate_object(metadata, stat=None):
        X_OBJECT_TYPE not in metadata.keys():
         return False
 
-    if stat and (int(metadata[X_CONTENT_LENGTH]) != stat.st_size):
+    if statinfo and stat.S_ISREG(statinfo.st_mode):
+
         # File length has changed.
+        if int(metadata[X_CONTENT_LENGTH]) != statinfo.st_size:
+            return False
+
         # TODO: Handle case where file content has changed but the length
         # remains the same.
-        return False
 
     if metadata[X_TYPE] == OBJECT:
         return True
diff --git a/gluster/swift/obj/diskfile.py b/gluster/swift/obj/diskfile.py
index c7240ae..b94cf3d 100644
--- a/gluster/swift/obj/diskfile.py
+++ b/gluster/swift/obj/diskfile.py
@@ -88,8 +88,7 @@ def make_directory(full_path, uid, gid, metadata=None):
                                     " path failed (%s)" % (full_path,
                                                            str(serr)))
             else:
-                is_dir = stat.S_ISDIR(stats.st_mode)
-                if not is_dir:
+                if not stat.S_ISDIR(stats.st_mode):
                     # FIXME: Ideally we'd want to return an appropriate error
                     # message and code in the PUT Object REST API response.
                     raise AlreadyExistsAsFile("make_directory:"
@@ -134,8 +133,7 @@ def make_directory(full_path, uid, gid, metadata=None):
                     raise DiskFileError(errmsg)
                 else:
                     # The directory at least exists now
-                    is_dir = stat.S_ISDIR(stats.st_mode)
-                    if is_dir:
+                    if stat.S_ISDIR(stats.st_mode):
                         # Dump the stats to the log with the original exception
                         logging.warn("make_directory: mkdir initially"
                                      " failed on path %s (%s) but a stat()"
@@ -339,8 +337,7 @@ class DiskFileWriter(object):
                                 ' longer exists (targeted for %s)' % (
                                     df._put_datadir, df._data_file))
                         else:
-                            is_dir = stat.S_ISDIR(dfstats.st_mode)
-                            if not is_dir:
+                            if not stat.S_ISDIR(dfstats.st_mode):
                                 raise DiskFileError(
                                     'DiskFile.put(): path to object, %s,'
                                     ' no longer a directory (targeted for'
@@ -387,7 +384,7 @@ class DiskFileWriter(object):
                 df._create_dir_object, df._data_file, metadata)
             return
 
-        if df._is_dir:
+        if df._stat and stat.S_ISDIR(df._stat.st_mode):
             # A pre-existing directory already exists on the file
             # system, perhaps gratuitously created when another
             # object was created, or created externally to Swift
@@ -563,7 +560,6 @@ class DiskFile(object):
         self._threadpool = threadpool or ThreadPool(nthreads=0)
         self._uid = int(uid)
         self._gid = int(gid)
-        self._is_dir = False
         self._metadata = None
         self._fd = None
         # This fd attribute is not used in PUT path. fd used in PUT path
@@ -622,7 +618,6 @@ class DiskFile(object):
         try:
             if not self._stat:
                 self._stat = do_fstat(self._fd)
-            self._is_dir = stat.S_ISDIR(self._stat.st_mode)
             obj_size = self._stat.st_size
 
             if not self._metadata:
@@ -633,7 +628,7 @@ class DiskFile(object):
             assert self._metadata is not None
             self._filter_metadata()
 
-            if self._is_dir:
+            if stat.S_ISDIR(self._stat.st_mode):
                 do_close(self._fd)
                 obj_size = 0
                 self._fd = -1
@@ -1042,7 +1037,7 @@ class DiskFile(object):
         return metadata
 
     def _unlinkold(self):
-        if self._is_dir:
+        if self._stat and stat.S_ISDIR(self._stat.st_mode):
             # Marker, or object, directory.
             #
             # Delete from the filesystem only if it contains no objects.
@@ -1052,7 +1047,7 @@ class DiskFile(object):
             # container or parent directory is deleted.
             #
             # FIXME: Ideally we should use an atomic metadata update operation
-            metadata = read_metadata(self._data_file)
+            metadata = self._metadata or read_metadata(self._data_file)
             if dir_is_object(metadata):
                 metadata[X_OBJECT_TYPE] = DIR_NON_OBJECT
                 write_metadata(self._data_file, metadata)
diff --git a/test/unit/common/test_utils.py b/test/unit/common/test_utils.py
index f88229a..8ba9a2a 100644
--- a/test/unit/common/test_utils.py
+++ b/test/unit/common/test_utils.py
@@ -810,9 +810,19 @@ class TestUtils(unittest.TestCase):
               utils.X_CONTENT_LENGTH: '12345',
               utils.X_TYPE: utils.OBJECT,
               utils.X_OBJECT_TYPE: 'na'}
-        fake_stat = Mock(st_size=12346)
+        fake_stat = Mock(st_size=12346, st_mode=33188)
         self.assertFalse(utils.validate_object(md, fake_stat))
-        fake_stat = Mock(st_size=12345)
+        fake_stat = Mock(st_size=12345, st_mode=33188)
+        self.assertTrue(utils.validate_object(md, fake_stat))
+
+    def test_validate_object_marker_dir(self):
+        md = {utils.X_TIMESTAMP: 'na',
+              utils.X_CONTENT_TYPE: 'application/directory',
+              utils.X_ETAG: 'bad',
+              utils.X_CONTENT_LENGTH: '0',
+              utils.X_TYPE: utils.OBJECT,
+              utils.X_OBJECT_TYPE: utils.DIR_OBJECT}
+        fake_stat = Mock(st_size=4096, st_mode=16744)
         self.assertTrue(utils.validate_object(md, fake_stat))
 
 
diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py
index 72db849..9701f44 100644
--- a/test/unit/obj/test_diskfile.py
+++ b/test/unit/obj/test_diskfile.py
@@ -163,7 +163,6 @@ class TestDiskFile(unittest.TestCase):
         assert gdf._obj_path == ""
         assert gdf._put_datadir == os.path.join(self.td, "vol0", "bar"), gdf._put_datadir
         assert gdf._data_file == os.path.join(self.td, "vol0", "bar", "z")
-        assert gdf._is_dir is False
         assert gdf._fd is None
 
     def test_constructor_leadtrail_slash(self):
@@ -193,10 +192,8 @@ class TestDiskFile(unittest.TestCase):
         assert gdf._fd is None
         assert gdf._disk_file_open is False
         assert gdf._metadata is None
-        assert not gdf._is_dir
         with gdf.open():
             assert gdf._data_file == the_file
-            assert not gdf._is_dir
             assert gdf._fd is not None
             assert gdf._metadata == exp_md
             assert gdf._disk_file_open is True
@@ -224,7 +221,6 @@ class TestDiskFile(unittest.TestCase):
         assert gdf._fd is None
         assert gdf._disk_file_open is False
         assert gdf._metadata is None
-        assert not gdf._is_dir
 
         # Case 1
         # Ensure that reading metadata for non-GET requests
@@ -285,9 +281,7 @@ class TestDiskFile(unittest.TestCase):
         assert gdf._fd is None
         assert gdf._metadata is None
         assert gdf._disk_file_open is False
-        assert not gdf._is_dir
         with gdf.open():
-            assert not gdf._is_dir
             assert gdf._data_file == the_file
             assert gdf._fd is not None
             assert gdf._metadata == exp_md, "%r != %r" % (gdf._metadata, exp_md)
@@ -308,7 +302,6 @@ class TestDiskFile(unittest.TestCase):
         _metadata[_mapit(the_file)] = inv_md
         gdf = self._get_diskfile("vol0", "p57", "ufo47", "bar", "z")
         assert gdf._obj == "z"
-        assert not gdf._is_dir
         assert gdf._fd is None
         assert gdf._disk_file_open is False
         with gdf.open():
@@ -334,10 +327,8 @@ class TestDiskFile(unittest.TestCase):
         del exp_md['X-Object-Type']
         gdf = self._get_diskfile("vol0", "p57", "ufo47", "bar", "d")
         assert gdf._obj == "d"
-        assert gdf._is_dir is False
         assert gdf._disk_file_open is False
         with gdf.open():
-            assert gdf._is_dir
             assert gdf._data_file == the_dir
             assert gdf._disk_file_open is True
         assert gdf._disk_file_open is False
@@ -353,7 +344,6 @@ class TestDiskFile(unittest.TestCase):
             fd.write("y" * fsize)
         gdf = self._get_diskfile(dev, par, acc, con, obj)
         assert gdf._obj == base_obj
-        assert not gdf._is_dir
         assert gdf._fd is None
         return gdf
 
@@ -865,7 +855,6 @@ class TestDiskFile(unittest.TestCase):
         gdf = self._get_diskfile("vol0", "p57", "ufo47", "bar", "z")
         assert gdf._obj == "z"
         assert gdf._data_file == the_file
-        assert not gdf._is_dir
         later = float(gdf.read_metadata()['X-Timestamp']) + 1
         gdf.delete(normalize_timestamp(later))
         assert os.path.isdir(gdf._put_datadir)
@@ -880,7 +869,6 @@ class TestDiskFile(unittest.TestCase):
         gdf = self._get_diskfile("vol0", "p57", "ufo47", "bar", "z")
         assert gdf._obj == "z"
         assert gdf._data_file == the_file
-        assert not gdf._is_dir
         now = float(gdf.read_metadata()['X-Timestamp'])
         gdf.delete(normalize_timestamp(now))
         assert os.path.isdir(gdf._put_datadir)
@@ -895,7 +883,6 @@ class TestDiskFile(unittest.TestCase):
         gdf = self._get_diskfile("vol0", "p57", "ufo47", "bar", "z")
         assert gdf._obj == "z"
         assert gdf._data_file == the_file
-        assert not gdf._is_dir
         later = float(gdf.read_metadata()['X-Timestamp']) + 1
 
         # Handle the case the file is not in the directory listing.
@@ -914,7 +901,6 @@ class TestDiskFile(unittest.TestCase):
         gdf = self._get_diskfile("vol0", "p57", "ufo47", "bar", "z")
         assert gdf._obj == "z"
         assert gdf._data_file == the_file
-        assert not gdf._is_dir
 
         later = float(gdf.read_metadata()['X-Timestamp']) + 1