Change in vdsm[master]: oop: improve safety for truncateFile
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
Change in vdsm[master]: oop: improve safety for truncateFile
Dan Kenigsberg has posted comments on this change. Change subject: oop: improve safety for truncateFile .. Patch Set 1: Code-Review+1 (1 comment) 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) So this bug is as old as http://gerrit.ovirt.org/14588 and should be backported to ovirt-3.3.0, too. Right? 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 -- 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
Change in vdsm[master]: oop: improve safety for truncateFile
Dan Kenigsberg has posted comments on this change. Change subject: oop: improve safety for truncateFile .. Patch Set 1: I think that the commit message should explain which user flow exposes this severe bug, so that our users can tell why they have to upgrade. -- 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: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: oop: improve safety for truncateFile
Sergey Gotliv has posted comments on this change. Change subject: oop: improve safety for truncateFile .. Patch Set 1: Code-Review+1 Since this patch cost me 2 destroyed VMs I definitely will give it +1. -- 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: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: oop: improve safety for truncateFile
Sergey Gotliv has posted comments on this change. Change subject: oop: improve safety for truncateFile .. Patch Set 1: (1 comment) 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) Yes. This bug is revealed in Online Disk Resize flow which is a part of 3.3. 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 -- 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
Change in vdsm[master]: oop: improve safety for truncateFile
Dan Kenigsberg has posted comments on this change. Change subject: oop: improve safety for truncateFile .. Patch Set 1: Code-Review+2 (1 comment) 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) This should be communicated loudly and clearly. 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 -- 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
Change in vdsm[master]: oop: improve safety for truncateFile
Federico Simoncelli has uploaded a new change for review. 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 fsimo...@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/46/20046/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 f01461f..5b24053 100644 --- a/vdsm/storage/remoteFileHandler.py +++ b/vdsm/storage/remoteFileHandler.py @@ -348,15 +348,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/20046 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ib71b53498c7bc4ea7a1ab725feb18bc5929f8c85 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Federico Simoncelli fsimo...@redhat.com ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: oop: improve safety for truncateFile
Federico Simoncelli has posted comments on this change. Change subject: oop: improve safety for truncateFile .. Patch Set 1: Verified+1 Verified using the provided unit test. -- 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: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: Yeela Kaplan ykap...@redhat.com Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: oop: improve safety for truncateFile
oVirt Jenkins CI Server has posted comments on this change. Change subject: oop: improve safety for truncateFile .. Patch Set 1: Build Successful http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/4886/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4001/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/4811/ : SUCCESS -- 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: 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: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: oop: improve safety for truncateFile
Dan Kenigsberg has posted comments on this change. Change subject: oop: improve safety for truncateFile .. Patch Set 1: (1 comment) 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) I don't understand the context of this patch. w has been used well before the recent http://gerrit.ovirt.org/#/c/19022/1/vdsm/storage/remoteFileHandler.py 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 -- 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: 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
