From 9a06128eec7f568c96c71c627a953a2cce6d9700 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Wed, 26 Oct 2022 18:13:55 +0200 Subject: [PATCH 1/5] remove dead code related to item renaming Signed-off-by: Matthieu Gallien --- src/libsync/discovery.cpp | 65 ------------------------------------ src/libsync/discoveryphase.h | 2 -- 2 files changed, 67 deletions(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 57426dd8a175b..8605d625553c3 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -615,24 +615,6 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo( // Unknown in db: new file on the server Q_ASSERT(!dbEntry.isValid()); - if (!serverEntry.renameName.isEmpty()) { - item->_renameTarget = _dirItem ? _dirItem->_file + "/" + serverEntry.renameName : serverEntry.renameName; - item->_originalFile = path._original; - item->_modtime = serverEntry.modtime; - item->_size = serverEntry.size; - item->_instruction = CSYNC_INSTRUCTION_RENAME; - item->_direction = SyncFileItem::Up; - item->_fileId = serverEntry.fileId; - item->_remotePerm = serverEntry.remotePerm; - item->_isShared = serverEntry.remotePerm.hasPermission(RemotePermissions::IsShared); - item->_lastShareStateFetchedTimestmap = QDateTime::currentMSecsSinceEpoch(); - item->_etag = serverEntry.etag; - item->_type = serverEntry.isDirectory ? CSyncEnums::ItemTypeDirectory : CSyncEnums::ItemTypeFile; - - processFileAnalyzeLocalInfo(item, path, localEntry, serverEntry, dbEntry, _queryServer); - return; - } - item->_instruction = CSYNC_INSTRUCTION_NEW; item->_direction = SyncFileItem::Down; item->_modtime = serverEntry.modtime; @@ -881,41 +863,6 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( processFileFinalize(item, path, recurse, recurseQueryLocal, recurseQueryServer); }; - auto handleInvalidSpaceRename = [&] (SyncFileItem::Direction direction) { - if (_dirItem) { - path._target = _dirItem->_file + "/" + localEntry.renameName; - } else { - path._target = localEntry.renameName; - } - OCC::SyncJournalFileRecord base; - if (!_discoveryData->_statedb->getFileRecordByInode(localEntry.inode, &base)) { - dbError(); - return; - } - const auto originalPath = base.path(); - const auto adjustedOriginalPath = _discoveryData->adjustRenamedPath(originalPath, SyncFileItem::Down); - _discoveryData->_renamedItemsLocal.insert(originalPath, path._target); - item->_renameTarget = path._target; - path._server = adjustedOriginalPath; - if (_dirItem) { - item->_file = _dirItem->_file + "/" + localEntry.name; - } else { - item->_file = localEntry.name; - } - path._original = originalPath; - item->_originalFile = path._original; - item->_modtime = base.isValid() ? base._modtime : localEntry.modtime; - item->_inode = base.isValid() ? base._inode : localEntry.inode; - item->_instruction = CSYNC_INSTRUCTION_RENAME; - item->_direction = direction; - item->_fileId = base.isValid() ? base._fileId : QByteArray{}; - item->_remotePerm = base.isValid() ? base._remotePerm : RemotePermissions{}; - item->_etag = base.isValid() ? base._etag : QByteArray{}; - item->_type = base.isValid() ? base._type : localEntry.type; - item->_isShared = base.isValid() ? base._isShared : false; - item->_lastShareStateFetchedTimestmap = base.isValid() ? base._lastShareStateFetchedTimestmap : 0; - }; - if (!localEntry.isValid()) { if (_queryLocal == ParentNotChanged && dbEntry.isValid()) { // Not modified locally (ParentNotChanged) @@ -1000,8 +947,6 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( || _discoveryData->_syncOptions._vfs->needsMetadataUpdate(*item))) { item->_instruction = CSYNC_INSTRUCTION_UPDATE_METADATA; item->_direction = SyncFileItem::Down; - } else if (!localEntry.renameName.isEmpty()) { - handleInvalidSpaceRename(SyncFileItem::Up); } } else if (!typeChange && isVfsWithSuffix() && dbEntry.isVirtualFile() && !localEntry.isVirtualFile @@ -1097,16 +1042,6 @@ void ProcessDirectoryJob::processFileAnalyzeLocalInfo( return; } - if (!localEntry.renameName.isEmpty()) { - handleInvalidSpaceRename(SyncFileItem::Down); - item->_instruction = CSYNC_INSTRUCTION_NEW; - item->_direction = SyncFileItem::Up; - item->_originalFile = item->_file; - item->_file = item->_renameTarget; - finalize(); - return; - } - // New local file or rename item->_instruction = CSYNC_INSTRUCTION_NEW; item->_direction = SyncFileItem::Up; diff --git a/src/libsync/discoveryphase.h b/src/libsync/discoveryphase.h index 0acb862d2518c..6c633d1bf919f 100644 --- a/src/libsync/discoveryphase.h +++ b/src/libsync/discoveryphase.h @@ -49,7 +49,6 @@ struct RemoteInfo { /** FileName of the entry (this does not contains any directory or path, just the plain name */ QString name; - QString renameName; QByteArray etag; QByteArray fileId; QByteArray checksumHeader; @@ -79,7 +78,6 @@ struct LocalInfo { /** FileName of the entry (this does not contains any directory or path, just the plain name */ QString name; - QString renameName; time_t modtime = 0; int64_t size = 0; uint64_t inode = 0; From 7fe6a3df78875d3fb9a03ae918edc344748bf74d Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 27 Oct 2022 12:49:34 +0200 Subject: [PATCH 2/5] logs from discovery are now namespaced by nextcloud to show debug logs use nextcloud namespace for discovery logs as it does not show correcly without that this is because we have central rules for all logs from nextcloud namespace Signed-off-by: Matthieu Gallien --- src/libsync/discovery.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 8605d625553c3..047a1bbc28a8c 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -35,7 +35,7 @@ namespace OCC { -Q_LOGGING_CATEGORY(lcDisco, "sync.discovery", QtInfoMsg) +Q_LOGGING_CATEGORY(lcDisco, "nextcloud.sync.discovery", QtInfoMsg) void ProcessDirectoryJob::start() { From f8fc720fbb6aadce5e457567621d37eee26ab72b Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 27 Oct 2022 13:13:21 +0200 Subject: [PATCH 3/5] improve logs of E2EE API to not pollute logs Signed-off-by: Matthieu Gallien --- src/libsync/capabilities.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libsync/capabilities.cpp b/src/libsync/capabilities.cpp index da713b46cfe48..1f4131e00a35b 100644 --- a/src/libsync/capabilities.cpp +++ b/src/libsync/capabilities.cpp @@ -143,24 +143,27 @@ bool Capabilities::clientSideEncryptionAvailable() const } const auto version = properties.value(QStringLiteral("api-version"), "1.0").toByteArray(); - qCInfo(lcServerCapabilities) << "E2EE API version:" << version; const auto splittedVersion = version.split('.'); bool ok = false; const auto major = !splittedVersion.isEmpty() ? splittedVersion.at(0).toInt(&ok) : 0; if (!ok) { - qCWarning(lcServerCapabilities) << "Didn't understand version scheme (major), E2EE disabled"; + qCWarning(lcServerCapabilities) << "Didn't understand version scheme (major), E2EE disabled" << version; return false; } ok = false; const auto minor = splittedVersion.size() > 1 ? splittedVersion.at(1).toInt(&ok) : 0; if (!ok) { - qCWarning(lcServerCapabilities) << "Didn't understand version scheme (minor), E2EE disabled"; + qCWarning(lcServerCapabilities) << "Didn't understand version scheme (minor), E2EE disabled" << version; return false; } - return major == 1 && minor >= 1; + const auto capabilityAvailable = (major == 1 && minor >= 1); + if (!capabilityAvailable) { + qCInfo(lcServerCapabilities) << "Incompatible E2EE API version:" << version; + } + return capabilityAvailable; } bool Capabilities::notificationsAvailable() const From 783cefe3f1efee67a99fc5048d5ce5bc3d284509 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 27 Oct 2022 18:00:10 +0200 Subject: [PATCH 4/5] new autotest to ensure that we delete folders despite blacklist errors Signed-off-by: Matthieu Gallien --- test/testsyncmove.cpp | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp index f1488c8189b75..c8f5ac2d68e00 100644 --- a/test/testsyncmove.cpp +++ b/test/testsyncmove.cpp @@ -890,6 +890,29 @@ private slots: QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); } + void testRenameParallelismWithBlacklist() + { + constexpr auto testFileName = "blackListFile"; + FakeFolder fakeFolder{ FileInfo{} }; + fakeFolder.remoteModifier().mkdir("A"); + fakeFolder.remoteModifier().insert("A/file"); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + + fakeFolder.remoteModifier().insert(testFileName); + fakeFolder.serverErrorPaths().append(testFileName, 500); // will be blacklisted + QVERIFY(!fakeFolder.syncOnce()); + + fakeFolder.remoteModifier().mkdir("B"); + fakeFolder.remoteModifier().rename("A/file", "B/file"); + fakeFolder.remoteModifier().remove("A"); + + QVERIFY(!fakeFolder.syncOnce()); + auto folderA = fakeFolder.currentLocalState().find("A"); + QCOMPARE(folderA, nullptr); + } + void testMovedWithError_data() { QTest::addColumn("vfsMode"); From 402d959caea89c461366a232e807dced2c712034 Mon Sep 17 00:00:00 2001 From: Matthieu Gallien Date: Thu, 27 Oct 2022 17:29:02 +0200 Subject: [PATCH 5/5] do not skip folder deletion if BlacklistedError during propagation in case some propgation actions ends up with a BlacklistedError, we would skip the deletions of local folders (even though they have been deleted from server) let's not skip them but rather keep track of the error status and properly report them Signed-off-by: Matthieu Gallien --- src/libsync/owncloudpropagator.cpp | 34 +++++++++++++++++++++++++++++- src/libsync/owncloudpropagator.h | 9 ++++++-- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/src/libsync/owncloudpropagator.cpp b/src/libsync/owncloudpropagator.cpp index 198f1b73b392a..cb3734e17a130 100644 --- a/src/libsync/owncloudpropagator.cpp +++ b/src/libsync/owncloudpropagator.cpp @@ -594,7 +594,7 @@ void OwncloudPropagator::start(SyncFileItemVector &&items) } foreach (PropagatorJob *it, directoriesToRemove) { - _rootJob->_dirDeletionJobs.appendJob(it); + _rootJob->appendDirDeletionJob(it); } connect(_rootJob.data(), &PropagatorJob::finished, this, &OwncloudPropagator::emitFinished); @@ -1292,6 +1292,11 @@ qint64 PropagateRootDirectory::committedDiskSpace() const return _subJobs.committedDiskSpace() + _dirDeletionJobs.committedDiskSpace(); } +void PropagateRootDirectory::appendDirDeletionJob(PropagatorJob *job) +{ + _dirDeletionJobs.appendJob(job); +} + bool PropagateRootDirectory::scheduleSelfOrChild() { qCInfo(lcRootDirectory()) << "scheduleSelfOrChild" << _state << "pending uploads" << propagator()->delayedTasks().size() << "subjobs state" << _subJobs._state; @@ -1327,6 +1332,7 @@ void PropagateRootDirectory::slotSubJobsFinished(SyncFileItem::Status status) if (status != SyncFileItem::Success && status != SyncFileItem::Restoration + && status != SyncFileItem::BlacklistedError && status != SyncFileItem::Conflict) { if (_state != Finished) { // Synchronously abort @@ -1338,11 +1344,37 @@ void PropagateRootDirectory::slotSubJobsFinished(SyncFileItem::Status status) return; } + 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::FileNameClash: + case SyncFileItem::DetailError: + case SyncFileItem::Success: + break; + case SyncFileItem::BlacklistedError: + _errorStatus = SyncFileItem::BlacklistedError; + break; + } + } + propagator()->scheduleNextJob(); } void PropagateRootDirectory::slotDirDeletionJobsFinished(SyncFileItem::Status status) { + if (_errorStatus != SyncFileItem::NoStatus && status == SyncFileItem::Success) { + qCInfo(lcPropagator) << "PropagateRootDirectory::slotDirDeletionJobsFinished" << "reporting previous error" << _errorStatus; + status = _errorStatus; + } + _state = Finished; qCInfo(lcPropagator) << "PropagateRootDirectory::slotDirDeletionJobsFinished" << "emit finished" << status; emit finished(status); diff --git a/src/libsync/owncloudpropagator.h b/src/libsync/owncloudpropagator.h index 87e695afc3e61..95fe4e74310c1 100644 --- a/src/libsync/owncloudpropagator.h +++ b/src/libsync/owncloudpropagator.h @@ -365,8 +365,6 @@ class OWNCLOUDSYNC_EXPORT PropagateRootDirectory : public PropagateDirectory { Q_OBJECT public: - PropagatorCompositeJob _dirDeletionJobs; - explicit PropagateRootDirectory(OwncloudPropagator *propagator); bool scheduleSelfOrChild() override; @@ -375,6 +373,9 @@ class OWNCLOUDSYNC_EXPORT PropagateRootDirectory : public PropagateDirectory [[nodiscard]] qint64 committedDiskSpace() const override; +public slots: + void appendDirDeletionJob(PropagatorJob *job); + private slots: void slotSubJobsFinished(SyncFileItem::Status status) override; void slotDirDeletionJobsFinished(SyncFileItem::Status status); @@ -382,6 +383,10 @@ private slots: private: bool scheduleDelayedJobs(); + + PropagatorCompositeJob _dirDeletionJobs; + + SyncFileItem::Status _errorStatus = SyncFileItem::Status::NoStatus; }; /**