From 69ce9a942ce4343f357304eac623923bda3278d2 Mon Sep 17 00:00:00 2001 From: Patric Schmitz Date: Tue, 28 Jun 2016 00:58:56 +0200 Subject: [PATCH 1/4] move savefile flush into try: block it seems flush might throw which will not be caught inside the (try-) else branch. (hopefully) fixes config corruption when no space available. --- qutebrowser/utils/qtutils.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/qutebrowser/utils/qtutils.py b/qutebrowser/utils/qtutils.py index 4da9d48af..4fe9ade0b 100644 --- a/qutebrowser/utils/qtutils.py +++ b/qutebrowser/utils/qtutils.py @@ -207,12 +207,11 @@ def savefile_open(filename, binary=False, encoding='utf-8'): else: new_f = io.TextIOWrapper(PyQIODevice(f), encoding=encoding) yield new_f + new_f.flush() except: f.cancelWriting() cancelled = True raise - else: - new_f.flush() finally: commit_ok = f.commit() if not commit_ok and not cancelled: From a33978ee89b0384e5a35e81471dc7827a658a58b Mon Sep 17 00:00:00 2001 From: Patric Schmitz Date: Tue, 28 Jun 2016 12:59:00 +0200 Subject: [PATCH 2/4] fix error handling in savefile_open and tests --- qutebrowser/utils/qtutils.py | 14 +++++++++++--- tests/unit/utils/test_qtutils.py | 19 +++++++++++++++---- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/qutebrowser/utils/qtutils.py b/qutebrowser/utils/qtutils.py index 4fe9ade0b..71d0dd12f 100644 --- a/qutebrowser/utils/qtutils.py +++ b/qutebrowser/utils/qtutils.py @@ -197,21 +197,29 @@ def deserialize_stream(stream, obj): def savefile_open(filename, binary=False, encoding='utf-8'): """Context manager to easily use a QSaveFile.""" f = QSaveFile(filename) + open_ok = False + caller_finished = False cancelled = False try: - ok = f.open(QIODevice.WriteOnly) - if not ok: + open_ok = f.open(QIODevice.WriteOnly) + if not open_ok: raise QtOSError(f) if binary: new_f = PyQIODevice(f) else: new_f = io.TextIOWrapper(PyQIODevice(f), encoding=encoding) yield new_f + caller_finished = True new_f.flush() except: f.cancelWriting() cancelled = True - raise + if not open_ok: + raise QtOSError(f, msg="Open failed!") + elif not caller_finished: + raise + else: + raise QtOSError(f, msg="Flush failed!") finally: commit_ok = f.commit() if not commit_ok and not cancelled: diff --git a/tests/unit/utils/test_qtutils.py b/tests/unit/utils/test_qtutils.py index 78c2cffdb..937b1f55f 100644 --- a/tests/unit/utils/test_qtutils.py +++ b/tests/unit/utils/test_qtutils.py @@ -388,7 +388,6 @@ class TestSavefileOpen: def test_mock_open_error(self, qsavefile_mock): """Test with a mock and a failing open().""" qsavefile_mock.open.return_value = False - qsavefile_mock.errorString.return_value = "Hello World" with pytest.raises(OSError) as excinfo: with qtutils.savefile_open('filename'): @@ -396,7 +395,7 @@ class TestSavefileOpen: qsavefile_mock.open.assert_called_once_with(QIODevice.WriteOnly) qsavefile_mock.cancelWriting.assert_called_once_with() - assert str(excinfo.value) == "Hello World" + assert str(excinfo.value) == "Open failed!" def test_mock_exception(self, qsavefile_mock): """Test with a mock and an exception in the block.""" @@ -477,20 +476,32 @@ class TestSavefileOpen: with qtutils.savefile_open(str(filename)): pass errors = ["Filename refers to a directory", # Qt >= 5.4 + "Open failed!", "Commit failed!"] # older Qt versions assert str(excinfo.value) in errors assert tmpdir.listdir() == [filename] + def test_failing_flush(self, tmpdir): + """Test with the file being closed before flushing.""" + filename = tmpdir / 'foo' + with pytest.raises(OSError) as excinfo: + with qtutils.savefile_open(str(filename), binary=True) as f: + f.write(b'Hello') + f.dev.commit() # provoke failing flush + + assert str(excinfo.value) == "Flush failed!" + assert tmpdir.listdir() == [filename] + def test_failing_commit(self, tmpdir): """Test with the file being closed before committing.""" filename = tmpdir / 'foo' with pytest.raises(OSError) as excinfo: with qtutils.savefile_open(str(filename), binary=True) as f: f.write(b'Hello') - f.dev.commit() # provoke failing "real" commit + f.dev.cancelWriting() # provoke failing commit assert str(excinfo.value) == "Commit failed!" - assert tmpdir.listdir() == [filename] + assert tmpdir.listdir() == [] def test_line_endings(self, tmpdir): """Make sure line endings are translated correctly. From 217b912f3c3ed559abdb67c3187ab5c789679ce0 Mon Sep 17 00:00:00 2001 From: Patric Schmitz Date: Tue, 28 Jun 2016 15:03:23 +0200 Subject: [PATCH 3/4] revert custom exception throwing in savefile_open instead adapt test_failing_flush to catch correct exception (ValueError). --- qutebrowser/utils/qtutils.py | 10 +--------- tests/unit/utils/test_qtutils.py | 8 ++++---- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/qutebrowser/utils/qtutils.py b/qutebrowser/utils/qtutils.py index 71d0dd12f..f22cb0f4c 100644 --- a/qutebrowser/utils/qtutils.py +++ b/qutebrowser/utils/qtutils.py @@ -197,8 +197,6 @@ def deserialize_stream(stream, obj): def savefile_open(filename, binary=False, encoding='utf-8'): """Context manager to easily use a QSaveFile.""" f = QSaveFile(filename) - open_ok = False - caller_finished = False cancelled = False try: open_ok = f.open(QIODevice.WriteOnly) @@ -209,17 +207,11 @@ def savefile_open(filename, binary=False, encoding='utf-8'): else: new_f = io.TextIOWrapper(PyQIODevice(f), encoding=encoding) yield new_f - caller_finished = True new_f.flush() except: f.cancelWriting() cancelled = True - if not open_ok: - raise QtOSError(f, msg="Open failed!") - elif not caller_finished: - raise - else: - raise QtOSError(f, msg="Flush failed!") + raise finally: commit_ok = f.commit() if not commit_ok and not cancelled: diff --git a/tests/unit/utils/test_qtutils.py b/tests/unit/utils/test_qtutils.py index 937b1f55f..a3d895842 100644 --- a/tests/unit/utils/test_qtutils.py +++ b/tests/unit/utils/test_qtutils.py @@ -388,6 +388,7 @@ class TestSavefileOpen: def test_mock_open_error(self, qsavefile_mock): """Test with a mock and a failing open().""" qsavefile_mock.open.return_value = False + qsavefile_mock.errorString.return_value = "Hello World" with pytest.raises(OSError) as excinfo: with qtutils.savefile_open('filename'): @@ -395,7 +396,7 @@ class TestSavefileOpen: qsavefile_mock.open.assert_called_once_with(QIODevice.WriteOnly) qsavefile_mock.cancelWriting.assert_called_once_with() - assert str(excinfo.value) == "Open failed!" + assert str(excinfo.value) == "Hello World" def test_mock_exception(self, qsavefile_mock): """Test with a mock and an exception in the block.""" @@ -476,7 +477,6 @@ class TestSavefileOpen: with qtutils.savefile_open(str(filename)): pass errors = ["Filename refers to a directory", # Qt >= 5.4 - "Open failed!", "Commit failed!"] # older Qt versions assert str(excinfo.value) in errors assert tmpdir.listdir() == [filename] @@ -484,12 +484,12 @@ class TestSavefileOpen: def test_failing_flush(self, tmpdir): """Test with the file being closed before flushing.""" filename = tmpdir / 'foo' - with pytest.raises(OSError) as excinfo: + with pytest.raises(ValueError) as excinfo: with qtutils.savefile_open(str(filename), binary=True) as f: f.write(b'Hello') f.dev.commit() # provoke failing flush - assert str(excinfo.value) == "Flush failed!" + assert str(excinfo.value) == "IO operation on closed device!" assert tmpdir.listdir() == [filename] def test_failing_commit(self, tmpdir): From fbac4cb95f664d932bf56d510e7e594300450cbf Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Wed, 27 Jul 2016 10:11:44 +0200 Subject: [PATCH 4/4] Regenerate authors --- README.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.asciidoc b/README.asciidoc index 8a5c7448d..e1b2a8cbc 100644 --- a/README.asciidoc +++ b/README.asciidoc @@ -152,8 +152,8 @@ Contributors, sorted by the number of commits in descending order: * Raphael Pierzina * Joel Torstensson * Jan Verbeek -* Tarcisio Fedrizzi * Patric Schmitz +* Tarcisio Fedrizzi * Claude * Corentin Julé * meles5