From 7dd9499fe8c8bc665e5bbb917f112d3354860c4a Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 9 Nov 2023 22:38:41 +0100 Subject: [PATCH] avoid modifying a placeholder (virtual files) when not needed acoid modifying some metadata of the placeholder when this placeholder has just been uploaded to the server (will avoid truncating the timestamps) Close #6190 Signed-off-by: Matthieu Gallien --- src/common/vfs.h | 19 +++- src/libsync/bulkpropagatorjob.cpp | 2 +- src/libsync/owncloudpropagator.cpp | 15 +-- src/libsync/owncloudpropagator.h | 15 ++- src/libsync/propagateupload.cpp | 2 +- src/libsync/vfs/cfapi/cfapiwrapper.cpp | 139 ++++++++++++++----------- src/libsync/vfs/cfapi/cfapiwrapper.h | 1 + src/libsync/vfs/cfapi/vfs_cfapi.cpp | 8 +- src/libsync/vfs/cfapi/vfs_cfapi.h | 2 +- src/libsync/vfs/suffix/vfs_suffix.cpp | 2 +- src/libsync/vfs/suffix/vfs_suffix.h | 2 +- src/libsync/vfs/xattr/vfs_xattr.cpp | 5 +- src/libsync/vfs/xattr/vfs_xattr.h | 5 +- 13 files changed, 130 insertions(+), 87 deletions(-) diff --git a/src/common/vfs.h b/src/common/vfs.h index 98757878ce3b..efe2e6654405 100644 --- a/src/common/vfs.h +++ b/src/common/vfs.h @@ -113,6 +113,15 @@ class OCSYNC_EXPORT Vfs : public QObject }; Q_ENUM(ConvertToPlaceholderResult) + enum UpdateMetadataType { + DatabaseMetadata = 1 << 0, + FileMetadata = 1 << 1, + AllMetadata = DatabaseMetadata | FileMetadata, + }; + + Q_DECLARE_FLAGS(UpdateMetadataTypes, UpdateMetadataType) + Q_FLAG(UpdateMetadataType) + static QString modeToString(Mode mode); static Optional modeFromString(const QString &str); @@ -203,10 +212,10 @@ class OCSYNC_EXPORT Vfs : public QObject * new placeholder shall supersede, for rename-replace actions with new downloads, * for example. */ - Q_REQUIRED_RESULT virtual Result convertToPlaceholder( - const QString &filename, - const SyncFileItem &item, - const QString &replacesFile = QString()) = 0; + Q_REQUIRED_RESULT virtual Result convertToPlaceholder(const QString &filename, + const SyncFileItem &item, + const QString &replacesFile = {}, + UpdateMetadataTypes updateType = AllMetadata) = 0; /// Determine whether the file at the given absolute path is a dehydrated placeholder. Q_REQUIRED_RESULT virtual bool isDehydratedPlaceholder(const QString &filePath) = 0; @@ -311,7 +320,7 @@ class OCSYNC_EXPORT VfsOff : public Vfs Result updateMetadata(const QString &, time_t, qint64, const QByteArray &) override { return {}; } Result createPlaceholder(const SyncFileItem &) override { return {}; } Result dehydratePlaceholder(const SyncFileItem &) override { return {}; } - Result convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) override { return ConvertToPlaceholderResult::Ok; } + Result convertToPlaceholder(const QString &, const SyncFileItem &, const QString &, UpdateMetadataTypes) override { return ConvertToPlaceholderResult::Ok; } bool needsMetadataUpdate(const SyncFileItem &) override { return false; } bool isDehydratedPlaceholder(const QString &) override { return false; } diff --git a/src/libsync/bulkpropagatorjob.cpp b/src/libsync/bulkpropagatorjob.cpp index 6d83210f0883..0f0a7db3fcfa 100644 --- a/src/libsync/bulkpropagatorjob.cpp +++ b/src/libsync/bulkpropagatorjob.cpp @@ -479,7 +479,7 @@ void BulkPropagatorJob::adjustLastJobTimeout(AbstractNetworkJob *job, qint64 fil void BulkPropagatorJob::finalizeOneFile(const BulkUploadItem &oneFile) { // Update the database entry - const auto result = propagator()->updateMetadata(*oneFile._item); + const auto result = propagator()->updateMetadata(*oneFile._item, Vfs::UpdateMetadataType::DatabaseMetadata); if (!result) { done(oneFile._item, SyncFileItem::FatalError, tr("Error updating metadata: %1").arg(result.error()), ErrorCategory::GenericError); return; diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 488a83a3f0ff..b2a058088c30 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -1001,16 +1001,19 @@ QString OwncloudPropagator::adjustRenamedPath(const QString &original) const return OCC::adjustRenamedPath(_renamedDirectories, original); } -Result OwncloudPropagator::updateMetadata(const SyncFileItem &item) +Result OwncloudPropagator::updateMetadata(const SyncFileItem &item, Vfs::UpdateMetadataTypes updateType) { - return OwncloudPropagator::staticUpdateMetadata(item, _localDir, syncOptions()._vfs.data(), _journal); + return OwncloudPropagator::staticUpdateMetadata(item, _localDir, syncOptions()._vfs.data(), _journal, updateType); } -Result OwncloudPropagator::staticUpdateMetadata(const SyncFileItem &item, const QString localDir, - Vfs *vfs, SyncJournalDb *const journal) +Result OwncloudPropagator::staticUpdateMetadata(const SyncFileItem &item, + const QString localDir, + Vfs *vfs, + SyncJournalDb *const journal, + Vfs::UpdateMetadataTypes updateType) { const QString fsPath = localDir + item.destination(); - const auto result = vfs->convertToPlaceholder(fsPath, item); + const auto result = vfs->convertToPlaceholder(fsPath, item, {}, updateType); if (!result) { return result.error(); } else if (*result == Vfs::ConvertToPlaceholderResult::Locked) { @@ -1582,7 +1585,7 @@ void CleanupPollsJob::slotPollFinished() } else if (job->_item->_status != SyncFileItem::Success) { qCWarning(lcCleanupPolls) << "There was an error with file " << job->_item->_file << job->_item->_errorString; } else { - if (!OwncloudPropagator::staticUpdateMetadata(*job->_item, _localPath, _vfs.data(), _journal)) { + if (!OwncloudPropagator::staticUpdateMetadata(*job->_item, _localPath, _vfs.data(), _journal, Vfs::AllMetadata)) { qCWarning(lcCleanupPolls) << "database error"; job->_item->_status = SyncFileItem::FatalError; job->_item->_errorString = tr("Error writing metadata to the database"); diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h index 46f0ea4e5ec2..440b98551e51 100644 --- a/src/libsync/owncloudpropagator.h +++ b/src/libsync/owncloudpropagator.h @@ -27,13 +27,15 @@ #include "accountfwd.h" #include "bandwidthmanager.h" -#include "common/syncjournaldb.h" -#include "common/utility.h" #include "csync.h" #include "progressdispatcher.h" #include "syncfileitem.h" #include "syncoptions.h" +#include "common/syncjournaldb.h" +#include "common/utility.h" +#include "common/vfs.h" + #include namespace OCC { @@ -592,7 +594,7 @@ class OWNCLOUDSYNC_EXPORT OwncloudPropagator : public QObject * * Will also trigger a Vfs::convertToPlaceholder. */ - Result updateMetadata(const SyncFileItem &item); + Result updateMetadata(const SyncFileItem &item, Vfs::UpdateMetadataTypes updateType = Vfs::AllMetadata); /** Update the database for an item. * @@ -601,8 +603,11 @@ class OWNCLOUDSYNC_EXPORT OwncloudPropagator : public QObject * * Will also trigger a Vfs::convertToPlaceholder. */ - static Result staticUpdateMetadata(const SyncFileItem &item, const QString localDir, - Vfs *vfs, SyncJournalDb * const journal); + static Result staticUpdateMetadata(const SyncFileItem &item, + const QString localDir, + Vfs *vfs, + SyncJournalDb * const journal, + Vfs::UpdateMetadataTypes updateType); Q_REQUIRED_RESULT bool isDelayedUploadItem(const SyncFileItemPtr &item) const; diff --git a/src/libsync/propagateupload.cpp b/src/libsync/propagateupload.cpp index 2a7a7bbaabc8..f74727ec7bb8 100644 --- a/src/libsync/propagateupload.cpp +++ b/src/libsync/propagateupload.cpp @@ -800,7 +800,7 @@ void PropagateUploadFileCommon::finalize() quotaIt.value() -= _fileToUpload._size; // Update the database entry - const auto result = propagator()->updateMetadata(*_item); + const auto result = propagator()->updateMetadata(*_item, Vfs::DatabaseMetadata); if (!result) { done(SyncFileItem::FatalError, tr("Error updating metadata: %1").arg(result.error())); return; diff --git a/src/libsync/vfs/cfapi/cfapiwrapper.cpp b/src/libsync/vfs/cfapi/cfapiwrapper.cpp index ee729896300a..bd445042dcae 100644 --- a/src/libsync/vfs/cfapi/cfapiwrapper.cpp +++ b/src/libsync/vfs/cfapi/cfapiwrapper.cpp @@ -48,6 +48,30 @@ constexpr auto syncRootFlagsNoCfApiContextMenu = 2; constexpr auto syncRootManagerRegKey = R"(SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\SyncRootManager)"; +DWORD sizeToDWORD(size_t size) +{ + return OCC::Utility::convertSizeToDWORD(size); +} + +OCC::PinState cfPinStateToPinState(CF_PIN_STATE state) +{ + switch (state) { + case CF_PIN_STATE_UNSPECIFIED: + return OCC::PinState::Unspecified; + case CF_PIN_STATE_PINNED: + return OCC::PinState::AlwaysLocal; + case CF_PIN_STATE_UNPINNED: + return OCC::PinState::OnlineOnly; + case CF_PIN_STATE_INHERIT: + return OCC::PinState::Inherited; + case CF_PIN_STATE_EXCLUDED: + return OCC::PinState::Excluded; + default: + Q_UNREACHABLE(); + return OCC::PinState::Inherited; + } +} + void cfApiSendTransferInfo(const CF_CONNECTION_KEY &connectionKey, const CF_TRANSFER_KEY &transferKey, NTSTATUS status, void *buffer, qint64 offset, qint64 currentBlockLength, qint64 totalLength) { @@ -237,6 +261,53 @@ void CALLBACK cfApiFetchDataCallback(const CF_CALLBACK_INFO *callbackInfo, const sendTransferError(); } } + +enum class CfApiUpdateMetadataType { + OnlyBasicMetadata, + AllMetadata, +}; + +OCC::Result updatePlaceholderState(const QString &path, time_t modtime, qint64 size, const QByteArray &fileId, const QString &replacesPath, CfApiUpdateMetadataType updateType) +{ + if (updateType == CfApiUpdateMetadataType::AllMetadata && modtime <= 0) { + return {QString{"Could not update metadata due to invalid modification time for %1: %2"}.arg(path).arg(modtime)}; + } + + const auto info = replacesPath.isEmpty() ? OCC::CfApiWrapper::findPlaceholderInfo(path) + : OCC::CfApiWrapper::findPlaceholderInfo(replacesPath); + if (!info) { + return { "Can't update non existing placeholder info" }; + } + + const auto previousPinState = cfPinStateToPinState(info->PinState); + const auto fileIdentity = QString::fromUtf8(fileId).toStdWString(); + const auto fileIdentitySize = (fileIdentity.length() + 1) * sizeof(wchar_t); + + CF_FS_METADATA metadata; + metadata.FileSize.QuadPart = size; + OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.CreationTime); + OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.LastWriteTime); + OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.LastAccessTime); + OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.ChangeTime); + metadata.BasicInfo.FileAttributes = 0; + + qCInfo(lcCfApiWrapper) << "updatePlaceholderState" << path << modtime; + const qint64 result = CfUpdatePlaceholder(OCC::CfApiWrapper::handleForPath(path).get(), updateType == CfApiUpdateMetadataType::AllMetadata ? &metadata : nullptr, + fileIdentity.data(), sizeToDWORD(fileIdentitySize), + nullptr, 0, CF_UPDATE_FLAG_MARK_IN_SYNC, nullptr, nullptr); + + if (result != S_OK) { + qCWarning(lcCfApiWrapper) << "Couldn't update placeholder info for" << path << ":" << QString::fromWCharArray(_com_error(result).ErrorMessage()) << replacesPath; + return { "Couldn't update placeholder info" }; + } + + // Pin state tends to be lost on updates, so restore it every time + if (!setPinState(path, previousPinState, OCC::CfApiWrapper::NoRecurse)) { + return { "Couldn't restore pin state" }; + } + + return OCC::Vfs::ConvertToPlaceholderResult::Ok; +} } void CALLBACK cfApiCancelFetchData(const CF_CALLBACK_INFO *callbackInfo, const CF_CALLBACK_PARAMETERS * /*callbackParameters*/) @@ -262,11 +333,6 @@ CF_CALLBACK_REGISTRATION cfApiCallbacks[] = { CF_CALLBACK_REGISTRATION_END }; -DWORD sizeToDWORD(size_t size) -{ - return OCC::Utility::convertSizeToDWORD(size); -} - void deletePlaceholderInfo(CF_PLACEHOLDER_BASIC_INFO *info) { auto byte = reinterpret_cast(info); @@ -281,25 +347,6 @@ std::wstring pathForHandle(const OCC::CfApiWrapper::FileHandle &handle) return std::wstring(buffer); } -OCC::PinState cfPinStateToPinState(CF_PIN_STATE state) -{ - switch (state) { - case CF_PIN_STATE_UNSPECIFIED: - return OCC::PinState::Unspecified; - case CF_PIN_STATE_PINNED: - return OCC::PinState::AlwaysLocal; - case CF_PIN_STATE_UNPINNED: - return OCC::PinState::OnlineOnly; - case CF_PIN_STATE_INHERIT: - return OCC::PinState::Inherited; - case CF_PIN_STATE_EXCLUDED: - return OCC::PinState::Excluded; - default: - Q_UNREACHABLE(); - return OCC::PinState::Inherited; - } -} - CF_PIN_STATE pinStateToCfPinState(OCC::PinState state) { switch (state) { @@ -750,44 +797,7 @@ OCC::Result OCC::CfApiWrapper::createPlaceholderInfo(const QStrin OCC::Result OCC::CfApiWrapper::updatePlaceholderInfo(const QString &path, time_t modtime, qint64 size, const QByteArray &fileId, const QString &replacesPath) { - - if (modtime <= 0) { - return {QString{"Could not update metadata due to invalid modification time for %1: %2"}.arg(path).arg(modtime)}; - } - - const auto info = replacesPath.isEmpty() ? findPlaceholderInfo(path) - : findPlaceholderInfo(replacesPath); - if (!info) { - return { "Can't update non existing placeholder info" }; - } - - const auto previousPinState = cfPinStateToPinState(info->PinState); - const auto fileIdentity = QString::fromUtf8(fileId).toStdWString(); - const auto fileIdentitySize = (fileIdentity.length() + 1) * sizeof(wchar_t); - - CF_FS_METADATA metadata; - metadata.FileSize.QuadPart = size; - OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.CreationTime); - OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.LastWriteTime); - OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.LastAccessTime); - OCC::Utility::UnixTimeToLargeIntegerFiletime(modtime, &metadata.BasicInfo.ChangeTime); - metadata.BasicInfo.FileAttributes = 0; - - const qint64 result = CfUpdatePlaceholder(handleForPath(path).get(), &metadata, - fileIdentity.data(), sizeToDWORD(fileIdentitySize), - nullptr, 0, CF_UPDATE_FLAG_MARK_IN_SYNC, nullptr, nullptr); - - if (result != S_OK) { - qCWarning(lcCfApiWrapper) << "Couldn't update placeholder info for" << path << ":" << QString::fromWCharArray(_com_error(result).ErrorMessage()) << replacesPath; - return { "Couldn't update placeholder info" }; - } - - // Pin state tends to be lost on updates, so restore it every time - if (!setPinState(path, previousPinState, NoRecurse)) { - return { "Couldn't restore pin state" }; - } - - return OCC::Vfs::ConvertToPlaceholderResult::Ok; + return updatePlaceholderState(path, modtime, size, fileId, replacesPath, CfApiUpdateMetadataType::AllMetadata); } OCC::Result OCC::CfApiWrapper::dehydratePlaceholder(const QString &path, time_t modtime, qint64 size, const QByteArray &fileId) @@ -862,3 +872,8 @@ OCC::Result OCC::CfApiWrapper::co return stateResult; } } + +OCC::Result OCC::CfApiWrapper::updatePlaceholderMarkInSync(const QString &path, const QByteArray &fileId, const QString &replacesPath) +{ + return updatePlaceholderState(path, {}, {}, fileId, replacesPath, CfApiUpdateMetadataType::OnlyBasicMetadata); +} diff --git a/src/libsync/vfs/cfapi/cfapiwrapper.h b/src/libsync/vfs/cfapi/cfapiwrapper.h index 10cb51aaa82b..94ebff914cbe 100644 --- a/src/libsync/vfs/cfapi/cfapiwrapper.h +++ b/src/libsync/vfs/cfapi/cfapiwrapper.h @@ -97,6 +97,7 @@ NEXTCLOUD_CFAPI_EXPORT Result createPlaceholderInfo(const QString NEXTCLOUD_CFAPI_EXPORT Result updatePlaceholderInfo(const QString &path, time_t modtime, qint64 size, const QByteArray &fileId, const QString &replacesPath = QString()); NEXTCLOUD_CFAPI_EXPORT Result convertToPlaceholder(const QString &path, time_t modtime, qint64 size, const QByteArray &fileId, const QString &replacesPath); NEXTCLOUD_CFAPI_EXPORT Result dehydratePlaceholder(const QString &path, time_t modtime, qint64 size, const QByteArray &fileId); +NEXTCLOUD_CFAPI_EXPORT Result updatePlaceholderMarkInSync(const QString &path, const QByteArray &fileId, const QString &replacesPath = QString()); } diff --git a/src/libsync/vfs/cfapi/vfs_cfapi.cpp b/src/libsync/vfs/cfapi/vfs_cfapi.cpp index 595cc5215918..e7905fe6ca7c 100644 --- a/src/libsync/vfs/cfapi/vfs_cfapi.cpp +++ b/src/libsync/vfs/cfapi/vfs_cfapi.cpp @@ -222,7 +222,7 @@ Result VfsCfApi::dehydratePlaceholder(const SyncFileItem &item) } } -Result VfsCfApi::convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &replacesFile) +Result VfsCfApi::convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &replacesFile, UpdateMetadataTypes updateType) { const auto localPath = QDir::toNativeSeparators(filename); @@ -238,7 +238,11 @@ Result VfsCfApi::convertToPlaceholder( const auto replacesPath = QDir::toNativeSeparators(replacesFile); if (cfapi::findPlaceholderInfo(localPath)) { - return cfapi::updatePlaceholderInfo(localPath, item._modtime, item._size, item._fileId, replacesPath); + if (updateType & Vfs::UpdateMetadataType::FileMetadata) { + return cfapi::updatePlaceholderInfo(localPath, item._modtime, item._size, item._fileId, replacesPath); + } else { + return cfapi::updatePlaceholderMarkInSync(localPath, item._fileId, replacesPath); + } } else { return cfapi::convertToPlaceholder(localPath, item._modtime, item._size, item._fileId, replacesPath); } diff --git a/src/libsync/vfs/cfapi/vfs_cfapi.h b/src/libsync/vfs/cfapi/vfs_cfapi.h index b675ae389f0e..ce0d97ea0ccb 100644 --- a/src/libsync/vfs/cfapi/vfs_cfapi.h +++ b/src/libsync/vfs/cfapi/vfs_cfapi.h @@ -45,7 +45,7 @@ class VfsCfApi : public Vfs Result createPlaceholder(const SyncFileItem &item) override; Result dehydratePlaceholder(const SyncFileItem &item) override; - Result convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &replacesFile) override; + Result convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &replacesFile, UpdateMetadataTypes updateType) override; bool needsMetadataUpdate(const SyncFileItem &) override; bool isDehydratedPlaceholder(const QString &filePath) override; diff --git a/src/libsync/vfs/suffix/vfs_suffix.cpp b/src/libsync/vfs/suffix/vfs_suffix.cpp index 6fcb1d2f51da..f4d5e9a05d72 100644 --- a/src/libsync/vfs/suffix/vfs_suffix.cpp +++ b/src/libsync/vfs/suffix/vfs_suffix.cpp @@ -138,7 +138,7 @@ Result VfsSuffix::dehydratePlaceholder(const SyncFileItem &item) return {}; } -Result VfsSuffix::convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) +Result VfsSuffix::convertToPlaceholder(const QString &, const SyncFileItem &, const QString &, UpdateMetadataTypes) { // Nothing necessary return Vfs::ConvertToPlaceholderResult::Ok; diff --git a/src/libsync/vfs/suffix/vfs_suffix.h b/src/libsync/vfs/suffix/vfs_suffix.h index 6169290ba322..3164802d9c83 100644 --- a/src/libsync/vfs/suffix/vfs_suffix.h +++ b/src/libsync/vfs/suffix/vfs_suffix.h @@ -42,7 +42,7 @@ class VfsSuffix : public Vfs Result createPlaceholder(const SyncFileItem &item) override; Result dehydratePlaceholder(const SyncFileItem &item) override; - Result convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &) override; + Result convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &, UpdateMetadataTypes updateType) override; bool needsMetadataUpdate(const SyncFileItem &) override { return false; } bool isDehydratedPlaceholder(const QString &filePath) override; diff --git a/src/libsync/vfs/xattr/vfs_xattr.cpp b/src/libsync/vfs/xattr/vfs_xattr.cpp index 4300f6d30ad2..f6ba09f1e365 100644 --- a/src/libsync/vfs/xattr/vfs_xattr.cpp +++ b/src/libsync/vfs/xattr/vfs_xattr.cpp @@ -122,7 +122,10 @@ Result VfsXAttr::dehydratePlaceholder(const SyncFileItem &item) return {}; } -Result VfsXAttr::convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) +Result VfsXAttr::convertToPlaceholder(const QString &, + const SyncFileItem &, + const QString &, + UpdateMetadataTypes) { // Nothing necessary return {ConvertToPlaceholderResult::Ok}; diff --git a/src/libsync/vfs/xattr/vfs_xattr.h b/src/libsync/vfs/xattr/vfs_xattr.h index 118d4d2864f9..b9fc9548c470 100644 --- a/src/libsync/vfs/xattr/vfs_xattr.h +++ b/src/libsync/vfs/xattr/vfs_xattr.h @@ -42,7 +42,10 @@ class VfsXAttr : public Vfs Result createPlaceholder(const SyncFileItem &item) override; Result dehydratePlaceholder(const SyncFileItem &item) override; - Result convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &replacesFile) override; + Result convertToPlaceholder(const QString &filename, + const SyncFileItem &item, + const QString &replacesFile, + UpdateMetadataTypes updateType) override; bool needsMetadataUpdate(const SyncFileItem &item) override; bool isDehydratedPlaceholder(const QString &filePath) override;