Change in vdsm[ovirt-3.3.0]: oop: improve safety for truncateFile
fsimonce at redhat.com
fsimonce at redhat.com
Thu Oct 10 09:02:38 UTC 2013
Hello Nir Soffer, Sergey Gotliv, Dan Kenigsberg,
I'd like you to do a code review. Please visit
http://gerrit.ovirt.org/20063
to review the following change.
Change subject: oop: improve safety for truncateFile
......................................................................
oop: improve safety for truncateFile
In order to make truncateFile safer and to avoid any confusion on its
behavior:
* a new comment has been added mentioning O_TRUNC and "w" to avoid any
future mistake in this area
* a new test has been added to check the expected outcomes
* the "w" mode has been removed from truncateFile (used in os.fdopen)
to prevent any future reconversion to open(path, "w")
* the risk of a file descriptor leak (for a failing os.fdopen call)
has been removed using the relevant posix calls
Change-Id: Ib71b53498c7bc4ea7a1ab725feb18bc5929f8c85
Signed-off-by: Federico Simoncelli <fsimonce at redhat.com>
Reviewed-on: http://gerrit.ovirt.org/20046
Reviewed-by: Nir Soffer <nsoffer at redhat.com>
Reviewed-by: Sergey Gotliv <sgotliv at redhat.com>
Reviewed-by: Dan Kenigsberg <danken at redhat.com>
---
M tests/remoteFileHandlerTests.py
M vdsm/storage/remoteFileHandler.py
2 files changed, 39 insertions(+), 3 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/63/20063/1
diff --git a/tests/remoteFileHandlerTests.py b/tests/remoteFileHandlerTests.py
index 544ec28..a2ca574 100644
--- a/tests/remoteFileHandlerTests.py
+++ b/tests/remoteFileHandlerTests.py
@@ -18,6 +18,8 @@
# Refer to the README and COPYING files for full details of the license
#
import os
+import string
+import tempfile
from vdsm import utils
from testrunner import VdsmTestCase as TestCaseBase
@@ -69,3 +71,29 @@
test = lambda: self.assertFalse(os.path.exists(procPath))
utils.retry(test, AssertionError, timeout=4, sleep=0.1)
+
+
+class RemoteFileHandlerFunctionTests(TestCaseBase):
+ def testTruncateFile(self):
+ fd, path = tempfile.mkstemp()
+ try:
+ os.write(fd, string.ascii_uppercase)
+ os.close(fd)
+
+ # Verifying content
+ data = string.ascii_uppercase
+ self.assertEquals(data, file(path).read())
+
+ # Testing truncate to a larger size
+ data = string.ascii_uppercase + chr(0) * 16
+
+ rhandler.truncateFile(path, len(data))
+ self.assertEquals(data, file(path).read())
+
+ # Testing truncate to a smaller size
+ data = string.ascii_uppercase
+
+ rhandler.truncateFile(path, len(data))
+ self.assertEquals(data, file(path).read())
+ finally:
+ os.unlink(path)
diff --git a/vdsm/storage/remoteFileHandler.py b/vdsm/storage/remoteFileHandler.py
index 47b237b..3a22bc4 100644
--- a/vdsm/storage/remoteFileHandler.py
+++ b/vdsm/storage/remoteFileHandler.py
@@ -335,15 +335,23 @@
def truncateFile(path, size, mode=None, creatExcl=False):
+ # NOTE: Under no circumstance you should add the O_TRUNC
+ # flag here. We rely on the fact that the file content is
+ # not deleted when truncating to a larger size.
+ # Please also note that the "w" option used in open/file
+ # contains O_TRUNC and therefore should not be used here.
flags = os.O_CREAT | os.O_WRONLY
+
if creatExcl:
flags |= os.O_EXCL
fd = os.open(path, flags)
- with os.fdopen(fd, 'w') as f:
+ try:
if mode is not None:
- os.chmod(path, mode)
- f.truncate(size)
+ os.fchmod(fd, mode)
+ os.ftruncate(fd, size)
+ finally:
+ os.close(fd)
def readLines(path):
--
To view, visit http://gerrit.ovirt.org/20063
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib71b53498c7bc4ea7a1ab725feb18bc5929f8c85
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: ovirt-3.3.0
Gerrit-Owner: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgotliv at redhat.com>
More information about the vdsm-patches
mailing list