1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

QFile::moveToTrash: fix error reporting on Windows

Browse Source 
If a move-to-trash operation failed, e.g. because the file was opened by
another process (or QFile), then the moveToTrash function would still
return true.

MSDN documents the IFileOperation::PerformOperations to return whether
the operation succeeded, but evidently this is only a statement about
the execution of queued up operations, not a statement about any of the
operations' success.

If the operation succeeded is reported by an HRESULT parameter
of the IFileOperationProgressSink::PostDeleteItem implementation,
and we ignored that parameter so far.

Check it via the SUCCEEDED macro, and set a boolean sink variable based
on that, which we can inspect to return the correct value.

Augment the test case by opening those files we create ourselves, and
if that fails (which it will on Windows, but not necessarily on other
platforms), then try again after closing the file. If the first attempt
succeeded, then the source file must also be gone.

Pick-to: 6.5 6.2 5.15
Fixes: QTBUG-117383
Done-With: Thiago Macieira <thiago.macieira@intel.com>
Change-Id: Icb82a0c9d3b337585dded622d6656e07dee33d84
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
(cherry picked from commit 6ca2008d6e1e02e85f39faab28d5c8a0c0dad082)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
This commit is contained in:
Volker Hilsheimer 2023-09-20 22:35:56 +02:00 committed by Qt Cherry-pick Bot
parent 290a9ec13c
commit a20cf52e09
3 changed files with 140 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
src/corelib/io/qfilesystemengine_win.cpp
View File

@ -840,9 +840,10 @@ public:
return (dwFlags & TSF_DELETE_RECYCLE_IF_POSSIBLE) ? S_OK : E_FAIL;
}
HRESULT STDMETHODCALLTYPE PostDeleteItem(DWORD /* dwFlags */, IShellItem * /* psiItem */,
HRESULT /* hrDelete */,
HRESULT hrDelete,
IShellItem *psiNewlyCreated) override
{
deleteResult = hrDelete;
if (psiNewlyCreated) {
wchar_t *pszName = nullptr;
psiNewlyCreated->GetDisplayName(SIGDN_FILESYSPATH, &pszName);
@ -863,6 +864,7 @@ public:
HRESULT STDMETHODCALLTYPE ResumeTimer() override { return S_OK; }
QString targetPath;
HRESULT deleteResult = S_OK;
private:
ULONG ref;
};
@ -1833,8 +1835,12 @@ bool QFileSystemEngine::moveFileToTrash(const QFileSystemEntry &source,
hres = pfo->PerformOperations();
if (!SUCCEEDED(hres))
return false;
newLocation = QFileSystemEntry(sink->targetPath);
if (!SUCCEEDED(sink->deleteResult)) {
error = QSystemError(sink->deleteResult, QSystemError::NativeError);
return false;
}
newLocation = QFileSystemEntry(sink->targetPath);
return true;
}

82
tests/auto/corelib/io/qfile/tst_qfile.cpp
View File

@ -281,6 +281,8 @@ private slots:
void moveToTrash_data();
void moveToTrash();
void moveToTrashOpenFile_data();
void moveToTrashOpenFile();
void stdfilesystem();
@ -3978,6 +3980,86 @@ void tst_QFile::moveToTrash()
}
}
void tst_QFile::moveToTrashOpenFile_data()
{
QTest::addColumn<bool>("useStatic");
QTest::addColumn<bool>("success");
// QFile::moveToTrash() non-static member closes the file before trashing,
// so this must always succeed.
QTest::newRow("member") << false << true;
// QFile::moveToTrash() static member cannot close the file because it
// operates on another QFile, so this operation will fail on OSes that do
// not permit deleting open files.
QTest::newRow("static") << true
#ifdef Q_OS_WIN
<< false;
#else
<< true;
#endif
}
void tst_QFile::moveToTrashOpenFile()
{
#if defined(Q_OS_ANDROID) || defined(Q_OS_WEBOS)
QSKIP("This platform doesn't implement a trash bin");
#endif
QFETCH(bool, useStatic);
QFETCH(bool, success);
const QByteArrayView contents = "Hello, World\n";
QString newFileName, origFileName;
auto cleanup = qScopeGuard([&] {
if (!origFileName.isEmpty())
QFile::remove(origFileName);
if (!newFileName.isEmpty() && newFileName != origFileName)
QFile::remove(newFileName);
});
origFileName = []() {
QTemporaryFile temp(QDir::homePath() + "/tst_qfile.moveToTrashOpenFile.XXXXXX");
temp.setAutoRemove(false);
if (!temp.open())
qWarning("Failed to create temporary file: %ls", qUtf16Printable(temp.errorString()));
return temp.fileName();
}();
QFile f;
f.setFileName(origFileName);
QVERIFY2(f.open(QIODevice::ReadWrite | QIODevice::Unbuffered), qPrintable(f.errorString()));
f.write(contents.data(), contents.size());
QString errorString;
auto doMoveToTrash = [&](QFile *f) {
if (!f->moveToTrash())
errorString = f->errorString();
newFileName = f->fileName();
};
if (useStatic) {
// it's the same as the static QFile::moveToTrash(), but gives us
// the error string
QFile other(origFileName);
doMoveToTrash(&other);
} else {
doMoveToTrash(&f);
}
QCOMPARE_NE(f.fileName(), QString());
if (success) {
QCOMPARE(errorString, QString());
QCOMPARE_NE(newFileName, origFileName); // must have changed!
QVERIFY(!QFile::exists(origFileName));
QVERIFY(QFile::exists(newFileName));
QCOMPARE(QFileInfo(newFileName).size(), contents.size());
} else {
QCOMPARE_NE(errorString, QString());
QCOMPARE(newFileName, origFileName); // mustn't have changed!
QVERIFY(QFile::exists(origFileName));
QCOMPARE(QFileInfo(origFileName).size(), contents.size());
}
}
void tst_QFile::stdfilesystem()
{
#if QT_CONFIG(cxx17_filesystem)

50
tests/auto/corelib/io/qtemporaryfile/tst_qtemporaryfile.cpp
View File

@ -63,6 +63,7 @@ private slots:
void stressTest();
void rename();
void renameFdLeak();
void moveToTrash();
void reOpenThroughQFile();
void keepOpenMode();
void resetTemplateAfterError();
@ -621,6 +622,55 @@ void tst_QTemporaryFile::renameFdLeak()
#endif
}
void tst_QTemporaryFile::moveToTrash()
{
#if defined(Q_OS_ANDROID) || defined(Q_OS_WEBOS)
QSKIP("This platform doesn't implement a trash bin");
#endif
#ifdef Q_OS_WIN
// QTemporaryFile won't really close the file with close(), so this is
// expected to fail with a sharing violation error.
constexpr bool expectSuccess = false;
#else
constexpr bool expectSuccess = true;
#endif
const QByteArrayView contents = "Hello, World\n";
QTemporaryFile f(QDir::homePath() + "/tst_qtemporaryfile.moveToTrash.XXXXXX");
QString origFileName;
auto cleanup = qScopeGuard([&] {
if (!origFileName.isEmpty())
QFile::remove(origFileName);
if (QString fn = f.fileName(); !fn.isEmpty() && fn != origFileName)
QFile::remove(fn);
});
if (!f.open())
QSKIP("Failed to create temporary file");
f.write(contents.data(), contents.size());
// we need an actual file name:
// 1) so we can delete it in the clean-up guard in case we fail to trash
// 2) so that the file exists on Linux in the first place (no sense in
// trashing an unnamed file)
origFileName = f.fileName();
if (expectSuccess) {
QVERIFY2(f.moveToTrash(), qPrintable(f.errorString()));
QCOMPARE_NE(f.fileName(), origFileName); // must have changed!
QCOMPARE_NE(f.fileName(), QString());
QVERIFY(!QFile::exists(origFileName));
QVERIFY(QFile::exists(f.fileName()));
QCOMPARE(QFileInfo(f.fileName()).size(), contents.size());
} else {
QVERIFY(!f.moveToTrash());
QCOMPARE(f.fileName(), origFileName); // mustn't have changed!
QCOMPARE_NE(f.error(), QFile::NoError);
QCOMPARE_NE(f.errorString(), "Unknown error");
QVERIFY(QFile::exists(origFileName));
}
}
void tst_QTemporaryFile::reOpenThroughQFile()
{
QByteArray data("abcdefghij");