From bf782ee02ac35d575247bf63eabbd38bd31c53af Mon Sep 17 00:00:00 2001 From: Georg Baum Date: Mon, 9 Jun 2014 11:08:24 +0200 Subject: [PATCH] Improve file saving strategy - The TempFile class guarantees to generate a file name, we are not limited to 100 tries of a predictable scheme anymore, which could break if LyX frequently crashes. - The temp file name generation has no race condition against another LyX instance in the same directory anymore. - Symlinks survive saving again (regression of 10364082c835). --- src/Buffer.cpp | 33 +++++++++++---------------------- src/support/FileName.cpp | 23 +++++++++++++++++++++-- src/support/FileName.h | 13 ++++++++++++- src/support/TempFile.cpp | 5 +++++ src/support/TempFile.h | 7 +++++++ 5 files changed, 56 insertions(+), 25 deletions(-) diff --git a/src/Buffer.cpp b/src/Buffer.cpp index 7bc75e4c60..866e4dcc36 100644 --- a/src/Buffer.cpp +++ b/src/Buffer.cpp @@ -1284,25 +1284,8 @@ bool Buffer::save() const // we first write the file to a new name, then move it to its // proper location once that has been done successfully. that // way we preserve the original file if something goes wrong. - string const savepath = fileName().onlyPath().absFileName(); - int fnum = 1; - string const fname = fileName().onlyFileName(); - string savename = "tmp-" + convert(fnum) + "-" + fname; - FileName savefile(addName(savepath, savename)); - while (savefile.exists()) { - // surely that is enough tries? - if (fnum > 100) { - Alert::error(_("Write failure"), - bformat(_("Cannot find temporary filename for:\n %1$s.\n" - "Even %2$s exists!"), - from_utf8(fileName().absFileName()), - from_utf8(savefile.absFileName()))); - return false; - } - fnum += 1; - savename = "tmp-" + convert(fnum) + "-" + fname; - savefile.set(addName(savepath, savename)); - } + TempFile tempfile(fileName().onlyPath(), "tmpXXXXXX.lyx"); + FileName savefile(tempfile.name()); LYXERR(Debug::FILES, "Saving to " << savefile.absFileName()); if (!writeFile(savefile)) @@ -1310,6 +1293,7 @@ bool Buffer::save() const // we will set this to false if we fail bool made_backup = true; + bool const symlink = fileName().isSymLink(); if (lyxrc.make_backup) { FileName backupName(absFileName() + '~'); if (!lyxrc.backupdir_path.empty()) { @@ -1321,7 +1305,7 @@ bool Buffer::save() const // Except file is symlink do not copy because of #6587. // Hard links have bad luck. - made_backup = fileName().isSymLink() ? + made_backup = symlink ? fileName().copyTo(backupName): fileName().moveTo(backupName); @@ -1333,8 +1317,13 @@ bool Buffer::save() const //LYXERR(Debug::DEBUG, "Fs error: " << fe.what()); } } - - if (made_backup && savefile.moveTo(fileName())) { + + // If we have no symlink, we can simply rename the temp file. + // Otherwise, we need to copy it so the symlink stays intact. + if (!symlink) + tempfile.setAutoRemove(false); + if (made_backup && + (symlink ? savefile.copyTo(fileName(), true) : savefile.moveTo(fileName()))) { markClean(); return true; } diff --git a/src/support/FileName.cpp b/src/support/FileName.cpp index 47d66699e7..08d726e0c6 100644 --- a/src/support/FileName.cpp +++ b/src/support/FileName.cpp @@ -217,9 +217,26 @@ void FileName::erase() } -bool FileName::copyTo(FileName const & name) const +bool FileName::copyTo(FileName const & name, bool keepsymlink) const { - LYXERR(Debug::FILES, "Copying " << name); + FileNameSet visited; + visited.insert(*this); + return copyTo(name, keepsymlink, visited); +} + + +bool FileName::copyTo(FileName const & name, bool keepsymlink, + FileName::FileNameSet & visited) const +{ + LYXERR(Debug::FILES, "Copying " << name << " keep symlink: " << keepsymlink); + if (keepsymlink && name.isSymLink()) { + FileName const target(fromqstr(name.d->fi.symLinkTarget())); + if (visited.find(target) != visited.end()) { + LYXERR(Debug::FILES, "Found circular symlink: " << target); + return false; + } + return copyTo(target, true); + } QFile::remove(name.d->fi.absoluteFilePath()); bool success = QFile::copy(d->fi.absoluteFilePath(), name.d->fi.absoluteFilePath()); if (!success) @@ -231,6 +248,7 @@ bool FileName::copyTo(FileName const & name) const bool FileName::renameTo(FileName const & name) const { + LYXERR(Debug::FILES, "Renaming " << name); bool success = QFile::rename(d->fi.absoluteFilePath(), name.d->fi.absoluteFilePath()); if (!success) LYXERR0("Could not rename file " << *this << " to " << name); @@ -240,6 +258,7 @@ bool FileName::renameTo(FileName const & name) const bool FileName::moveTo(FileName const & name) const { + LYXERR(Debug::FILES, "Moving " << name); QFile::remove(name.d->fi.absoluteFilePath()); bool success = QFile::rename(d->fi.absoluteFilePath(), diff --git a/src/support/FileName.h b/src/support/FileName.h index fe15163889..1b37ef2629 100644 --- a/src/support/FileName.h +++ b/src/support/FileName.h @@ -16,6 +16,7 @@ #include "support/strfwd.h" #include +#include namespace lyx { @@ -123,7 +124,9 @@ public: /// \return true when file/directory is writable (write test file) /// \warning This methods has different semantics when system level /// copy command, it will overwrite the \c target file if it exists, - bool copyTo(FileName const & target) const; + /// If \p keepsymlink is true, the copy will be written to the symlink + /// target. Otherwise, the symlink will be destroyed. + bool copyTo(FileName const & target, bool keepsymlink = false) const; /// remove pointed file. /// \return true on success. @@ -136,6 +139,8 @@ public: bool renameTo(FileName const & target) const; /// move pointed file to \param target. + /// If \p target exists it will be overwritten (if it is a symlink, + /// the symlink will be destroyed). /// \return true on success. bool moveTo(FileName const & target) const; @@ -212,6 +217,12 @@ public: private: friend bool equivalent(FileName const &, FileName const &); + /// Set for tracking of already visited file names. + /// Uses operator==() (which may be case insensitive), and not + /// equvalent(), so that symlinks are not resolved. + typedef std::set FileNameSet; + /// Helper for public copyTo() to find circular symlink chains + bool copyTo(FileName const &, bool, FileNameSet &) const; /// struct Private; Private * const d; diff --git a/src/support/TempFile.cpp b/src/support/TempFile.cpp index 5c4b389b84..21559e144f 100644 --- a/src/support/TempFile.cpp +++ b/src/support/TempFile.cpp @@ -74,5 +74,10 @@ FileName TempFile::name() const } +void TempFile::setAutoRemove(bool autoremove) +{ + d->f.setAutoRemove(autoremove); +} + } // namespace support } // namespace lyx diff --git a/src/support/TempFile.h b/src/support/TempFile.h index c9342f91f2..6410dda7fc 100644 --- a/src/support/TempFile.h +++ b/src/support/TempFile.h @@ -46,6 +46,13 @@ public: * This is empty if the file could not be created. */ FileName name() const; + /** + * Set whether the file should be automatically deleted in the + * destructor. + * Automatic deletion is the default, but it can be switched off if + * the file should be kept, because it should be renamed afterwards. + */ + void setAutoRemove(bool autoremove); private: /// struct Private; -- 2.39.2