From: Olivier Goffart Date: Wed, 30 Oct 2019 12:16:32 +0000 (+0100) Subject: VFS: Do not overwrite existing files by placeholder X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~21^2~468^2~159 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=66f7b271211955f984dc81a64926385e12940096;p=nextcloud-desktop.git VFS: Do not overwrite existing files by placeholder For issue #7557 and #7556 Note: this change the API of the VFS plugin, so the VFS plugin needs small adaptations --- diff --git a/src/common/result.h b/src/common/result.h index 61805982e..579914f5c 100644 --- a/src/common/result.h +++ b/src/common/result.h @@ -128,6 +128,18 @@ public: } }; +namespace detail { + struct NoResultData{}; +} + +template +class Result : public Result +{ +public: + using Result::Result; + Result() : Result(detail::NoResultData{}) {} +}; + namespace detail { struct OptionalNoErrorData{}; } diff --git a/src/common/vfs.h b/src/common/vfs.h index ca28b631b..669e3f5b6 100644 --- a/src/common/vfs.h +++ b/src/common/vfs.h @@ -151,20 +151,18 @@ public: * * If the remote metadata changes, the local placeholder's metadata should possibly * change as well. - * - * Returning false and setting error indicates an error. */ - virtual bool updateMetadata(const QString &filePath, time_t modtime, qint64 size, const QByteArray &fileId, QString *error) = 0; + virtual Result updateMetadata(const QString &filePath, time_t modtime, qint64 size, const QByteArray &fileId) = 0; /// Create a new dehydrated placeholder. Called from PropagateDownload. - virtual void createPlaceholder(const SyncFileItem &item) = 0; + virtual Result createPlaceholder(const SyncFileItem &item) = 0; /** Convert a hydrated placeholder to a dehydrated one. Called from PropagateDownlaod. * * This is different from delete+create because preserving some file metadata * (like pin states) may be essential for some vfs plugins. */ - virtual void dehydratePlaceholder(const SyncFileItem &item) = 0; + virtual Result dehydratePlaceholder(const SyncFileItem &item) = 0; /** Discovery hook: even unchanged files may need UPDATE_METADATA. * @@ -289,9 +287,9 @@ public: bool socketApiPinStateActionsShown() const override { return false; } bool isHydrating() const override { return false; } - bool updateMetadata(const QString &, time_t, qint64, const QByteArray &, QString *) override { return true; } - void createPlaceholder(const SyncFileItem &) override {} - void dehydratePlaceholder(const SyncFileItem &) override {} + Result updateMetadata(const QString &, time_t, qint64, const QByteArray &) override { return {}; } + Result createPlaceholder(const SyncFileItem &) override { return {}; } + Result dehydratePlaceholder(const SyncFileItem &) override { return {}; } void convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) override {} bool needsMetadataUpdate(const SyncFileItem &) override { return false; } diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index 7ac691df1..bdd250cee 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -424,7 +424,11 @@ void PropagateDownloadFile::startAfterIsEncryptedIsChecked() } qCDebug(lcPropagateDownload) << "dehydrating file" << _item->_file; - vfs->dehydratePlaceholder(*_item); + auto r = vfs->dehydratePlaceholder(*_item); + if (!r) { + done(SyncFileItem::NormalError, r.error()); + return; + } propagator()->_journal->deleteFileRecord(_item->_originalFile); updateMetadata(false); return; @@ -435,7 +439,11 @@ void PropagateDownloadFile::startAfterIsEncryptedIsChecked() } if (_item->_type == ItemTypeVirtualFile) { qCDebug(lcPropagateDownload) << "creating virtual file" << _item->_file; - vfs->createPlaceholder(*_item); + auto r = vfs->createPlaceholder(*_item); + if (!r) { + done(SyncFileItem::NormalError, r.error()); + return; + } updateMetadata(false); return; } diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index bb2a83a25..a185dd2b0 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -351,10 +351,10 @@ void OCC::SyncEngine::slotItemDiscovered(const OCC::SyncFileItemPtr &item) // Update on-disk virtual file metadata if (item->_type == ItemTypeVirtualFile) { - QString error; - if (!_syncOptions._vfs->updateMetadata(filePath, item->_modtime, item->_size, item->_fileId, &error)) { + auto r = _syncOptions._vfs->updateMetadata(filePath, item->_modtime, item->_size, item->_fileId); + if (!r) { item->_instruction = CSYNC_INSTRUCTION_ERROR; - item->_errorString = tr("Could not update virtual file metadata: %1").arg(error); + item->_errorString = tr("Could not update virtual file metadata: %1").arg(r.error()); return; } } diff --git a/src/libsync/vfs/suffix/vfs_suffix.cpp b/src/libsync/vfs/suffix/vfs_suffix.cpp index 6827126a9..be64b62a6 100644 --- a/src/libsync/vfs/suffix/vfs_suffix.cpp +++ b/src/libsync/vfs/suffix/vfs_suffix.cpp @@ -68,34 +68,47 @@ bool VfsSuffix::isHydrating() const return false; } -bool VfsSuffix::updateMetadata(const QString &filePath, time_t modtime, qint64, const QByteArray &, QString *) +Result VfsSuffix::updateMetadata(const QString &filePath, time_t modtime, qint64, const QByteArray &) { FileSystem::setModTime(filePath, modtime); - return true; + return {}; } -void VfsSuffix::createPlaceholder(const SyncFileItem &item) +Result VfsSuffix::createPlaceholder(const SyncFileItem &item) { // The concrete shape of the placeholder is also used in isDehydratedPlaceholder() below QString fn = _setupParams.filesystemPath + item._file; if (!fn.endsWith(fileSuffix())) { ASSERT(false, "vfs file isn't ending with suffix"); - return; + return QString("vfs file isn't ending with suffix"); } QFile file(fn); - file.open(QFile::ReadWrite | QFile::Truncate); + if (file.exists() && file.size() > 1 + && !FileSystem::verifyFileUnchanged(fn, item._size, item._modtime)) { + return QString("Cannot create a placeholder because a file with the placeholder name already exist"); + } + + if (!file.open(QFile::ReadWrite | QFile::Truncate)) + return file.errorString(); + file.write(" "); file.close(); FileSystem::setModTime(fn, item._modtime); + return {}; } -void VfsSuffix::dehydratePlaceholder(const SyncFileItem &item) +Result VfsSuffix::dehydratePlaceholder(const SyncFileItem &item) { - QFile::remove(_setupParams.filesystemPath + item._file); SyncFileItem virtualItem(item); virtualItem._file = item._renameTarget; - createPlaceholder(virtualItem); + auto r = createPlaceholder(virtualItem); + if (!r) + return r; + + if (item._file != item._renameTarget) { // can be the same when renaming foo -> foo.owncloud to dehydrate + QFile::remove(_setupParams.filesystemPath + item._file); + } // Move the item's pin state auto pin = _setupParams.journal->internalPinStates().rawForPath(item._file.toUtf8()); @@ -108,6 +121,7 @@ void VfsSuffix::dehydratePlaceholder(const SyncFileItem &item) pin = pinState(item._renameTarget); if (pin && *pin == PinState::AlwaysLocal) setPinState(item._renameTarget, PinState::Unspecified); + return {}; } void VfsSuffix::convertToPlaceholder(const QString &, const SyncFileItem &, const QString &) diff --git a/src/libsync/vfs/suffix/vfs_suffix.h b/src/libsync/vfs/suffix/vfs_suffix.h index 3426c39fe..ce70c2ebb 100644 --- a/src/libsync/vfs/suffix/vfs_suffix.h +++ b/src/libsync/vfs/suffix/vfs_suffix.h @@ -38,10 +38,10 @@ public: bool socketApiPinStateActionsShown() const override { return true; } bool isHydrating() const override; - bool updateMetadata(const QString &filePath, time_t modtime, qint64 size, const QByteArray &fileId, QString *error) override; + Result updateMetadata(const QString &filePath, time_t modtime, qint64 size, const QByteArray &fileId) override; - void createPlaceholder(const SyncFileItem &item) override; - void dehydratePlaceholder(const SyncFileItem &item) override; + Result createPlaceholder(const SyncFileItem &item) override; + Result dehydratePlaceholder(const SyncFileItem &item) override; void convertToPlaceholder(const QString &filename, const SyncFileItem &item, const QString &) override; bool needsMetadataUpdate(const SyncFileItem &) override { return false; } diff --git a/test/testsyncvirtualfiles.cpp b/test/testsyncvirtualfiles.cpp index 647890feb..71af44185 100644 --- a/test/testsyncvirtualfiles.cpp +++ b/test/testsyncvirtualfiles.cpp @@ -603,6 +603,7 @@ private slots: QVERIFY(!fakeFolder.currentLocalState().find("A/a1")); QVERIFY(fakeFolder.currentLocalState().find("A/a1" DVSUFFIX)); + QVERIFY(fakeFolder.currentLocalState().find("A/a1" DVSUFFIX)->size <= 1); QVERIFY(fakeFolder.currentRemoteState().find("A/a1")); QVERIFY(itemInstruction(completeSpy, "A/a1" DVSUFFIX, CSYNC_INSTRUCTION_SYNC)); QCOMPARE(dbRecord(fakeFolder, "A/a1" DVSUFFIX)._type, ItemTypeVirtualFile); @@ -1342,6 +1343,63 @@ private slots: QVERIFY(fakeFolder.currentLocalState().find("online/file1")); QVERIFY(fakeFolder.currentLocalState().find("local/file1" DVSUFFIX)); } + + void testPlaceHolderExist() { + FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() }; + fakeFolder.remoteModifier().insert("A/a1" DVSUFFIX, 111); + fakeFolder.remoteModifier().insert("A/hello" DVSUFFIX, 222); + QVERIFY(fakeFolder.syncOnce()); + auto vfs = setupVfs(fakeFolder); + + ItemCompletedSpy completeSpy(fakeFolder); + auto cleanup = [&]() { completeSpy.clear(); }; + cleanup(); + + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QVERIFY(itemInstruction(completeSpy, "A/a1" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE)); + QVERIFY(itemInstruction(completeSpy, "A/hello" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE)); + + fakeFolder.remoteModifier().insert("A/a2" DVSUFFIX); + fakeFolder.remoteModifier().insert("A/hello", 12); + fakeFolder.localModifier().insert("A/igno" DVSUFFIX, 123); + cleanup(); + QVERIFY(fakeFolder.syncOnce()); + QVERIFY(itemInstruction(completeSpy, "A/a1" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE)); + QVERIFY(itemInstruction(completeSpy, "A/igno" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE)); + + // verify that the files are still present + QCOMPARE(fakeFolder.currentLocalState().find("A/hello" DVSUFFIX)->size, 222); + QCOMPARE(*fakeFolder.currentLocalState().find("A/hello" DVSUFFIX), + *fakeFolder.currentRemoteState().find("A/hello" DVSUFFIX)); + QCOMPARE(fakeFolder.currentLocalState().find("A/igno" DVSUFFIX)->size, 123); + + cleanup(); + // Dehydrate + vfs->setPinState(QString(), PinState::OnlineOnly); + QVERIFY(!fakeFolder.syncOnce()); + + QVERIFY(itemInstruction(completeSpy, "A/igno" DVSUFFIX, CSYNC_INSTRUCTION_IGNORE)); + // verify that the files are still present + QCOMPARE(fakeFolder.currentLocalState().find("A/a1" DVSUFFIX)->size, 111); + QCOMPARE(fakeFolder.currentLocalState().find("A/hello" DVSUFFIX)->size, 222); + QCOMPARE(*fakeFolder.currentLocalState().find("A/hello" DVSUFFIX), + *fakeFolder.currentRemoteState().find("A/hello" DVSUFFIX)); + QCOMPARE(*fakeFolder.currentLocalState().find("A/a1"), + *fakeFolder.currentRemoteState().find("A/a1")); + QCOMPARE(fakeFolder.currentLocalState().find("A/igno" DVSUFFIX)->size, 123); + + // Now disable vfs and check that all files are still there + cleanup(); + SyncEngine::wipeVirtualFiles(fakeFolder.localPath(), fakeFolder.syncJournal(), *vfs); + fakeFolder.switchToVfs(QSharedPointer(new VfsOff)); + QVERIFY(fakeFolder.syncOnce()); + QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); + QCOMPARE(fakeFolder.currentLocalState().find("A/a1" DVSUFFIX)->size, 111); + QCOMPARE(fakeFolder.currentLocalState().find("A/hello")->size, 12); + QCOMPARE(fakeFolder.currentLocalState().find("A/hello" DVSUFFIX)->size, 222); + QCOMPARE(fakeFolder.currentLocalState().find("A/igno" DVSUFFIX)->size, 123); + } }; QTEST_GUILESS_MAIN(TestSyncVirtualFiles)