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 <fsimo...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Saggi Mizrahi <smizr...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Yeela Kaplan <ykap...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches