| Summary: | Assertion on write error (e.g. disk full situation) for KCompressionDevice | ||
|---|---|---|---|
| Product: | [Frameworks and Libraries] frameworks-karchive | Reporter: | Christoph Cullmann <christoph> |
| Component: | general | Assignee: | David Faure <faure> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | christoph, kdelibs-bugs |
| Priority: | NOR | ||
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| Latest Commit: | https://commits.kde.org/karchive/f013beaa1a86c2ce06c9d63cf2e5507be1d02e0f | Version Fixed In: | |
| Sentry Crash Report: | |||
|
Description
Christoph Cullmann
2018-08-17 10:37:36 UTC
Here when I do `kate /tmp/mnt/test.gz` and I write a ton of text and then save, I don't get any assert, but also no error and a zero bytes file on disk! kcompressiondevice.cpp:173 calls QFile::close (which is where the writing actually happens) but doesn't check for error... (gdb) p d->filter->device()->errorString() $21 = "No space left on device" All this checking of write calls... when in fact there's buffering everywhere nowadays and it all happens on close, which doesn't even return a bool :-) KArchive fix: https://phabricator.kde.org/D14913 ktexteditor fix on top of D14890: http://www.davidfaure.fr/2018/check_save.diff Did you try it with my patch from https://phabricator.kde.org/D14890 applied? For me it only happens with the None filter variant of the KDevFilter, which is worked around in that. Could you try with some non-gz file but without the shortcut in the line QScopedPointer<QIODevice> saveFile((type == KCompressionDevice::None) ? static_cast<QIODevice*>(new QFile(filename)) : static_cast<QIODevice*>(new KCompressionDevice(filename, type))); aka always using the KCompressionDevice? Git commit b0b2ee5a149356515beff46b3b0591ea71121bc0 by David Faure. Committed on 18/08/2018 at 11:02. Pushed by dfaure into branch 'master'. KCompressionDevice: propagate errors from QIODevice::close() Summary: QFile::close() doesn't return a value, but sets an error string instead, we need to check for that and set it in KCompressionDevice so apps can check for that in turn. Test Plan: Saving into /tmp/mnt/test.gz with kate, with the setup described in https://phabricator.kde.org/D14890 Reviewers: cullmann Reviewed By: cullmann Subscribers: kde-frameworks-devel Tags: #frameworks Differential Revision: https://phabricator.kde.org/D14913 M +35 -1 autotests/kcompressiondevicetest.cpp M +3 -0 autotests/kcompressiondevicetest.h M +36 -5 src/kcompressiondevice.cpp M +11 -1 src/kcompressiondevice.h https://commits.kde.org/karchive/b0b2ee5a149356515beff46b3b0591ea71121bc0 Ah, I failed to see the comment about the avail_out assert only happening with NoneFilter. Reopening, since that's still to be fixed. Still can't reproduce the assert though. In ktexteditor/src/buffer/katetextbuffer.cpp I do - QScopedPointer<QIODevice> saveFile((type == KCompressionDevice::None) ? static_cast<QIODevice*>(new QFile(filename)) : static_cast<QIODevice*>(new KCompressionDevice(filename, type))); + QScopedPointer<QIODevice> saveFile(new KCompressionDevice(filename, type)); And then kwrite /tmp/mnt/foobar, paste a huge amount of text, save. No error, 0 bytes file (because I don't have your KCompressionDevice error handling in yet). Ah I see, one needs to paste a REALLY huge amount of text ;) I'll debug it. :=) Yeah, its a really nice corner case issue. One solution would be to test stream.status() inside the loop in Kate::TextBuffer::save, since that (now) correctly gets set to WriteFailed. Or at least after the loop, before calling KCompressionDevice::close. But https://phabricator.kde.org/D14920 fixes that anyhow. Git commit f013beaa1a86c2ce06c9d63cf2e5507be1d02e0f by David Faure. Committed on 18/08/2018 at 14:02. Pushed by dfaure into branch 'master'. KCompressionDevice: don't call write after WriteError Summary: if an earlier call to write failed, then there is no point in doing the final write call, it even asserts in the NoneFilter. Test Plan: This fixes saving to full disk using a KCompressionDevice set up with KNoneFilter, in kwrite. Reviewers: cullmann Reviewed By: cullmann Subscribers: kde-frameworks-devel Tags: #frameworks Differential Revision: https://phabricator.kde.org/D14920 M +1 -2 src/kcompressiondevice.cpp https://commits.kde.org/karchive/f013beaa1a86c2ce06c9d63cf2e5507be1d02e0f Thanks for the fix. I removed again my shortcut around the KCompressionDevice for compression == None. Git commit 50d853c42025fca2128b46642cb7ab041d51d9d8 by Christoph Cullmann. Committed on 18/08/2018 at 14:24. Pushed by cullmann into branch 'master'. simplify error checking, always use the KCompressionDevice now that the assert/crash is fixed early out on error in line writing loop Differential Revision: https://phabricator.kde.org/D14890 M +17 -20 src/buffer/katetextbuffer.cpp https://commits.kde.org/ktexteditor/50d853c42025fca2128b46642cb7ab041d51d9d8 Code is simpler again and I added the early out in the loop for line writing, no reason to continue there after first error, good hint, thanks. |