From be2df3c6e099485f5bd805d8a1e775996684c9d2 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Tue, 21 Jan 2025 10:39:07 -0800 Subject: [PATCH] QFileSystemEngine: let cloneFile() inform QFile of permanent errors The Unix implementation in QFileSystemEngine::cloneFile() was unable to tell the upper layer whether the failure was permanent (like ENOSPC and EIO) or whether the fast copy attempt wasn't possible. This resulted in QFile always retrying even after ENOSPC conditions. This commit resolves that. Change-Id: I38a830a99a0d38bcb51efffdf34bb7fead639496 Reviewed-by: Ivan Solovev Reviewed-by: Ahmad Samir --- src/corelib/io/qabstractfileengine.cpp | 4 +-- src/corelib/io/qabstractfileengine_p.h | 7 ++++- src/corelib/io/qfile.cpp | 7 ++++- src/corelib/io/qfilesystemengine_p.h | 4 ++- src/corelib/io/qfilesystemengine_unix.cpp | 34 +++++++++++++---------- src/corelib/io/qfsfileengine_p.h | 2 +- src/corelib/io/qfsfileengine_unix.cpp | 9 ++++-- src/corelib/io/qfsfileengine_win.cpp | 4 +-- 8 files changed, 46 insertions(+), 25 deletions(-) diff --git a/src/corelib/io/qabstractfileengine.cpp b/src/corelib/io/qabstractfileengine.cpp index 43ceb21a57d..4ee29daa7a9 100644 --- a/src/corelib/io/qabstractfileengine.cpp +++ b/src/corelib/io/qabstractfileengine.cpp @@ -829,10 +829,10 @@ bool QAbstractFileEngine::unmap(uchar *address) Returns \c true on success; otherwise, \c false is returned. */ -bool QAbstractFileEngine::cloneTo(QAbstractFileEngine *target) +QAbstractFileEngine::TriStateResult QAbstractFileEngine::cloneTo(QAbstractFileEngine *target) { Q_UNUSED(target); - return false; + return TriStateResult::NotSupported; } /*! diff --git a/src/corelib/io/qabstractfileengine_p.h b/src/corelib/io/qabstractfileengine_p.h index cc35d3c45d5..ca196305765 100644 --- a/src/corelib/io/qabstractfileengine_p.h +++ b/src/corelib/io/qabstractfileengine_p.h @@ -83,6 +83,11 @@ public: OwnerGroup }; + enum class TriStateResult : qint8 { + NotSupported = -1, + Failed = 0, + Success = 1, + }; virtual ~QAbstractFileEngine(); @@ -119,7 +124,7 @@ public: virtual QDateTime fileTime(QFile::FileTime time) const; virtual void setFileName(const QString &file); virtual int handle() const; - virtual bool cloneTo(QAbstractFileEngine *target); + virtual TriStateResult cloneTo(QAbstractFileEngine *target); bool atEnd() const; uchar *map(qint64 offset, qint64 size, QFile::MemoryMapFlags flags); bool unmap(uchar *ptr); diff --git a/src/corelib/io/qfile.cpp b/src/corelib/io/qfile.cpp index dcb6f5ce4e0..2a1484e1120 100644 --- a/src/corelib/io/qfile.cpp +++ b/src/corelib/io/qfile.cpp @@ -834,7 +834,12 @@ QFile::copy(const QString &newName) out.close(); close(); } else { - if (!d->engine()->cloneTo(out.d_func()->engine())) { + 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); diff --git a/src/corelib/io/qfilesystemengine_p.h b/src/corelib/io/qfilesystemengine_p.h index f3f63a96af4..773c1d34f87 100644 --- a/src/corelib/io/qfilesystemengine_p.h +++ b/src/corelib/io/qfilesystemengine_p.h @@ -60,6 +60,8 @@ Q_CORE_EXPORT bool qt_isCaseSensitive(const QFileSystemEntry &entry, QFileSystem class Q_AUTOTEST_EXPORT QFileSystemEngine { public: + using TriStateResult = QAbstractFileEngine::TriStateResult; + static bool isCaseSensitive(const QFileSystemEntry &entry, QFileSystemMetaData &data); static QFileSystemEntry getLinkTarget(const QFileSystemEntry &link, QFileSystemMetaData &data); @@ -86,7 +88,7 @@ public: static bool fillMetaData(const QFileSystemEntry &entry, QFileSystemMetaData &data, QFileSystemMetaData::MetaDataFlags what); #if defined(Q_OS_UNIX) - static bool cloneFile(int srcfd, int dstfd, const QFileSystemMetaData &knownData); + static TriStateResult cloneFile(int srcfd, int dstfd, const QFileSystemMetaData &knownData); static bool fillMetaData(int fd, QFileSystemMetaData &data); // what = PosixStatFlags static QByteArray id(int fd); static bool setFileTime(int fd, const QDateTime &newDate, diff --git a/src/corelib/io/qfilesystemengine_unix.cpp b/src/corelib/io/qfilesystemengine_unix.cpp index b1e41906e64..15a004df13b 100644 --- a/src/corelib/io/qfilesystemengine_unix.cpp +++ b/src/corelib/io/qfilesystemengine_unix.cpp @@ -1068,7 +1068,7 @@ bool QFileSystemEngine::fillMetaData(const QFileSystemEntry &entry, QFileSystemM } // static -bool QFileSystemEngine::cloneFile(int srcfd, int dstfd, const QFileSystemMetaData &knownData) +auto QFileSystemEngine::cloneFile(int srcfd, int dstfd, const QFileSystemMetaData &knownData) -> TriStateResult { QT_STATBUF statBuffer; if (knownData.hasFlags(QFileSystemMetaData::PosixStatFlags) && @@ -1076,12 +1076,14 @@ bool QFileSystemEngine::cloneFile(int srcfd, int dstfd, const QFileSystemMetaDat statBuffer.st_mode = S_IFREG; } else if (knownData.hasFlags(QFileSystemMetaData::PosixStatFlags) && knownData.isDirectory()) { - return false; // fcopyfile(3) returns success on directories + errno = EISDIR; + return TriStateResult::Failed; // fcopyfile(3) returns success on directories } else if (QT_FSTAT(srcfd, &statBuffer) == -1) { - return false; + // errno was set + return TriStateResult::Failed; } else if (!S_ISREG((statBuffer.st_mode))) { // not a regular file, let QFile do the copy - return false; + return TriStateResult::NotSupported; } [[maybe_unused]] auto destinationIsEmpty = [dstfd]() { @@ -1093,7 +1095,7 @@ bool QFileSystemEngine::cloneFile(int srcfd, int dstfd, const QFileSystemMetaDat #if defined(Q_OS_LINUX) // first, try FICLONE (only works on regular files and only on certain fs) if (::ioctl(dstfd, FICLONE, srcfd) == 0) - return true; + return TriStateResult::Success; // Second, try sendfile (it can send to some special types too). // sendfile(2) is limited in the kernel to 2G - 4k @@ -1102,30 +1104,34 @@ bool QFileSystemEngine::cloneFile(int srcfd, int dstfd, const QFileSystemMetaDat ssize_t n = ::sendfile(dstfd, srcfd, nullptr, SendfileSize); if (n == -1) { // if we got an error here, give up and try at an upper layer - return false; + return TriStateResult::NotSupported; } while (n) { n = ::sendfile(dstfd, srcfd, nullptr, SendfileSize); if (n == -1) { - // uh oh, this is probably a real error (like ENOSPC), but we have - // no way to notify QFile of partial success, so just erase any work - // done (hopefully we won't get any errors, because there's nothing - // we can do about them) + // uh oh, this is probably a real error (like ENOSPC) n = ftruncate(dstfd, 0); n = lseek(srcfd, 0, SEEK_SET); n = lseek(dstfd, 0, SEEK_SET); - return false; + return TriStateResult::Failed; } } - return true; + return TriStateResult::Success; #elif defined(Q_OS_DARWIN) // try fcopyfile - return fcopyfile(srcfd, dstfd, nullptr, COPYFILE_DATA | COPYFILE_STAT) == 0; + if (fcopyfile(srcfd, dstfd, nullptr, COPYFILE_DATA | COPYFILE_STAT) == 0) + return TriStateResult::Success; + switch (errno) { + case ENOTSUP: + case ENOMEM: + return TriStateResult::NotSupported; // let QFile try + } + return TriStateResult::Failed; #else Q_UNUSED(dstfd); - return false; + return TriStateResult::NotSupported; #endif } diff --git a/src/corelib/io/qfsfileengine_p.h b/src/corelib/io/qfsfileengine_p.h index c8ed1f71dc8..8744a8f53ef 100644 --- a/src/corelib/io/qfsfileengine_p.h +++ b/src/corelib/io/qfsfileengine_p.h @@ -93,7 +93,7 @@ public: qint64 read(char *data, qint64 maxlen) override; qint64 readLine(char *data, qint64 maxlen) override; qint64 write(const char *data, qint64 len) override; - bool cloneTo(QAbstractFileEngine *target) override; + TriStateResult cloneTo(QAbstractFileEngine *target) override; virtual bool isUnnamedFile() const { return false; } diff --git a/src/corelib/io/qfsfileengine_unix.cpp b/src/corelib/io/qfsfileengine_unix.cpp index afcbb104413..45a8332512b 100644 --- a/src/corelib/io/qfsfileengine_unix.cpp +++ b/src/corelib/io/qfsfileengine_unix.cpp @@ -648,15 +648,18 @@ bool QFSFileEnginePrivate::unmap(uchar *ptr) /*! \reimp */ -bool QFSFileEngine::cloneTo(QAbstractFileEngine *target) +QAbstractFileEngine::TriStateResult QFSFileEngine::cloneTo(QAbstractFileEngine *target) { Q_D(QFSFileEngine); if ((target->fileFlags(LocalDiskFlag) & LocalDiskFlag) == 0) - return false; + return TriStateResult::NotSupported; int srcfd = d->nativeHandle(); int dstfd = target->handle(); - return QFileSystemEngine::cloneFile(srcfd, dstfd, d->metaData); + TriStateResult r = QFileSystemEngine::cloneFile(srcfd, dstfd, d->metaData); + if (r == TriStateResult::Failed) + setError(QFile::CopyError, qt_error_string(errno)); + return r; } QT_END_NAMESPACE diff --git a/src/corelib/io/qfsfileengine_win.cpp b/src/corelib/io/qfsfileengine_win.cpp index bf8bb102d81..c436d4cd26b 100644 --- a/src/corelib/io/qfsfileengine_win.cpp +++ b/src/corelib/io/qfsfileengine_win.cpp @@ -897,12 +897,12 @@ bool QFSFileEnginePrivate::unmap(uchar *ptr) /*! \reimp */ -bool QFSFileEngine::cloneTo(QAbstractFileEngine *target) +QAbstractFileEngine::TriStateResult QFSFileEngine::cloneTo(QAbstractFileEngine *target) { // There's some Windows Server 2016 API, but we won't // bother with it. Q_UNUSED(target); - return false; + return TriStateResult::NotSupported; } QT_END_NAMESPACE