From: Christian Kamm Date: Wed, 21 Nov 2018 11:23:08 +0000 (+0100) Subject: vfs: Ensure SyncOptions::_vfs is never null X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~21^2~468^2~379 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=b30f79edf6583615652a8b6bbe2e89951356192e;p=nextcloud-desktop.git vfs: Ensure SyncOptions::_vfs is never null - Create a VfsOff derived class - Make it a shared pointer shared with Folder::_vfs --- diff --git a/src/common/vfs.cpp b/src/common/vfs.cpp index 961c0c280..d9881e4f9 100644 --- a/src/common/vfs.cpp +++ b/src/common/vfs.cpp @@ -58,6 +58,13 @@ Optional Vfs::modeFromString(const QString &str) return {}; } +VfsOff::VfsOff(QObject *parent) + : Vfs(parent) +{ +} + +VfsOff::~VfsOff() = default; + static QString modeToPluginName(Vfs::Mode mode) { if (mode == Vfs::WithSuffix) @@ -69,6 +76,8 @@ static QString modeToPluginName(Vfs::Mode mode) bool OCC::isVfsPluginAvailable(Vfs::Mode mode) { + if (mode == Vfs::Off) + return true; auto name = modeToPluginName(mode); if (name.isEmpty()) return false; @@ -89,6 +98,9 @@ Q_LOGGING_CATEGORY(lcPlugin, "plugins", QtInfoMsg) std::unique_ptr OCC::createVfsFromPlugin(Vfs::Mode mode) { + if (mode == Vfs::Off) + return std::unique_ptr(new VfsOff); + auto name = modeToPluginName(mode); if (name.isEmpty()) return nullptr; diff --git a/src/common/vfs.h b/src/common/vfs.h index 28fab2085..7803eabc7 100644 --- a/src/common/vfs.h +++ b/src/common/vfs.h @@ -91,7 +91,7 @@ public: virtual Mode mode() const = 0; - /// For WithSuffix modes: what's the suffix (including the dot)? + /// For WithSuffix modes: the suffix (including the dot) virtual QString fileSuffix() const = 0; @@ -158,6 +158,35 @@ signals: void doneHydrating(); }; +/// Implementation of Vfs for Vfs::Off mode - does nothing +class OCSYNC_EXPORT VfsOff : public Vfs +{ + Q_OBJECT + +public: + VfsOff(QObject* parent = nullptr); + virtual ~VfsOff(); + + Mode mode() const override { return Vfs::Off; } + + QString fileSuffix() const override { return QString(); } + + void registerFolder(const VfsSetupParams &) override {} + void start(const VfsSetupParams &) override {} + void stop() override {} + void unregisterFolder() override {} + + + bool isHydrating() const override { return false; } + + bool updateMetadata(const QString &, time_t, quint64, const QByteArray &, QString *) override { return true; } + void createPlaceholder(const QString &, const SyncFileItemPtr &) override {} + void convertToPlaceholder(const QString &, const SyncFileItemPtr &) override {} + + bool isDehydratedPlaceholder(const QString &) override { return false; } + bool statTypeVirtualFile(csync_file_stat_t *, void *) override { return false; } +}; + /// Check whether the plugin for the mode is available. OCSYNC_EXPORT bool isVfsPluginAvailable(Vfs::Mode mode); diff --git a/src/csync/vio/csync_vio_local_unix.cpp b/src/csync/vio/csync_vio_local_unix.cpp index 56de3250c..5b58b01cb 100644 --- a/src/csync/vio/csync_vio_local_unix.cpp +++ b/src/csync/vio/csync_vio_local_unix.cpp @@ -124,7 +124,7 @@ std::unique_ptr csync_vio_local_readdir(csync_vio_handle_t *h // Override type for virtual files if desired if (vfs) { - // Directly modifiest file_stat->type. + // Directly modifies file_stat->type. // We can ignore the return value since we're done here anyway. vfs->statTypeVirtualFile(file_stat.get(), nullptr); } diff --git a/src/gui/folder.cpp b/src/gui/folder.cpp index f58e1e699..22f124448 100644 --- a/src/gui/folder.cpp +++ b/src/gui/folder.cpp @@ -119,10 +119,11 @@ Folder::Folder(const FolderDefinition &definition, _localDiscoveryTracker.data(), &LocalDiscoveryTracker::slotItemCompleted); // Potentially upgrade suffix vfs to windows vfs + ENFORCE(_vfs); if (_definition.virtualFilesMode == Vfs::WithSuffix && _definition.upgradeVfsMode) { if (auto winvfs = createVfsFromPlugin(Vfs::WindowsCfApi)) { // Wipe the existing suffix files from fs and journal - SyncEngine::wipeVirtualFiles(path(), _journal, _vfs.data()); + SyncEngine::wipeVirtualFiles(path(), _journal, *_vfs); // Then switch to winvfs mode _vfs.reset(winvfs.release()); @@ -132,15 +133,13 @@ Folder::Folder(const FolderDefinition &definition, } // Initialize the vfs plugin - if (_definition.virtualFilesMode != Vfs::Off) - startVfs(); + startVfs(); } Folder::~Folder() { // TODO cfapi: unregister on wipe()? There should probably be a wipeForRemoval() where this cleanup is appropriate - if (_vfs) - _vfs->stop(); + _vfs->stop(); // Reset then engine first as it will abort and try to access members of the Folder _engine.reset(); @@ -246,7 +245,7 @@ bool Folder::isBusy() const bool Folder::isSyncRunning() const { - return _engine->isSyncRunning() || (_vfs && _vfs->isHydrating()); + return _engine->isSyncRunning() || _vfs->isHydrating(); } QString Folder::remotePath() const @@ -622,27 +621,28 @@ void Folder::dehydrateFile(const QString &_relativepath) void Folder::setUseVirtualFiles(bool enabled) { + Vfs::Mode newMode = _definition.virtualFilesMode; if (enabled && _definition.virtualFilesMode == Vfs::Off) { - _definition.virtualFilesMode = bestAvailableVfsMode(); - - _vfs.reset(createVfsFromPlugin(_definition.virtualFilesMode).release()); - startVfs(); - - _saveInFoldersWithPlaceholders = true; + newMode = bestAvailableVfsMode(); + } else if (!enabled && _definition.virtualFilesMode != Vfs::Off) { + newMode = Vfs::Off; } - if (!enabled && _definition.virtualFilesMode != Vfs::Off) { - ENFORCE(_vfs); + if (newMode != _definition.virtualFilesMode) { // TODO: Must wait for current sync to finish! - SyncEngine::wipeVirtualFiles(path(), _journal, _vfs.data()); + SyncEngine::wipeVirtualFiles(path(), _journal, *_vfs); _vfs->stop(); _vfs->unregisterFolder(); - _vfs.reset(nullptr); - _definition.virtualFilesMode = Vfs::Off; + _vfs.reset(createVfsFromPlugin(newMode).release()); + startVfs(); + + _definition.virtualFilesMode = newMode; + if (newMode != Vfs::Off) + _saveInFoldersWithPlaceholders = true; + saveToSettings(); } - saveToSettings(); } void Folder::saveToSettings() const @@ -827,7 +827,7 @@ void Folder::setSyncOptions() opt._newBigFolderSizeLimit = newFolderLimit.first ? newFolderLimit.second * 1000LL * 1000LL : -1; // convert from MB to B opt._confirmExternalStorage = cfgFile.confirmExternalStorage(); opt._moveFilesToTrash = cfgFile.moveToTrash(); - opt._vfs = _vfs.data(); + opt._vfs = _vfs; QByteArray chunkSizeEnv = qgetenv("OWNCLOUD_CHUNK_SIZE"); if (!chunkSizeEnv.isEmpty()) { diff --git a/src/gui/folder.h b/src/gui/folder.h index 2666c8f4e..41171e13e 100644 --- a/src/gui/folder.h +++ b/src/gui/folder.h @@ -449,9 +449,9 @@ private: QScopedPointer _localDiscoveryTracker; /** - * The vfs mode instance (created by plugin) to use. Null means no vfs. + * The vfs mode instance (created by plugin) to use. Never null. */ - QScopedPointer _vfs; + QSharedPointer _vfs; }; } diff --git a/src/gui/folderman.cpp b/src/gui/folderman.cpp index f17f9b402..6cdb4af17 100644 --- a/src/gui/folderman.cpp +++ b/src/gui/folderman.cpp @@ -285,7 +285,7 @@ void FolderMan::setupFoldersHelper(QSettings &settings, AccountStatePtr account, } auto vfs = createVfsFromPlugin(folderDefinition.virtualFilesMode); - if (!vfs && folderDefinition.virtualFilesMode != Vfs::Off) { + if (!vfs) { // TODO: Must do better error handling qFatal("Could not load plugin"); } @@ -994,7 +994,7 @@ Folder *FolderMan::addFolder(AccountState *accountState, const FolderDefinition } auto vfs = createVfsFromPlugin(folderDefinition.virtualFilesMode); - if (!vfs && folderDefinition.virtualFilesMode != Vfs::Off) { + if (!vfs) { qCWarning(lcFolderMan) << "Could not load plugin for mode" << folderDefinition.virtualFilesMode; return 0; } diff --git a/src/libsync/discovery.cpp b/src/libsync/discovery.cpp index 1924187eb..ad20e6ab2 100644 --- a/src/libsync/discovery.cpp +++ b/src/libsync/discovery.cpp @@ -399,8 +399,8 @@ void ProcessDirectoryJob::processFileAnalyzeRemoteInfo( return; } // Turn new remote files into virtual files if the option is enabled. - auto vfs = _discoveryData->_syncOptions._vfs; - if (!localEntry.isValid() && vfs && item->_type == ItemTypeFile) { + auto &vfs = _discoveryData->_syncOptions._vfs; + if (!localEntry.isValid() && vfs->mode() != Vfs::Off && item->_type == ItemTypeFile) { item->_type = ItemTypeVirtualFile; if (isVfsWithSuffix()) addVirtualFileSuffix(path._original); @@ -1181,8 +1181,7 @@ void ProcessDirectoryJob::dbError() void ProcessDirectoryJob::addVirtualFileSuffix(QString &str) const { - if (auto vfs = _discoveryData->_syncOptions._vfs) - str.append(vfs->fileSuffix()); + str.append(_discoveryData->_syncOptions._vfs->fileSuffix()); } bool ProcessDirectoryJob::hasVirtualFileSuffix(const QString &str) const @@ -1281,7 +1280,7 @@ bool ProcessDirectoryJob::runLocalQuery() return false; } errno = 0; - while (auto dirent = csync_vio_local_readdir(dh, _discoveryData->_syncOptions._vfs)) { + while (auto dirent = csync_vio_local_readdir(dh, _discoveryData->_syncOptions._vfs.data())) { if (dirent->type == ItemTypeSkip) continue; LocalInfo i; @@ -1320,8 +1319,7 @@ bool ProcessDirectoryJob::runLocalQuery() bool ProcessDirectoryJob::isVfsWithSuffix() const { - auto vfs = _discoveryData->_syncOptions._vfs; - return vfs && vfs->mode() == Vfs::WithSuffix; + return _discoveryData->_syncOptions._vfs->mode() == Vfs::WithSuffix; } } diff --git a/src/libsync/discovery.h b/src/libsync/discovery.h index 1d663a90e..7927077a8 100644 --- a/src/libsync/discovery.h +++ b/src/libsync/discovery.h @@ -62,9 +62,9 @@ public: , _queryServer(queryServer) , _queryLocal(queryLocal) , _discoveryData(data) - { } + void start(); /** Start up to nbJobs, return the number of job started; emit finished() when done */ int processSubJobs(int nbJobs); diff --git a/src/libsync/discoveryphase.cpp b/src/libsync/discoveryphase.cpp index e036f79f3..e97af0e7e 100644 --- a/src/libsync/discoveryphase.cpp +++ b/src/libsync/discoveryphase.cpp @@ -77,7 +77,7 @@ bool DiscoveryPhase::isInSelectiveSyncBlackList(const QString &path) const void DiscoveryPhase::checkSelectiveSyncNewFolder(const QString &path, RemotePermissions remotePerm, std::function callback) { - if (_syncOptions._confirmExternalStorage && !_syncOptions._vfs + if (_syncOptions._confirmExternalStorage && _syncOptions._vfs->mode() == Vfs::Off && remotePerm.hasPermission(RemotePermissions::IsMounted)) { // external storage. @@ -100,7 +100,7 @@ void DiscoveryPhase::checkSelectiveSyncNewFolder(const QString &path, RemotePerm } auto limit = _syncOptions._newBigFolderSizeLimit; - if (limit < 0 || _syncOptions._vfs) { + if (limit < 0 || _syncOptions._vfs->mode() != Vfs::Off) { // no limit, everything is allowed; return callback(false); } diff --git a/src/libsync/propagatedownload.cpp b/src/libsync/propagatedownload.cpp index b4607a63b..6e930848e 100644 --- a/src/libsync/propagatedownload.cpp +++ b/src/libsync/propagatedownload.cpp @@ -415,7 +415,7 @@ void PropagateDownloadFile::startAfterIsEncryptedIsChecked() _stopwatch.start(); auto &syncOptions = propagator()->syncOptions(); - auto vfs = syncOptions._vfs; + auto &vfs = syncOptions._vfs; // For virtual files just create the file and be done if (_item->_type == ItemTypeVirtualFileDehydration) { @@ -426,17 +426,20 @@ void PropagateDownloadFile::startAfterIsEncryptedIsChecked() propagator()->_journal->deleteFileRecord(_item->_file); QFile::remove(fn); - if (vfs && vfs->mode() == Vfs::WithSuffix) { + if (vfs->mode() == Vfs::WithSuffix) { // Normally new suffix-virtual files already have the suffix included in the path // but for dehydrations that isn't the case. Adjust it here. _item->_file.append(vfs->fileSuffix()); } } + if (vfs->mode() == Vfs::Off && _item->_type == ItemTypeVirtualFile) { + qCWarning(lcPropagateDownload) << "ignored virtual file type of" << _item->_file; + _item->_type = ItemTypeFile; + } if (_item->_type == ItemTypeVirtualFile) { auto fn = propagator()->getFilePath(_item->_file); qCDebug(lcPropagateDownload) << "creating virtual file" << fn; - ASSERT(vfs); vfs->createPlaceholder(propagator()->_localDir, _item); updateMetadata(false); return; @@ -987,9 +990,7 @@ void PropagateDownloadFile::downloadFinished() } // Make the file a hydrated placeholder if possible - if (auto vfs = propagator()->syncOptions()._vfs) { - vfs->convertToPlaceholder(fn, _item); - } + propagator()->syncOptions()._vfs->convertToPlaceholder(fn, _item); FileSystem::setFileHidden(fn, false); diff --git a/src/libsync/propagateremotemove.cpp b/src/libsync/propagateremotemove.cpp index e9c4edc40..65ad5d527 100644 --- a/src/libsync/propagateremotemove.cpp +++ b/src/libsync/propagateremotemove.cpp @@ -90,8 +90,8 @@ void PropagateRemoteMove::start() QString source = propagator()->_remoteFolder + _item->_file; QString destination = QDir::cleanPath(propagator()->account()->davUrl().path() + propagator()->_remoteFolder + _item->_renameTarget); - auto vfs = propagator()->syncOptions()._vfs; - if (vfs && vfs->mode() == Vfs::WithSuffix + auto &vfs = propagator()->syncOptions()._vfs; + if (vfs->mode() == Vfs::WithSuffix && (_item->_type == ItemTypeVirtualFile || _item->_type == ItemTypeVirtualFileDownload)) { const auto suffix = vfs->fileSuffix(); ASSERT(source.endsWith(suffix) && destination.endsWith(suffix)); diff --git a/src/libsync/syncengine.cpp b/src/libsync/syncengine.cpp index e2fa5adff..1a96ed657 100644 --- a/src/libsync/syncengine.cpp +++ b/src/libsync/syncengine.cpp @@ -483,12 +483,10 @@ void SyncEngine::startSync() _lastLocalDiscoveryStyle = _localDiscoveryStyle; - if (_syncOptions._vfs && _syncOptions._vfs->mode() == Vfs::WithSuffix) { - if (_syncOptions._vfs->fileSuffix().isEmpty()) { - syncError(tr("Using virtual files with suffix, but suffix is not set")); - finalize(false); - return; - } + if (_syncOptions._vfs->mode() == Vfs::WithSuffix && _syncOptions._vfs->fileSuffix().isEmpty()) { + syncError(tr("Using virtual files with suffix, but suffix is not set")); + finalize(false); + return; } // If needed, make sure we have up to date E2E information before the @@ -1001,7 +999,7 @@ bool SyncEngine::shouldDiscoverLocally(const QString &path) const return false; } -void SyncEngine::wipeVirtualFiles(const QString &localPath, SyncJournalDb &journal, Vfs *vfs) +void SyncEngine::wipeVirtualFiles(const QString &localPath, SyncJournalDb &journal, Vfs &vfs) { qCInfo(lcEngine) << "Wiping virtual files inside" << localPath; journal.getFilesBelowPath(QByteArray(), [&](const SyncJournalFileRecord &rec) { @@ -1014,7 +1012,7 @@ void SyncEngine::wipeVirtualFiles(const QString &localPath, SyncJournalDb &journ // If the local file is a dehydrated placeholder, wipe it too. // Otherwise leave it to allow the next sync to have a new-new conflict. QString localFile = localPath + rec._path; - if (QFile::exists(localFile) && vfs && vfs->isDehydratedPlaceholder(localFile)) { + if (QFile::exists(localFile) && vfs.isDehydratedPlaceholder(localFile)) { qCDebug(lcEngine) << "Removing local dehydrated placeholder" << rec._path; QFile::remove(localFile); } diff --git a/src/libsync/syncengine.h b/src/libsync/syncengine.h index 5fd961a37..49927772f 100644 --- a/src/libsync/syncengine.h +++ b/src/libsync/syncengine.h @@ -124,7 +124,7 @@ public: * Particularly useful when switching off vfs mode or switching to a * different kind of vfs. */ - static void wipeVirtualFiles(const QString &localPath, SyncJournalDb &journal, Vfs *vfs); + static void wipeVirtualFiles(const QString &localPath, SyncJournalDb &journal, Vfs &vfs); auto getPropagator() { return _propagator; } // for the test diff --git a/src/libsync/syncoptions.h b/src/libsync/syncoptions.h index 7e09526c5..8de39a789 100644 --- a/src/libsync/syncoptions.h +++ b/src/libsync/syncoptions.h @@ -16,6 +16,7 @@ #include "owncloudlib.h" #include +#include #include #include "common/vfs.h" @@ -26,6 +27,10 @@ namespace OCC { */ struct SyncOptions { + SyncOptions() + : _vfs(new VfsOff) + {} + /** Maximum size (in Bytes) a folder can have without asking for confirmation. * -1 means infinite */ qint64 _newBigFolderSizeLimit = -1; @@ -36,8 +41,8 @@ struct SyncOptions /** If remotely deleted files are needed to move to trash */ bool _moveFilesToTrash = false; - /** Create a virtual file for new files instead of downloading */ - Vfs *_vfs = nullptr; + /** Create a virtual file for new files instead of downloading. May not be null */ + QSharedPointer _vfs; /** The initial un-adjusted chunk size in bytes for chunked uploads, both * for old and new chunking algorithm, which classifies the item to be chunked diff --git a/test/testsyncvirtualfiles.cpp b/test/testsyncvirtualfiles.cpp index d647d58a8..040caac08 100644 --- a/test/testsyncvirtualfiles.cpp +++ b/test/testsyncvirtualfiles.cpp @@ -62,8 +62,7 @@ void markForDehydration(FakeFolder &folder, const QByteArray &path) SyncOptions vfsSyncOptions() { SyncOptions options; - // leak here - options._vfs = createVfsFromPlugin(Vfs::WithSuffix).release(); + options._vfs.reset(createVfsFromPlugin(Vfs::WithSuffix).release()); return options; } @@ -435,7 +434,7 @@ private slots: QVERIFY(fakeFolder.currentLocalState().find("A/a1.nextcloud")); // Switch off new files becoming virtual files - syncOptions._vfs = nullptr; + syncOptions._vfs.reset(new VfsOff); fakeFolder.syncEngine().setSyncOptions(syncOptions); // A sync that doesn't do remote discovery will wipe the placeholder, but not redownload @@ -772,7 +771,7 @@ private slots: // Now wipe the virtuals - SyncEngine::wipeVirtualFiles(fakeFolder.localPath(), fakeFolder.syncJournal(), fakeFolder.syncEngine().syncOptions()._vfs); + SyncEngine::wipeVirtualFiles(fakeFolder.localPath(), fakeFolder.syncJournal(), *fakeFolder.syncEngine().syncOptions()._vfs); QVERIFY(!fakeFolder.currentLocalState().find("f1.nextcloud")); QVERIFY(!fakeFolder.currentLocalState().find("A/a1.nextcloud"));