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 961c0c280984c5815659e0b96a83954ef2bf68be..d9881e4f9b0862d8534381eb6b9837bfb12b1162 100644 (file)
--- a/src/common/vfs.cpp
+++ b/src/common/vfs.cpp
@@ -58,6 +58,13 @@ Optional<Vfs::Mode> 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<Vfs> OCC::createVfsFromPlugin(Vfs::Mode mode)
 {
+    if (mode == Vfs::Off)
+        return std::unique_ptr<Vfs>(new VfsOff);
+
     auto name = modeToPluginName(mode);
     if (name.isEmpty())
         return nullptr;

diff --git a/src/common/vfs.h b/src/common/vfs.h
index 28fab2085a17d343ddb8c25e2b1dae68653573f3..7803eabc7039af33c96d8b16ab8de5b07385faeb 100644 (file)
--- 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 56de3250cea4f2c6d4573e675eb85b2a0274b154..5b58b01cb1c9c2ee3e3ad024274b80b3193dac35 100644 (file)
--- 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_file_stat_t> 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 f58e1e69969f77793b08503db1acd2cce3bb7354..22f124448f3243e49c0d723f567660dd6af24dde 100644 (file)
--- 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 2666c8f4e259fae4e7c1f9e51267e56813814982..41171e13e0c5495efa6655ff0ae9fc8c3aaea6be 100644 (file)
--- a/src/gui/folder.h
+++ b/src/gui/folder.h
@@ -449,9 +449,9 @@ private:
     QScopedPointer<LocalDiscoveryTracker> _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> _vfs;
+    QSharedPointer<Vfs> _vfs;
 };
 }
 

diff --git a/src/gui/folderman.cpp b/src/gui/folderman.cpp
index f17f9b40245ad42852dc04c40f2d9a0626310547..6cdb4af17005f8b19e708b293c5b67d7d9c1df0b 100644 (file)
--- 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 1924187eb86a0c83720044c27dc29c55754a0e4f..ad20e6ab2aaf65ddabb0249406193852c5b524d3 100644 (file)
--- 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 1d663a90e19f132e317e3231947b08e8b50a98a7..7927077a86831b7cf863d122580df6086eb2ab89 100644 (file)
--- 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 e036f79f3fefc60193a3adb08933a614263a2bf4..e97af0e7e079862aa2a83a8d2bd95232297c6c4c 100644 (file)
--- 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<void(bool)> 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 b4607a63b3205a26c775d36a64fa143e0c6a0780..6e930848ea72e160f1ed02be154ed21462ee4568 100644 (file)
--- 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 e9c4edc409d47d6aebf48080fbc1e345245cf7d6..65ad5d52764aec08abb75e2d3db9623741871f79 100644 (file)
--- 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 e2fa5adff5740a3f1031c179db6bc2fa86637858..1a96ed6578d42bb5dee753e1a006f8fa7b73312d 100644 (file)
--- 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 5fd961a371fa8a8ad29fe1094cc242a48dd41b69..49927772fa79ae10ed05c37d2b4465af7bf7cc1f 100644 (file)
--- 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 7e09526c5186dc6f5c92a1e7710456eca859f3ce..8de39a7892a2c8a0093bcdd20ff79022d091aad4 100644 (file)
--- a/src/libsync/syncoptions.h
+++ b/src/libsync/syncoptions.h
@@ -16,6 +16,7 @@
 
 #include "owncloudlib.h"
 #include <QString>
+#include <QSharedPointer>
 #include <chrono>
 #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> _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 d647d58a8b71df81eb18548485c86e188cc801bd..040caac0846ff8d0f50f0f2e6e29ae708ef0a6f3 100644 (file)
--- 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"));