From b5a67fe37302e77eb1d5c389a749b40d08b62460 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Fri, 25 Apr 2025 09:47:08 +0200 Subject: [PATCH 1/5] fix(filesystem): log more permissions when deleting a file fails Signed-off-by: Matthieu Gallien --- src/common/filesystembase.cpp | 30 ++++++++++++++++++++++++++++++ src/common/filesystembase.h | 1 + 2 files changed, 31 insertions(+) diff --git a/src/common/filesystembase.cpp b/src/common/filesystembase.cpp index fafd0eeba61be..88ace9a91b862 100644 --- a/src/common/filesystembase.cpp +++ b/src/common/filesystembase.cpp @@ -320,6 +320,11 @@ bool FileSystem::openAndSeekFileSharedRead(QFile *file, QString *errorOrNull, qi } #ifdef Q_OS_WIN +std::filesystem::perms FileSystem::filePermissionsWinSymlinkSafe(const QString &filename) +{ + return std::filesystem::symlink_status(filename.toStdWString()).permissions(); +} + std::filesystem::perms FileSystem::filePermissionsWin(const QString &filename) { return std::filesystem::status(filename.toStdWString()).permissions(); @@ -551,6 +556,31 @@ bool FileSystem::remove(const QString &fileName, QString *errorString) *errorString = f.errorString(); } qCWarning(lcFileSystem()) << f.errorString() << fileName; + +#if defined Q_OS_WIN + const auto permissionsDisplayHelper = [] (std::filesystem::perms currentPermissions) { + const auto unitaryHelper = [currentPermissions] (std::filesystem::perms testedPermission, char permissionChar) { + return (static_cast(currentPermissions & testedPermission) ? permissionChar : '-'); + }; + + qCInfo(lcFileSystem()) << unitaryHelper(std::filesystem::perms::owner_read, 'r') + << unitaryHelper(std::filesystem::perms::owner_write, 'w') + << unitaryHelper(std::filesystem::perms::owner_exec, 'x') + << unitaryHelper(std::filesystem::perms::group_read, 'r') + << unitaryHelper(std::filesystem::perms::group_write, 'w') + << unitaryHelper(std::filesystem::perms::group_exec, 'x') + << unitaryHelper(std::filesystem::perms::others_read, 'r') + << unitaryHelper(std::filesystem::perms::others_write, 'w') + << unitaryHelper(std::filesystem::perms::others_exec, 'x'); + }; + + const auto unsafeFilePermissions = filePermissionsWin(fileName); + permissionsDisplayHelper(unsafeFilePermissions); + + const auto safeFilePermissions = filePermissionsWinSymlinkSafe(fileName); + permissionsDisplayHelper(safeFilePermissions); +#endif + return false; } return true; diff --git a/src/common/filesystembase.h b/src/common/filesystembase.h index e9547d2d0a7b6..481e765f638f4 100644 --- a/src/common/filesystembase.h +++ b/src/common/filesystembase.h @@ -173,6 +173,7 @@ namespace FileSystem { */ QString OCSYNC_EXPORT pathtoUNC(const QString &str); + std::filesystem::perms OCSYNC_EXPORT filePermissionsWinSymlinkSafe(const QString &filename); std::filesystem::perms OCSYNC_EXPORT filePermissionsWin(const QString &filename); void OCSYNC_EXPORT setFilePermissionsWin(const QString &filename, const std::filesystem::perms &perms); #endif From 3f57fa1b4f7ff2629f69eda9f9916e6090aa487f Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Fri, 25 Apr 2025 10:11:15 +0200 Subject: [PATCH 2/5] fix(filesystem): use platform specific API to make a file read-only on Windows we may fail to mark a file read-only be read-write again using high level API switch to use of low level C API from Microsoft Signed-off-by: Matthieu Gallien --- src/common/filesystembase.cpp | 57 ++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/src/common/filesystembase.cpp b/src/common/filesystembase.cpp index 88ace9a91b862..d2497a3017a54 100644 --- a/src/common/filesystembase.cpp +++ b/src/common/filesystembase.cpp @@ -111,36 +111,43 @@ static QFile::Permissions getDefaultWritePermissions() void FileSystem::setFileReadOnly(const QString &filename, bool readonly) { #ifdef Q_OS_WIN - if (isLnkFile(filename)) { - if (!fileExists(filename)) { - return; - } - try { - const auto permissions = filePermissionsWin(filename); + if (!fileExists(filename)) { + Q_ASSERT(false); + return; + } - std::filesystem::perms allWritePermissions = std::filesystem::perms::_All_write; - static std::filesystem::perms defaultWritePermissions = std::filesystem::perms::others_write; + const auto fileAttributes = GetFileAttributesW(filename.toStdWString().c_str()); + if (fileAttributes == INVALID_FILE_ATTRIBUTES) { + const auto lastError = GetLastError(); + auto errorMessage = static_cast(nullptr); + if (FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, + nullptr, lastError, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), errorMessage, 0, nullptr) == 0) { + qCWarning(lcFileSystem()) << "GetFileAttributesW" << filename << (readonly ? "readonly" : "read write") << errorMessage; + } else { + qCWarning(lcFileSystem()) << "GetFileAttributesW" << filename << (readonly ? "readonly" : "read write") << "unknown error" << lastError; + } + return; + } - std::filesystem::permissions(filename.toStdWString(), allWritePermissions, std::filesystem::perm_options::remove); + auto newFileAttributes = fileAttributes; + if (readonly) { + newFileAttributes = newFileAttributes | FILE_ATTRIBUTE_READONLY; + } else { + newFileAttributes = newFileAttributes & (~FILE_ATTRIBUTE_READONLY); + } - if (!readonly) { - std::filesystem::permissions(filename.toStdWString(), defaultWritePermissions, std::filesystem::perm_options::add); - } - } - catch (const std::filesystem::filesystem_error &e) - { - qCWarning(lcFileSystem()) << filename << (readonly ? "readonly" : "read write") << e.what(); - } - catch (const std::system_error &e) - { - qCWarning(lcFileSystem()) << filename << e.what(); - } - catch (...) - { - qCWarning(lcFileSystem()) << filename; + if (SetFileAttributesW(filename.toStdWString().c_str(), newFileAttributes) == 0) { + const auto lastError = GetLastError(); + auto errorMessage = static_cast(nullptr); + if (FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM | FORMAT_MESSAGE_IGNORE_INSERTS, + nullptr, lastError, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), errorMessage, 0, nullptr) == 0) { + qCWarning(lcFileSystem()) << "SetFileAttributesW" << filename << (readonly ? "readonly" : "read write") << errorMessage; + } else { + qCWarning(lcFileSystem()) << "SetFileAttributesW" << filename << (readonly ? "readonly" : "read write") << "unknown error" << lastError; } - return; } + + return; #endif QFile file(filename); QFile::Permissions permissions = file.permissions(); From 6f97ee8da68aaf5b536fc5af95fc26000c42f557 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Fri, 25 Apr 2025 14:00:30 +0200 Subject: [PATCH 3/5] fix(propagation): ensure we delete pending folders before terminating we might forget to run the pending folder deletions when terminating synchronization ensure we check if any of them are to be done Signed-off-by: Matthieu Gallien --- src/libsync/owncloudpropagator.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index b89e203fcd58e..0e67c28ea348f 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -1611,6 +1611,11 @@ void PropagateRootDirectory::slotSubJobsFinished(SyncFileItem::Status status) return; } + if (!_dirDeletionJobs._jobsToDo.empty()) { + _dirDeletionJobs.scheduleSelfOrChild(); + return; + } + if (status != SyncFileItem::Success && status != SyncFileItem::Restoration && status != SyncFileItem::BlacklistedError From 30db63bacaff65bf8f4652880fd903c7d0f3f527 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Fri, 25 Apr 2025 12:19:26 +0200 Subject: [PATCH 4/5] fix(propagation): more logs on folder deletions propagation step Signed-off-by: Matthieu Gallien --- src/libsync/owncloudpropagator.cpp | 32 ++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 0e67c28ea348f..65279c5cc744d 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -1366,6 +1366,7 @@ PropagatorJob::JobParallelism PropagateDirectory::parallelism() const bool PropagateDirectory::scheduleSelfOrChild() { if (_state == Finished) { + qCDebug(lcDirectory) << "folder job finished"; return false; } @@ -1374,15 +1375,32 @@ bool PropagateDirectory::scheduleSelfOrChild() } if (_firstJob && _firstJob->_state == NotYetStarted) { - return _firstJob->scheduleSelfOrChild(); + const auto result = _firstJob->scheduleSelfOrChild(); + + if (result) { + qCDebug(lcDirectory) << "folder first job has more work to do"; + } else { + qCDebug(lcDirectory) << "folder first job is done"; + } + + return result; } if (_firstJob && _firstJob->_state == Running) { // Don't schedule any more job until this is done. + qCDebug(lcDirectory) << "first job is running"; return false; } - return _subJobs.scheduleSelfOrChild(); + const auto result = _subJobs.scheduleSelfOrChild(); + + if (result) { + qCDebug(lcDirectory) << "folder child jobs have more work to do"; + } else { + qCDebug(lcDirectory) << "folder child jobs are done"; + } + + return result; } void PropagateDirectory::slotFirstJobFinished(SyncFileItem::Status status) @@ -1527,6 +1545,7 @@ void PropagateDirectory::slotSubJobsFinished(SyncFileItem::Status status) } } _state = Finished; + qCDebug(lcDirectory()) << "PropagateDirectory::slotSubJobsFinished" << "emit finished" << status; emit finished(status); } @@ -1579,28 +1598,36 @@ qint64 PropagateRootDirectory::committedDiskSpace() const void PropagateRootDirectory::appendDirDeletionJob(PropagatorJob *job) { + if (auto directoryJob = qobject_cast(job)) { + qCDebug(lcRootDirectory) << "new folder deletion job" << directoryJob->_item->_file; + } _dirDeletionJobs.appendJob(job); } bool PropagateRootDirectory::scheduleSelfOrChild() { if (_state == Finished) { + qCDebug(lcRootDirectory) << "root folder fully propagated"; return false; } if (PropagateDirectory::scheduleSelfOrChild() && propagator()->delayedTasks().empty()) { + qCDebug(lcRootDirectory) << "root folder has more jobs to do"; return true; } // Important: Finish _subJobs before scheduling any deletes. if (_subJobs._state != Finished) { + qCDebug(lcRootDirectory) << "root folder has running jobs to do"; return false; } if (!propagator()->delayedTasks().empty()) { + qCDebug(lcRootDirectory) << "root folder has more delayed jobs to do"; return scheduleDelayedJobs(); } + qCDebug(lcRootDirectory) << "schedule folder deletions step"; return _dirDeletionJobs.scheduleSelfOrChild(); } @@ -1625,6 +1652,7 @@ void PropagateRootDirectory::slotSubJobsFinished(SyncFileItem::Status status) // Synchronously abort abort(AbortType::Synchronous); _state = Finished; + qCInfo(lcRootDirectory()) << "PropagateRootDirectory::slotSubJobsFinished" << "emit finished" << status; emit finished(status); } return; From b6d74509fb619b359676f884eb2f7b4dfc17bc7f Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Fri, 25 Apr 2025 15:52:31 +0200 Subject: [PATCH 5/5] fix(propagation): ensure we run file removal propagation steps Signed-off-by: Matthieu Gallien --- src/libsync/owncloudpropagator.cpp | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 65279c5cc744d..5751911e18643 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -1638,16 +1638,7 @@ void PropagateRootDirectory::slotSubJobsFinished(SyncFileItem::Status status) return; } - if (!_dirDeletionJobs._jobsToDo.empty()) { - _dirDeletionJobs.scheduleSelfOrChild(); - return; - } - - if (status != SyncFileItem::Success - && status != SyncFileItem::Restoration - && status != SyncFileItem::BlacklistedError - && status != SyncFileItem::FileNameClash - && status != SyncFileItem::Conflict) { + if (status == SyncFileItem::FatalError) { if (_state != Finished) { // Synchronously abort abort(AbortType::Synchronous); @@ -1661,18 +1652,18 @@ void PropagateRootDirectory::slotSubJobsFinished(SyncFileItem::Status status) if (_errorStatus == SyncFileItem::NoStatus) { switch (status) { case SyncFileItem::NoStatus: - case SyncFileItem::FatalError: - case SyncFileItem::NormalError: - case SyncFileItem::SoftError: - case SyncFileItem::Conflict: case SyncFileItem::FileIgnored: - case SyncFileItem::FileLocked: case SyncFileItem::Restoration: - case SyncFileItem::FileNameInvalid: - case SyncFileItem::FileNameInvalidOnServer: - case SyncFileItem::DetailError: case SyncFileItem::Success: break; + case SyncFileItem::FileLocked: + case SyncFileItem::DetailError: + case SyncFileItem::SoftError: + case SyncFileItem::Conflict: + case SyncFileItem::FatalError: + case SyncFileItem::FileNameInvalid: + case SyncFileItem::FileNameInvalidOnServer: + case SyncFileItem::NormalError: case SyncFileItem::FileNameClash: case SyncFileItem::BlacklistedError: _errorStatus = status;