From 1de95f36c42f05942ec689b74db8a159ddff8acb Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Thu, 23 Jan 2025 19:59:42 -0800 Subject: [PATCH] QFile::copy: refactor to use QSaveFile This code mostly existed from before QTemporaryFile, let alone QSaveFile, and yet duplicated a lot of its functionality. So I'm choosing to simplify our lives by depending on QSaveFile. As a minor improvement, the setPermissions() call on Unix will perform an fchmod(2) system call on the file descriptor instead of chmod(2) on the name. I've extracted it to a separate function so I can use that in rename() too. I had to disable the longFileName test on VxWorks because this fails. With no system to debug why, I can only guess that it's now just too long. No ChangeLog because the feature system isn't supported. Change-Id: I033d677fe090ca3c29d4fffd4024f149402df51d Reviewed-by: David Faure --- src/corelib/io/qfile.cpp | 163 ++++++++++------------ src/corelib/io/qfile.h | 4 +- src/corelib/io/qfile_p.h | 1 + src/corelib/io/qfiledevice_p.h | 2 + src/corelib/io/qsavefile.h | 1 + tests/auto/corelib/io/qfile/tst_qfile.cpp | 2 + 6 files changed, 83 insertions(+), 90 deletions(-) diff --git a/src/corelib/io/qfile.cpp b/src/corelib/io/qfile.cpp index 05a55cf2327..263806c542d 100644 --- a/src/corelib/io/qfile.cpp +++ b/src/corelib/io/qfile.cpp @@ -5,14 +5,15 @@ #include "qplatformdefs.h" #include "qdebug.h" #include "qfile.h" -#include "qfsfileengine_p.h" -#include "qtemporaryfile.h" -#include "qtemporaryfile_p.h" -#include "qlist.h" #include "qfileinfo.h" +#include "qfsfileengine_p.h" +#include "qlist.h" +#include "qsavefile.h" +#include "qtemporaryfile.h" #include "private/qiodevice_p.h" #include "private/qfile_p.h" #include "private/qfilesystemengine_p.h" +#include "private/qsavefile_p.h" #include "private/qsystemerror_p.h" #include "private/qtemporaryfile_p.h" #if defined(QT_BUILD_CORE_LIB) @@ -778,7 +779,72 @@ QFile::link(const QString &fileName, const QString &linkName) return QFile(fileName).link(linkName); } -#ifndef QT_BOOTSTRAPPED // dangerous without QTemporaryFile +#if QT_CONFIG(temporaryfile) // dangerous without QTemporaryFile +bool QFilePrivate::copy(const QString &newName) +{ + Q_Q(QFile); + Q_ASSERT(error == QFile::NoError); + Q_ASSERT(!q->isOpen()); + + // Some file engines can perform this copy more efficiently (e.g., Windows + // calling CopyFile). + if (engine()->copy(newName)) + return true; + + if (!q->open(QFile::ReadOnly | QFile::Unbuffered)) { + setError(QFile::CopyError, QFile::tr("Cannot open %1 for input").arg(fileName)); + return false; + } + + QSaveFile out(newName); + out.setDirectWriteFallback(true); + if (!out.open(QIODevice::WriteOnly | QIODevice::Unbuffered)) { + q->close(); + setError(QFile::CopyError, QFile::tr("Cannot open for output: %1").arg(out.errorString())); + return false; + } + + // Attempt to do an OS-level data copy + QAbstractFileEngine::TriStateResult r = engine()->cloneTo(out.d_func()->engine()); + if (r == QAbstractFileEngine::TriStateResult::Failed) { + q->close(); + setError(QFile::CopyError, QFile::tr("Could not copy to %1: %2") + .arg(newName, engine()->errorString())); + return false; + } + + while (r == QAbstractFileEngine::TriStateResult::NotSupported) { + // OS couldn't do it, so do a block-level copy + char block[4096]; + qint64 in = q->read(block, sizeof(block)); + if (in == 0) + break; // eof + if (in < 0) { + // Unable to read from the source. Save the error from read() above. + QString s = std::move(errorString); + q->close(); + setError(QFile::CopyError, std::move(s)); + return false; + } + if (in != out.write(block, in)) { + q->close(); + setError(QFile::CopyError, QFile::tr("Failure to write block: %1") + .arg(out.errorString())); + return false; + } + } + + // copy the permissions + out.setPermissions(q->permissions()); + q->close(); + + // final step: commit the copy + if (out.commit()) + return true; + setError(out.error(), out.errorString()); + return false; +} + /*! Copies the file named fileName() to \a newName. @@ -807,89 +873,8 @@ QFile::copy(const QString &newName) } unsetError(); close(); - if (error() == QFile::NoError) { - if (d->engine()->copy(newName)) { - unsetError(); - return true; - } else { - bool error = false; - if (!open(QFile::ReadOnly | QFile::Unbuffered)) { - error = true; - d->setError(QFile::CopyError, tr("Cannot open %1 for input").arg(d->fileName)); - } else { - const auto fileTemplate = "%1/qt_temp.XXXXXX"_L1; -#if !QT_CONFIG(temporaryfile) - QFile out(fileTemplate.arg(QFileInfo(newName).path())); - if (!out.open(QIODevice::ReadWrite | QIODevice::Truncate)) - error = true; -#else - QTemporaryFile out(fileTemplate.arg(QFileInfo(newName).path())); - if (!out.open()) - error = true; -#endif - if (error) { - d->setError(QFile::CopyError, tr("Cannot open for output: %1").arg(out.errorString())); - out.close(); - close(); - } else { - QAbstractFileEngine::TriStateResult r = d->engine()->cloneTo(out.d_func()->engine()); - if (r == QAbstractFileEngine::TriStateResult::Failed) { - d->setError(QFile::CopyError, tr("Could not copy to %1: %2") - .arg(newName, d->engine()->errorString())); - error = true; - } else if (r == QAbstractFileEngine::TriStateResult::NotSupported) { - char block[4096]; - qint64 totalRead = 0; - out.setOpenMode(ReadWrite | Unbuffered); - while (!atEnd()) { - qint64 in = read(block, sizeof(block)); - if (in <= 0) - break; - totalRead += in; - if (in != out.write(block, in)) { - close(); - d->setError(QFile::CopyError, tr("Failure to write block: %1") - .arg(out.errorString())); - error = true; - break; - } - } - - if (totalRead != size()) { - // Unable to read from the source. The error string is - // already set from read(). - error = true; - } - } - - if (!error) { - // Sync to disk if possible. Ignore errors (e.g. not supported). - out.d_func()->fileEngine->syncToDisk(); - - if (!out.rename(newName)) { - error = true; - close(); - d->setError(QFile::CopyError, tr("Cannot create %1 for output: %2") - .arg(newName, out.errorString())); - } - } -#if !QT_CONFIG(temporaryfile) - if (error) - out.remove(); -#else - if (!error) - out.setAutoRemove(false); -#endif - } - } - if (!error) { - QFile::setPermissions(newName, permissions()); - close(); - unsetError(); - return true; - } - } - } + if (error() == QFile::NoError) + return d->copy(newName); return false; } @@ -911,7 +896,7 @@ QFile::copy(const QString &fileName, const QString &newName) { return QFile(fileName).copy(newName); } -#endif // QT_BOOTSTRAPPED +#endif // QT_CONFIG(temporaryfile) /*! Opens the file using \a mode flags, returning \c true if successful; diff --git a/src/corelib/io/qfile.h b/src/corelib/io/qfile.h index 9c6f600bae7..c6caf5c56e3 100644 --- a/src/corelib/io/qfile.h +++ b/src/corelib/io/qfile.h @@ -263,13 +263,15 @@ public: } #endif // QT_CONFIG(cxx17_filesystem) +#if QT_CONFIG(temporaryfile) bool copy(const QString &newName); static bool copy(const QString &fileName, const QString &newName); +#endif #ifdef Q_QDOC bool copy(const std::filesystem::path &newName); static bool copy(const std::filesystem::path &fileName, const std::filesystem::path &newName); -#elif QT_CONFIG(cxx17_filesystem) +#elif QT_CONFIG(cxx17_filesystem) && QT_CONFIG(temporaryfile) template = 0> bool copy(const T &newName) { diff --git a/src/corelib/io/qfile_p.h b/src/corelib/io/qfile_p.h index e4e305309ac..126da28b0f9 100644 --- a/src/corelib/io/qfile_p.h +++ b/src/corelib/io/qfile_p.h @@ -35,6 +35,7 @@ protected: bool openExternalFile(QIODevice::OpenMode flags, FILE *fh, QFile::FileHandleFlags handleFlags); QAbstractFileEngine *engine() const override; + bool copy(const QString &newName); QString fileName; }; diff --git a/src/corelib/io/qfiledevice_p.h b/src/corelib/io/qfiledevice_p.h index 72b43ed5a1e..b66e15db624 100644 --- a/src/corelib/io/qfiledevice_p.h +++ b/src/corelib/io/qfiledevice_p.h @@ -45,8 +45,10 @@ protected: QFileDevicePrivate(); ~QFileDevicePrivate(); +public: virtual QAbstractFileEngine *engine() const; +protected: inline bool ensureFlushed() const; bool putCharHelper(char c) override; diff --git a/src/corelib/io/qsavefile.h b/src/corelib/io/qsavefile.h index 4dd712d4b60..bf0a91bae74 100644 --- a/src/corelib/io/qsavefile.h +++ b/src/corelib/io/qsavefile.h @@ -58,6 +58,7 @@ private: private: Q_DISABLE_COPY(QSaveFile) + friend class QFilePrivate; }; QT_END_NAMESPACE diff --git a/tests/auto/corelib/io/qfile/tst_qfile.cpp b/tests/auto/corelib/io/qfile/tst_qfile.cpp index 2e27574b291..306dda14690 100644 --- a/tests/auto/corelib/io/qfile/tst_qfile.cpp +++ b/tests/auto/corelib/io/qfile/tst_qfile.cpp @@ -2275,6 +2275,7 @@ void tst_QFile::longFileName_data() QTest::newRow( "148 chars" ) << QString::fromLatin1("longFileNamelongFileNamelongFileNamelongFileName" "longFileNamelongFileNamelongFileNamelongFileName" "longFileNamelongFileNamelongFileNamelongFileName.txt"); +#ifndef Q_OS_VXWORKS QTest::newRow( "244 chars" ) << QString::fromLatin1("longFileNamelongFileNamelongFileNamelongFileName" "longFileNamelongFileNamelongFileNamelongFileName" "longFileNamelongFileNamelongFileNamelongFileName" @@ -2291,6 +2292,7 @@ void tst_QFile::longFileName_data() "longFileNamelongFileNamelongFileNamelongFileName" "longFileNamelongFileNamelongFileNamelongFileName" "longFileNamelongFileNamelongFileNamelongFileName.txt");*/ +#endif } void tst_QFile::longFileName()