On Sunday 16 October 2011 13:37:48 Oswald Buddenhagen wrote:
> so maybe filesaver should be just folded into qfile? how to
> activate that mode? setLatchErrors(bool)?
I had another look at this issue, and I need your input.
Were you thinking of having the whole functionality in QFile?
In that email thread from october I wasn't, but now I wanted to give it a try.
I added this API to QFile:
bool useTemporaryFile() const;
void setUseTemporaryFile(bool useTemp);
void rollback();
bool commit();
and sorted out "the internal name given to the file engine is different from
the
publically visible name" by changing the implementation of fileName().
The main issue is opening the temp file in open(), rather than the target
filename. QTemporaryFile has the whole logic of "opening the file itself and
keeping it open in order to prevent race conditions", rather than the unsafe
mktemp approach of "just give me a filename". I found a way though: stealing
the fileEngine from the QTemporaryFile object after it opens the file, see
patch.
This leads to a WIP patch like the one attached -- it has TODOs and whitespace
issues and probably style issues, please try to overlook those. The question
is mainly: overall, does an API like this seem OK to you, given the
"interesting" implementation that it requires?
It also raises a few questions, like: should remove(), exists(), etc. be about
the final filename or the temporary file...
I would say: to the temp file while writing (between open() and commit()), and
to the target file the rest of the time (before open() and after commit()).
If this seems too weird to you, I'll just forget the idea and put creator's
savefile class in Qt... I kind of liked the idea of the above API though, so
that applications which often use QFile directly initially, can switch to the
safer "use of a temp file for saving" by calling two methods, instead of having
to port to another class altogether.
--
David Faure, fa...@kde.org, http://www.davidfaure.fr
Sponsored by Nokia to work on KDE, incl. KDE Frameworks 5
diff --git a/src/corelib/io/qfile.cpp b/src/corelib/io/qfile.cpp
index d511852..3f2ea15 100644
--- a/src/corelib/io/qfile.cpp
+++ b/src/corelib/io/qfile.cpp
@@ -90,7 +90,7 @@ QFile::EncoderFn QFilePrivate::encoder = locale_encode;
QFile::DecoderFn QFilePrivate::decoder = locale_decode;
QFilePrivate::QFilePrivate()
- : fileEngine(0), lastWasWrite(false),
+ : fileEngine(0), useTemporaryFile(false), finalized(true), lastWasWrite(false),
writeBuffer(QFILE_WRITEBUFFER_SIZE), error(QFile::NoError),
cachedSize(0)
{
@@ -434,6 +434,9 @@ QFile::QFile(QFilePrivate &dd, QObject *parent)
*/
QFile::~QFile()
{
+ Q_D(QFile);
+ if (!d->finalized)
+ rollback();
close();
}
@@ -445,7 +448,8 @@ QFile::~QFile()
*/
QString QFile::fileName() const
{
- return fileEngine()->fileName(QAbstractFileEngine::DefaultName);
+ Q_D(const QFile);
+ return d->fileName;
}
/*!
@@ -482,6 +486,25 @@ QFile::setFileName(const QString &name)
d->fileName = name;
}
+// TODO DOCU
+bool QFile::useTemporaryFile() const
+{
+ Q_D(const QFile);
+ return d->useTemporaryFile;
+}
+
+// TODO (taken from setFileName)
+void QFile::setUseTemporaryFile(bool useTemp)
+{
+ Q_D(QFile);
+ if (isOpen()) {
+ qWarning("QFile::setUseTemporaryFile: File (%s) is already opened",
+ qPrintable(fileName()));
+ return;
+ }
+ d->useTemporaryFile = useTemp;
+}
+
/*!
\fn QString QFile::decodeName(const char *localFileName)
@@ -1019,6 +1042,27 @@ bool QFile::open(OpenMode mode)
return false;
}
+ if (d->useTemporaryFile && (mode & WriteOnly)) {
+ // TODO check if existing file is writable
+ QTemporaryFile tempFile;
+ tempFile.setAutoRemove(false);
+ tempFile.setFileTemplate(d->fileName);
+ if (!tempFile.open(mode)) {
+ d->setError(tempFile.error(), tempFile.errorString());
+ return false;
+ }
+ if (QFile::exists(d->fileName))
+ tempFile.setPermissions(permissions());
+ // Steal the file engine from the temp file
+ delete d->fileEngine;
+ QFilePrivate* tempPrivate = static_cast<QFilePrivate *>(tempFile.d_ptr.data());
+ d->fileEngine = tempPrivate->fileEngine;
+ tempPrivate->fileEngine = 0;
+ tempFile.setOpenMode(NotOpen);
+ d->finalized = false;
+ return true;
+ }
+
#ifdef Q_OS_SYMBIAN
// For symbian, the unbuffered flag is used to control write-behind cache behaviour
if (fileEngine()->open(mode))
@@ -1039,6 +1083,36 @@ bool QFile::open(OpenMode mode)
return false;
}
+// TODO DOCU
+void QFile::rollback()
+{
+ Q_D(QFile);
+ remove(); // remove the temp file
+ d->finalized = true;
+}
+
+// TODO DOCU
+bool QFile::commit()
+{
+ Q_D(QFile);
+ if (d->finalized)
+ return false;
+ d->finalized = true;
+ close();
+ if (error() != NoError) {
+ remove();
+ return false;
+ }
+
+ if (!fileEngine()->rename(d->fileName)) { // atomically replace old file with new file. TODO works on Windows?
+ remove();
+ return false;
+ }
+ fileEngine()->setFileName(d->fileName);
+
+ return true;
+}
+
/*!
\overload
diff --git a/src/corelib/io/qfile.h b/src/corelib/io/qfile.h
index 554b295..c92bed5 100644
--- a/src/corelib/io/qfile.h
+++ b/src/corelib/io/qfile.h
@@ -117,6 +117,11 @@ public:
QString fileName() const;
void setFileName(const QString &name);
+ bool useTemporaryFile() const;
+ void setUseTemporaryFile(bool useTemp);
+ void rollback();
+ bool commit();
+
typedef QByteArray (*EncoderFn)(const QString &fileName);
typedef QString (*DecoderFn)(const QByteArray &localfileName);
static QByteArray encodeName(const QString &fileName);
diff --git a/src/corelib/io/qfile_p.h b/src/corelib/io/qfile_p.h
index 3072f5b..3b05697 100644
--- a/src/corelib/io/qfile_p.h
+++ b/src/corelib/io/qfile_p.h
@@ -76,12 +76,15 @@ protected:
QString fileName;
mutable QAbstractFileEngine *fileEngine;
+ bool useTemporaryFile;
+ bool finalized;
+
bool lastWasWrite;
QRingBuffer writeBuffer;
inline bool ensureFlushed() const;
bool putCharHelper(char c);
-
+
QFile::FileError error;
void setError(QFile::FileError err);
void setError(QFile::FileError err, const QString &errorString);
diff --git a/tests/auto/corelib/io/qfile/tst_qfile.cpp b/tests/auto/corelib/io/qfile/tst_qfile.cpp
index fdb0a7e..ac32c74 100644
--- a/tests/auto/corelib/io/qfile/tst_qfile.cpp
+++ b/tests/auto/corelib/io/qfile/tst_qfile.cpp
@@ -216,6 +216,8 @@ private slots:
void openDirectory();
void writeNothing();
+ void useTemporary();
+
public:
// disabled this test for the moment... it hangs
void invalidFile_data();
@@ -1218,7 +1220,7 @@ void tst_QFile::copyFallback()
QVERIFY(QFile::exists("file-copy-destination.txt"));
QVERIFY(!file.isOpen());
- file.close();
+ file.close();
QFile::remove("file-copy-destination.txt");
}
@@ -2812,8 +2814,8 @@ void tst_QFile::map()
// exotic test to make sure that multiple maps work
// note: windows ce does not reference count mutliple maps
- // it's essentially just the same reference but it
- // cause a resource lock on the file which prevents it
+ // it's essentially just the same reference but it
+ // cause a resource lock on the file which prevents it
// from being removed uchar *memory1 = file.map(0, file.size());
uchar *memory1 = file.map(0, file.size());
QCOMPARE(file.error(), QFile::NoError);
@@ -3189,5 +3191,36 @@ void tst_QFile::autocloseHandle()
}
}
+void tst_QFile::useTemporary()
+{
+ // Test basic functionality
+
+ const QString targetFile = QString::fromLatin1("outfile");
+ QFile::remove(targetFile);
+ QFile file(targetFile);
+ file.setUseTemporaryFile(true);
+ QVERIFY(file.open(QIODevice::WriteOnly));
+ QCOMPARE(file.fileName(), targetFile);
+ QVERIFY(file.exists()); // the tempfile exists
+ QVERIFY(!QFile::exists(targetFile));
+
+ QTextStream ts(&file);
+ ts << "This is test data one.\n";
+ ts.flush();
+ QCOMPARE(file.error(), QFile::NoError);
+ QVERIFY(!QFile::exists(targetFile));
+
+ QVERIFY(file.commit());
+ QVERIFY(QFile::exists(targetFile));
+ QCOMPARE(file.fileName(), targetFile);
+
+ file.remove();
+ QVERIFY(!QFile::exists(targetFile));
+}
+
+// TODO test rollback
+// TODO test write error?
+
QTEST_MAIN(tst_QFile)
+
#include "tst_qfile.moc"
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
