Remove QSaveFile in favor of plain old file saving
ClosedPublic

Authored by cullmann on Aug 16 2018, 8:12 PM.

Details

Summary

Rationale: for many use cases that e.g. have acls, complex other extended attributes, static links e.g. the rename() doesnt do the trick it should other ways would be start to add workarounds to all cases, which is hard, e.g. if that is something shared via SMB...

Test Plan

make && make test

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
cullmann created this revision.Aug 16 2018, 8:12 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptAug 16 2018, 8:12 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
cullmann requested review of this revision.Aug 16 2018, 8:12 PM
cullmann added a comment.EditedAug 16 2018, 8:16 PM

Relevant bugs

bug 379818
bug 333577
bug 366583
bug 354405
bug 358457
bug 385596

dfaure added a subscriber: dfaure.Aug 16 2018, 8:33 PM

But what about the issue where you try to save over an existing file, and the partition is full (or the app crashes), and the user loses the file?

Actually, that the application crashs during that actions is highly unlikely, I can't remember any crash during save being reported in the last 5 years.
The partition full is more ugly, that is true.
(and in any case, for sure is direct writing more unsafe than the write in a copy)

But I don't see that we will be able to workaround all the issues people face with the trivial save-as-copy and rename scheme we use.
Actually having the wrong ACLs can be very bad if you have that in your network share and suddenly the rights change.
Same for the static link stuff.
I looked a bit how other editors handle that, but that is more or less very non-trivial (and I am not sure I have seen senseful handling on Windows).

I just tested how vim handles this. No QSaveFile-like behaviour (direct write, says strace), and in case of partition full, this error message appears:

"myfile" E514: write error (file system full?)

WARNING: Original file may be lost or damaged don't quit the editor until the file is successfully written!

That's the beautiful difference between a text editor and a program saving a config file or whatever else: you can leave the editor open so the data is not lost!
I recommend that kate implements something similar, if not already done.

For the curious, here's how I investigated vim's behaviour without filling my real partition:

cd /tmp
dd if=/dev/zero of=loopback count=200
sudo mkfs.ext2 loopback
sudo mount -o loop loopback mnt
sudo chown $USER:$GID mnt
cd mnt
yes > myfile
  # (this ends with "No space left on device")
vim myfile

# elsewhere: strace -e file -p `pidof vim`

in vim, paste a whole bunch of additional text into the file, save

That sounds reasonable.
The hint is actually very good, at the moment in the best case the user get some "saving failed" message like:

<snip>
The document could not be saved, as it was not possible to write to /XML-slowdown-KWRITE.xml.

Check that you have write access to this file or that enough disk space is available.
</snip>

An additional hint the "WARNING: Original file may be lost or damaged don't quit the editor until the file is successfully written!" would make that message more clear for the average user what the implications are.

cfeck retitled this revision from remove QSaveFile in favor of plain old file saving rational: for many use cases that e.g. have acls, complex other extended attributes, static links e.g. the rename() doesnt do the trick it should other ways would be start to add workarounds to... to Remove QSaveFile in favor of plain old file saving.Aug 16 2018, 10:35 PM
cfeck edited the summary of this revision. (Show Details)
dfaure accepted this revision.Aug 17 2018, 7:43 AM
This revision is now accepted and ready to land.Aug 17 2018, 7:43 AM
cullmann updated this revision to Diff 39903.Aug 17 2018, 9:36 AM
  • improve the error message, add vim like hint that one might loose data
  • Merge branch 'master' into nosavefile
cullmann requested review of this revision.Aug 17 2018, 9:40 AM

There is one tiny problem with that change:

The compression filter device crashs sometimes, if you have bad luck, e.g. tried like David did show above with a small tmp mount:

ASSERT: "d->avail_out > 0" in file /home/cullmann/kde/src/frameworks/karchive/src/knonefilter.cpp, line 128

Thread 1 "kwrite" received signal SIGABRT, Aborted.
0x00007ffff156708b in raise () from /lib64/libc.so.6
(gdb) bt
#0 0x00007ffff156708b in raise () from /lib64/libc.so.6
#1 0x00007ffff15504e9 in abort () from /lib64/libc.so.6
#2 0x00007ffff22b022b in QMessageLogger::fatal(char const*, ...) const () from /usr/lib64/libQt5Core.so.5
#3 0x00007ffff22af7d9 in qt_assert(char const*, char const*, int) () from /usr/lib64/libQt5Core.so.5
#4 0x00007ffff04b44fb in KNoneFilter::copyData (this=0x7fffdc0278f0) at /home/cullmann/kde/src/frameworks/karchive/src/knonefilter.cpp:128
#5 0x00007ffff04b44c6 in KNoneFilter::compress (this=0x7fffdc0278f0, finish=true) at /home/cullmann/kde/src/frameworks/karchive/src/knonefilter.cpp:123
#6 0x00007ffff04b277e in KCompressionDevice::writeData (this=0x7fffffffc220, data=0x0, len=0)

at /home/cullmann/kde/src/frameworks/karchive/src/kcompressiondevice.cpp:343

#7 0x00007ffff23ddddd in QIODevice::write(char const*, long long) () from /usr/lib64/libQt5Core.so.5
#8 0x00007ffff04b1f1d in KCompressionDevice::close (this=0x7fffffffc220) at /home/cullmann/kde/src/frameworks/karchive/src/kcompressiondevice.cpp:165
#9 0x00007ffff77ee7fd in Kate::TextBuffer::save (this=0x72a930, filename=...)

at /home/cullmann/kde/src/frameworks/ktexteditor/src/buffer/katetextbuffer.cpp:858

I think the integration must wait until we resolve the KArchive issue.
I have taken a short look but seen no "trivial" fix, opened a new bug for that, https://bugs.kde.org/show_bug.cgi?id=397545

cullmann updated this revision to Diff 39921.Aug 17 2018, 1:15 PM

skip filter dev hanlding in the common case of no compression
compress in memory to the buffer for the kauth case

this actually leads now to the wanted error message for uncompressed files
in disk full situations without any segfault

for the special case of compressed files I opened bug 397545

  • Merge branch 'master' into nosavefile

Fix for KCompressionDevice + addendum to this patch posted on https://bugs.kde.org/show_bug.cgi?id=397545

cullmann updated this revision to Diff 39956.Aug 18 2018, 5:53 AM
  • better error handling, take a look at the file error string
dhaumann accepted this revision.Aug 18 2018, 9:11 AM

Looks reasonable to me.

This revision is now accepted and ready to land.Aug 18 2018, 9:11 AM
This revision was automatically updated to reflect the committed changes.
cullmann reopened this revision.Aug 18 2018, 9:31 AM

The errorString().isEmpty doesn't work, you get unknown error even for the non-errors cases.
QFile would allow to check for the set errorCode.

This revision is now accepted and ready to land.Aug 18 2018, 9:31 AM

Atm reverted the last addition of

// did save work?
  • if (!saveFile->errorString().isEmpty()) {
  • BUFFER_DEBUG << "Saving file " << filename << "failed with error" << saveFile->errorString();
  • return false;
  • }

+ FIXME if (!saveFile->errorString().isEmpty()) {
+
BUFFER_DEBUG << "Saving file " << filename << "failed with error" << saveFile->errorString();
+ return false;
+
}

For the QFile case, I could query http://doc.qt.io/qt-5/qfiledevice.html#error, but for the FilterDev case I have no such function available, or?

cullmann planned changes to this revision.Aug 18 2018, 9:45 AM

Yeah that's the problem.

The way I see it, the QIODevice API assumes that one will call errorString() only after some method returns an error, e.g. after QAbstractSocket::error() is emitted, or after socket.waitForEncrypted() returns false.

QFile[Device] and QAbstractSocket add their own errorcode enums on top of that, but that's not part of the base QIODevice API, which is all we have in KCompressionDevice.

Ideally QIODevice::close() would return a bool, and I'm more and more thinking that this should be done for Qt 6.
But pending that, I'm afraid that we'll have to downcast to KCompressionDevice to call a new error() accessor after close()...

For the QFile case, I added now this check:

// did save work?
  // FIXME for KCompressionDevice
  if (qobject_cast<QFileDevice *>(saveFile.data()) && qobject_cast<QFileDevice *>(saveFile.data())->error() != QFileDevice::NoError) {
      BUFFER_DEBUG << "Saving file " << filename << "failed with error" << saveFile->errorString();
      return false;
  }

that seems to handle the QFile case well.

(the alternative is to use Unbuffered on those devices, but that's probably not a great idea performance wise)

But pending that, I'm afraid that we'll have to downcast to KCompressionDevice to call a new error() accessor after close()...

I can live with that, see comment above, I already added the QFileDevice case.
It is not that nice but at least one can handle the error.
If you add the missing API, I will adapt the code here.

I'll look into it. I also just reported https://bugreports.qt.io/browse/QTBUG-70033

Just wanted to add the alternative code path, might it be that the Q_OBJECT stuff is really missing by accident? Or is this intentional?

/usr/include/qt5/QtCore/qobject.h: In instantiation of ‘T qobject_cast(QObject*) [with T = KCompressionDevice*]’:
/home/cullmann/kde/src/frameworks/ktexteditor/src/buffer/katetextbuffer.cpp:863:82: required from here
/usr/include/qt5/QtCore/qobject.h:502:5: error: static assertion failed: qobject_cast requires the type to have a Q_OBJECT macro

Q_STATIC_ASSERT_X(QtPrivate::HasQ_OBJECT_Macro<ObjType>::Value,

Oops fixed.

BTW, doing a qobject_cast twice (for each type) seems a bit wasteful, you might want to split the if() like I did in KArchive...

My local patch already does that :)

This revision was not accepted when it landed; it landed in state Changes Planned.Aug 18 2018, 11:56 AM
This revision was automatically updated to reflect the committed changes.