From 72dbd476f0234b9d050ad87b29a8a0aa58a18a7c Mon Sep 17 00:00:00 2001 From: Prashanth Pai Date: Thu, 19 May 2016 12:08:28 +0530 Subject: [PATCH] Don't (f)chown when it has no effect For (f)chown calls which can change both UID and GID at once, -1 is reserved as a sentinel value to indicate "omitted argument" or "do not change". This makes sense when one of the args to (f)chown is -1. When both uid and gid args are -1, it doesn't make sense to call (f)chown as neither is going to be changed. Further, as of today, diskfile doesn't get the information (uid and gid) of the authenticated user from auth middleware. Retained the calls in code for future when such functionality might be added. This is a port of the following change: http://review.gluster.org/#/c/13778 Change-Id: I74f1fa50bdfea30b404ac941b4345d8a94d5d141 Signed-off-by: Prashanth Pai --- swiftonfile/swift/obj/diskfile.py | 14 ++++++++-- test/unit/obj/test_diskfile.py | 46 +++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 2 deletions(-) diff --git a/swiftonfile/swift/obj/diskfile.py b/swiftonfile/swift/obj/diskfile.py index a549327..9f57d3f 100644 --- a/swiftonfile/swift/obj/diskfile.py +++ b/swiftonfile/swift/obj/diskfile.py @@ -166,7 +166,12 @@ def make_directory(full_path, uid, gid, metadata=None): # We created it, so we are reponsible for always setting the proper # ownership. - do_chown(full_path, uid, gid) + if not ((uid == DEFAULT_UID) and (gid == DEFAULT_GID)): + # If both UID and GID is -1 (default values), it has no effect. + # So don't do a chown. + # Further, at the time of this writing, UID and GID information + # is not passed to DiskFile. + do_chown(full_path, uid, gid) return True, metadata @@ -1019,7 +1024,12 @@ class DiskFile(object): raise DiskFileNoSpace() raise # Ensure it is properly owned before we make it available. - do_fchown(fd, self._uid, self._gid) + if not ((self._uid == DEFAULT_UID) and (self._gid == DEFAULT_GID)): + # If both UID and GID is -1 (default values), it has no effect. + # So don't do a fchown. + # Further, at the time of this writing, UID and GID information + # is not passed to DiskFile. + do_fchown(fd, self._uid, self._gid) dw = DiskFileWriter(fd, tmppath, self) # It's now the responsibility of DiskFileWriter to close this fd. fd = None diff --git a/test/unit/obj/test_diskfile.py b/test/unit/obj/test_diskfile.py index bb37cd8..a01aa0b 100644 --- a/test/unit/obj/test_diskfile.py +++ b/test/unit/obj/test_diskfile.py @@ -1104,3 +1104,49 @@ class TestDiskFile(unittest.TestCase): self.assertFalse(gdf._fd) # Close the actual fd, as we had mocked do_close os.close(_m_do_close.call_args[0][0]) + + def make_directory_chown_call(self): + path = os.path.join(self.td, "a/b/c") + _m_do_chown = Mock() + with patch("swiftonfile.swift.obj.diskfile.do_chown", _m_do_chown): + diskfile.make_directory(path, -1, -1) + self.assertFalse(_m_do_chown.called) + self.assertTrue(os.path.isdir(path)) + + path = os.path.join(self.td, "d/e/f") + _m_do_chown.reset_mock() + with patch("swiftonfile.swift.obj.diskfile.do_chown", _m_do_chown): + diskfile.make_directory(path, -1, 99) + self.assertEqual(_m_do_chown.call_count, 3) + self.assertTrue(os.path.isdir(path)) + + path = os.path.join(self.td, "g/h/i") + _m_do_chown.reset_mock() + with patch("swiftonfile.swift.obj.diskfile.do_chown", _m_do_chown): + diskfile.make_directory(path, 99, -1) + self.assertEqual(_m_do_chown.call_count, 3) + self.assertTrue(os.path.isdir(path)) + + def test_fchown_not_called_on_default_uid_gid_values(self): + the_cont = os.path.join(self.td, "vol0", "ufo47", "bar") + os.makedirs(the_cont) + body = '1234' + metadata = { + 'X-Timestamp': '1234', + 'Content-Type': 'file', + 'ETag': md5(body).hexdigest(), + 'Content-Length': len(body), + } + + _m_do_fchown = Mock() + gdf = self._get_diskfile("vol0", "p57", "ufo47", "bar", "z") + with gdf.create() as dw: + assert dw._tmppath is not None + tmppath = dw._tmppath + dw.write(body) + with patch("swiftonfile.swift.obj.diskfile.do_fchown", + _m_do_fchown): + dw.put(metadata) + self.assertFalse(_m_do_fchown.called) + assert os.path.exists(gdf._data_file) + assert not os.path.exists(tmppath)