Change in vdsm[master]: oop: improve safety for truncateFile

nsoffer at redhat.com nsoffer at redhat.com
Thu Oct 10 08:25:33 UTC 2013


Nir Soffer has posted comments on this change.

Change subject: oop: improve safety for truncateFile
......................................................................


Patch Set 1: Code-Review+1

(7 comments)

The change is good and important, and having a test for this is even more important.

However I would prefer that you didn't submit this without cleaning up the tests :-)

....................................................
Commit Message
Line 12: * a new comment has been added mentioning O_TRUNC and "w" to avoid any
Line 13:   future mistake in this area
Line 14: * a new test has been added to check the expected outcomes
Line 15: * the "w" mode has been removed from truncateFile (used in os.fdopen)
Line 16:   to prevent any future reconversion to open(path, "w")
Using open(path, 'w') to truncate a file was a severe bug, causing truncation of the file data to zero before truncating the file to the given size.
Line 17: * the risk of a file descriptor leak (for a failing os.fdopen call)
Line 18:   has been removed using the relevant posix calls
Line 19: 
Line 20: Change-Id: Ib71b53498c7bc4ea7a1ab725feb18bc5929f8c85


....................................................
File tests/remoteFileHandlerTests.py
Line 73:         utils.retry(test, AssertionError, timeout=4, sleep=0.1)
Line 74: 
Line 75: 
Line 76: class RemoteFileHandlerFunctionTests(TestCaseBase):
Line 77:     def testTruncateFile(self):
Rename to testTruncateFileDefaults - this test does not test the mode option or the new exclusive option.
Line 78:         fd, path = tempfile.mkstemp()
Line 79:         try:
Line 80:             os.write(fd, string.ascii_uppercase)
Line 81:             os.close(fd)


Line 76: class RemoteFileHandlerFunctionTests(TestCaseBase):
Line 77:     def testTruncateFile(self):
Line 78:         fd, path = tempfile.mkstemp()
Line 79:         try:
Line 80:             os.write(fd, string.ascii_uppercase)
I would use self descriptive test data:
    olddata = "old data"
    os.write(fd, olddata)

And shouldn't we wait until the data reach the disk before testing it?

    os.fsync(fd)
Line 81:             os.close(fd)
Line 82: 
Line 83:             # Verifying content
Line 84:             data = string.ascii_uppercase


Line 77:     def testTruncateFile(self):
Line 78:         fd, path = tempfile.mkstemp()
Line 79:         try:
Line 80:             os.write(fd, string.ascii_uppercase)
Line 81:             os.close(fd)
If os.write raises, we would leak the file descriptor. Probably not critical in a test.
Line 82: 
Line 83:             # Verifying content
Line 84:             data = string.ascii_uppercase
Line 85:             self.assertEquals(data, file(path).read())


Line 84:             data = string.ascii_uppercase
Line 85:             self.assertEquals(data, file(path).read())
Line 86: 
Line 87:             # Testing truncate to a larger size
Line 88:             data = string.ascii_uppercase + chr(0) * 16
Use new variable for new data?
Line 89: 
Line 90:             rhandler.truncateFile(path, len(data))
Line 91:             self.assertEquals(data, file(path).read())
Line 92: 


Line 90:             rhandler.truncateFile(path, len(data))
Line 91:             self.assertEquals(data, file(path).read())
Line 92: 
Line 93:             # Testing truncate to a smaller size
Line 94:             data = string.ascii_uppercase
Should be separate test - if truncating to bigger size fails, the second test will not run.
Line 95: 
Line 96:             rhandler.truncateFile(path, len(data))
Line 97:             self.assertEquals(data, file(path).read())
Line 98:         finally:


....................................................
File vdsm/storage/remoteFileHandler.py
Line 363:         if mode is not None:
Line 364:             os.fchmod(fd, mode)
Line 365:         os.ftruncate(fd, size)
Line 366:     finally:
Line 367:         os.close(fd)
Good change
Line 368: 
Line 369: 
Line 370: def readLines(path):
Line 371:     with open(path, "r") as f:


-- 
To view, visit http://gerrit.ovirt.org/20046
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib71b53498c7bc4ea7a1ab725feb18bc5929f8c85
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgotliv at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list