hi,
this is not fixing the root of #6587 but at least we add some
safety measures. since its touching sensitive area of code,
i'd like to ask for review before committing.
to make a brief summary ext4 has long time window between truncating
file in journal and actually moving the actual data on disk.
if your computer collapses during this time, you
1. have 0 sized .lyx file
2. because we delete backup and then copy newer one on the position of backup
we get 0 sized backup
3. because we remove autosave just after saving even autosave is lost.
the attached:
fixes point 2 - use move instead of copy.
fixes point 3 - remove autosave only on closing file.
both tested by the original reporter, backup and autosave survived.
i have a small doubt whether this snippet
// Saving failed, so backup is not backup
if (madeBackup)
- backupName.moveTo(d->filename);
+ backupName.copyTo(d->filename);
wouldnt be better this way
// Saving failed, so backup is not backup
if (madeBackup)
backupName.moveTo(d->filename);
+ d->filename.copyTo(d->backupName);
pavel
just as btw
for the point 1, i dont see any nice fix. the possible solutions would be using
a) plain C routines fopen,fsync,fclose after we saved the file
b) use stdio_filebuf to obtain descriptor from stream and call fsync on it
both "solutions" are not crossplatform and i hate both of them.
ANSI C do not provide any fsync mechanism, C++ Standard wont even provide
way how to get file descriptor from stream.
if somebody has better idea for fixes i'm all ears.
there have been big flames in kernel folks, which finally forced ext4 devs
to add special heuristic code into 2.6.30 kernels. i was said that it should
prevent this kind of happening for normal files, but our reporter denied it
by examining on 2.6.31 :(
as a private edification - go from ext4 as far as possible, because this kind
of things must happen on thousand and one c++ apps.
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 1da4d99..fc0669d 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -975,6 +975,17 @@ Buffer::ReadStatus Buffer::readFile(Lexer & lex, FileName
const & filename,
// Should probably be moved to somewhere else: BufferView? GuiView?
bool Buffer::save() const
{
+ // ask if the disk file has been externally modified (use checksum
method)
+ if (fileName().exists() && isExternallyModified(checksum_method)) {
+ docstring const file = makeDisplayPath(absFileName(), 20);
+ docstring text = bformat(_("Document %1$s has been externally
modified. Are you sure "
+ "you want to
overwrite this file?"), file);
+ int const ret = Alert::prompt(_("Overwrite modified file?"),
+ text, 1, 1, _("&Overwrite"), _("&Cancel"));
+ if (ret == 1)
+ return false;
+ }
+
// We don't need autosaves in the immediate future. (Asger)
resetAutosaveTimers();
@@ -990,7 +1001,7 @@ bool Buffer::save() const
backupName = FileName(addName(lyxrc.backupdir_path,
mangledName));
}
- if (fileName().copyTo(backupName)) {
+ if (fileName().moveTo(backupName)) {
madeBackup = true;
} else {
Alert::error(_("Backup failure"),
@@ -1001,24 +1012,13 @@ bool Buffer::save() const
}
}
- // ask if the disk file has been externally modified (use checksum
method)
- if (fileName().exists() && isExternallyModified(checksum_method)) {
- docstring const file = makeDisplayPath(absFileName(), 20);
- docstring text = bformat(_("Document %1$s has been externally
modified. Are you sure "
- "you want to
overwrite this file?"), file);
- int const ret = Alert::prompt(_("Overwrite modified file?"),
- text, 1, 1, _("&Overwrite"), _("&Cancel"));
- if (ret == 1)
- return false;
- }
-
if (writeFile(d->filename)) {
markClean();
return true;
} else {
// Saving failed, so backup is not backup
if (madeBackup)
- backupName.moveTo(d->filename);
+ backupName.copyTo(d->filename);
return false;
}
}
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index fc0669d..813a12b 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -1050,7 +1050,8 @@ bool Buffer::writeFile(FileName const & fname) const
return false;
}
- removeAutosaveFile();
+ // see bug 6587
+ // removeAutosaveFile();
saveCheckSum(d->filename);
message(str + _(" done."));
diff --git a/src/frontends/qt4/GuiView.cpp b/src/frontends/qt4/GuiView.cpp
index 09dae40..62c3b56 100644
--- a/src/frontends/qt4/GuiView.cpp
+++ b/src/frontends/qt4/GuiView.cpp
@@ -2304,6 +2304,7 @@ bool GuiView::closeBuffer(Buffer & buf)
guiApp->gotoBookmark(i+1, false, false);
if (saveBufferIfNeeded(buf, false)) {
+ buf.removeAutosaveFile();
theBufferList().release(&buf);
return true;
}
@@ -2376,7 +2377,8 @@ bool GuiView::saveBufferIfNeeded(Buffer & buf, bool
hiding)
// if we crash after this we could
// have no autosave file but I guess
// this is really improbable (Jug)
- buf.removeAutosaveFile();
+ // Sometime improbable things happen, bug 6857 (ps)
+ // buf.removeAutosaveFile();
if (hiding)
// revert all changes
buf.reload();
