From a324c6e5cdfad77e8f91ec9869deb6b78425807e Mon Sep 17 00:00:00 2001 From: Prashanth Pai Date: Thu, 19 May 2016 15:33:07 +0530 Subject: 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 Reviewed-on: http://review.gluster.org/14423 Reviewed-by: Thiago da Silva Tested-by: Thiago da Silva --- gluster/swift/common/utils.py | 9 ++++++--- gluster/swift/obj/diskfile.py | 19 +++++++------------ test/unit/common/test_utils.py | 14 ++++++++++++-- test/unit/obj/test_diskfile.py | 14 -------------- 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 -- cgit