Merge pull request #4959 from evsh/small-fix-for-settings-storage

refactor SettingsStorage class

src/base/settingsstorage.cpp

@@ -27,6 +27,9 @@
  * exception statement from your version.
  */
 
+#include "settingsstorage.h"
+
+#include <memory>
 #include <QFile>
 #include <QHash>
 #include <QStringList>
@@ -34,19 +37,48 @@
 
 #include "logger.h"
 #include "utils/fs.h"
-#include "settingsstorage.h"
+
+#ifdef Q_OS_MAC
+#define QSETTINGS_SYNC_IS_SAVE // whether QSettings::sync() is "atomic"
+#endif
 
 namespace
 {
-    inline QSettings *createSettings(const QString &name)
+    // Encapsulates serialization of settings in "atomic" way.
+    // write() does not leave half-written files,
+    // read() has a workaround for a case of power loss during a previous serialization
+    class TransactionalSettings
+    {
+    public:
+        TransactionalSettings(const QString &name)
+            : m_name(name)
+        {
+        }
+
+        QVariantHash read();
+        bool write(const QVariantHash &data);
+
+    private:
+        // we return actual file names used by QSettings because
+        // there is no other way to get that name except
+        // actually create a QSettings object.
+        // if serialization operation was not successful we return empty string
+        QString deserialize(const QString &name, QVariantHash &data);
+        QString serialize(const QString &name, const QVariantHash &data);
+
+        using SettingsPtr = std::unique_ptr<QSettings>;
+        SettingsPtr createSettings(const QString &name)
         {
 #ifdef Q_OS_WIN
-        return new QSettings(QSettings::IniFormat, QSettings::UserScope, "qBittorrent", name);
+            return SettingsPtr(new QSettings(QSettings::IniFormat, QSettings::UserScope, "qBittorrent", name));
 #else
-        return new QSettings("qBittorrent", name);
+            return SettingsPtr(new QSettings("qBittorrent", name));
 #endif
         }
 
+        QString m_name;
+    };
+
 #ifdef QBT_USES_QT5
     typedef QHash<QString, QString> MappingTable;
 #else
@@ -92,48 +124,10 @@ namespace
 SettingsStorage *SettingsStorage::m_instance = nullptr;
 
 SettingsStorage::SettingsStorage()
-    : m_dirty(false)
+    : m_data{TransactionalSettings(QLatin1String("qBittorrent")).read()}
+    , m_dirty(false)
     , m_lock(QReadWriteLock::Recursive)
 {
-    QSettings *settings;
-#ifdef Q_OS_MAC
-    settings = createSettings("qBittorrent");
-#else
-    settings = createSettings("qBittorrent_new");
-    QString newPath = settings->fileName();
-
-    // This means that the PC closed either due to power outage
-    // or because the disk was full. In any case the settings weren't transfered
-    // in their final position. So assume that qbittorrent_new.ini/qbittorrent_new.conf
-    // contains the most recent settings.
-    if (!settings->allKeys().isEmpty()) {
-        Logger::instance()->addMessage(tr("Detected unclean program exit. Using fallback file to restore settings."), Log::WARNING);
-        m_dirty = true;
-    }
-    else {
-        delete settings;
-        settings = createSettings("qBittorrent");
-    }
-#endif
-
-    QStringList keys = settings->allKeys();
-
-    // Copy everything into memory. This means even keys inserted in the file manually
-    // or that we don't touch directly in this code(eg disabled by ifdef). This ensures
-    // that they will be copied over when save our settings to disk.
-    foreach (const QString &key, keys)
-        m_data[key] = settings->value(key);
-
-    //Ensures sync to disk before we attempt to manipulate the files from save().
-    delete settings;
-
-#ifndef Q_OS_MAC
-    Utils::Fs::forceRemove(newPath);
-
-    if (m_dirty)
-        save();
-#endif
-
     m_timer.setSingleShot(true);
     m_timer.setInterval(5 * 1000);
     connect(&m_timer, SIGNAL(timeout()), SLOT(save()));
@@ -163,54 +157,17 @@ SettingsStorage *SettingsStorage::instance()
 
 bool SettingsStorage::save()
 {
+    if (!m_dirty) return false; // Obtaining the lock is expensive, let's check early
     QWriteLocker locker(&m_lock);
+    if (!m_dirty) return false; // something might have changed while we were getting the lock
 
-    if (!m_dirty) return false;
+    TransactionalSettings settings(QLatin1String("qBittorrent"));
-
+    if (settings.write(m_data)) {
-#ifndef Q_OS_MAC
-    // QSettings delete the file before writing it out. This can result in problems
-    // if the disk is full or a power outage occurs. Those events might occur
-    // between deleting the file and recreating it. This is a safety measure.
-    // Write everything to qBittorrent_new.ini/qBittorrent_new.conf and if it succeeds
-    // replace qBittorrent_new.ini/qBittorrent.conf with it.
-    QSettings *settings = createSettings("qBittorrent_new");
-#else
-    QSettings *settings = createSettings("qBittorrent");
-#endif
-
-    foreach (const QString &key, m_data.keys())
-        settings->setValue(key, m_data[key]);
-
         m_dirty = false;
-    locker.unlock();
+        return true;
-
-#ifndef Q_OS_MAC
-    settings->sync(); // Important to get error status
-    QSettings::Status status = settings->status();
-    QString newPath = settings->fileName();
-
-    if (status != QSettings::NoError) {
-        if (status == QSettings::AccessError)
-            Logger::instance()->addMessage(tr("An access error occurred while trying to write the configuration file."), Log::CRITICAL);
-        else
-            Logger::instance()->addMessage(tr("A format error occurred while trying to write the configuration file."), Log::CRITICAL);
-
-        delete settings;
-        Utils::Fs::forceRemove(newPath);
-        return false;
     }
 
-    delete settings;
+    return false;
-    QString finalPath = newPath;
-    int index = finalPath.lastIndexOf("_new", -1, Qt::CaseInsensitive);
-    finalPath.remove(index, 4);
-    Utils::Fs::forceRemove(finalPath);
-    QFile::rename(newPath, finalPath);
-#else
-    delete settings;
-#endif
-
-    return true;
 }
 
 QVariant SettingsStorage::loadValue(const QString &key, const QVariant &defaultValue) const
@@ -225,8 +182,8 @@ void SettingsStorage::storeValue(const QString &key, const QVariant &value)
     QWriteLocker locker(&m_lock);
     if (m_data.value(realKey) != value) {
         m_dirty = true;
-        m_timer.start();
         m_data.insert(realKey, value);
+        m_timer.start();
     }
 }
 
@@ -236,7 +193,94 @@ void SettingsStorage::removeValue(const QString &key)
     QWriteLocker locker(&m_lock);
     if (m_data.contains(realKey)) {
         m_dirty = true;
-        m_timer.start();
         m_data.remove(realKey);
+        m_timer.start();
+    }
+}
+
+QVariantHash TransactionalSettings::read()
+{
+    QVariantHash res;
+#ifdef QSETTINGS_SYNC_IS_SAVE
+    deserialize(m_name, res);
+#else
+    bool writeBackNeeded = false;
+    QString newPath = deserialize(m_name + QLatin1String("_new"), res);
+    if (!newPath.isEmpty()) { // "_new" file is NOT empty
+        // This means that the PC closed either due to power outage
+        // or because the disk was full. In any case the settings weren't transfered
+        // in their final position. So assume that qbittorrent_new.ini/qbittorrent_new.conf
+        // contains the most recent settings.
+        Logger::instance()->addMessage(QObject::tr("Detected unclean program exit. Using fallback file to restore settings."), Log::WARNING);
+        writeBackNeeded = true;
+    }
+    else {
+        deserialize(m_name, res);
+    }
+
+    Utils::Fs::forceRemove(newPath);
+
+    if (writeBackNeeded)
+        write(res);
+#endif
+    return res;
+}
+
+bool TransactionalSettings::write(const QVariantHash &data)
+{
+#ifdef QSETTINGS_SYNC_IS_SAVE
+    serialize(m_name, data);
+#else
+    // QSettings delete the file before writing it out. This can result in problems
+    // if the disk is full or a power outage occurs. Those events might occur
+    // between deleting the file and recreating it. This is a safety measure.
+    // Write everything to qBittorrent_new.ini/qBittorrent_new.conf and if it succeeds
+    // replace qBittorrent.ini/qBittorrent.conf with it.
+    QString newPath = serialize(m_name + QLatin1String("_new"), data);
+    if (newPath.isEmpty()) {
+        Utils::Fs::forceRemove(newPath);
+        return false;
+    }
+
+    QString finalPath = newPath;
+    int index = finalPath.lastIndexOf("_new", -1, Qt::CaseInsensitive);
+    finalPath.remove(index, 4);
+    Utils::Fs::forceRemove(finalPath);
+    QFile::rename(newPath, finalPath);
+#endif
+    return true;
+}
+
+QString TransactionalSettings::deserialize(const QString &name, QVariantHash &data)
+{
+    SettingsPtr settings = createSettings(name);
+
+    if (settings->allKeys().isEmpty())
+        return QString();
+
+    // Copy everything into memory. This means even keys inserted in the file manually
+    // or that we don't touch directly in this code (eg disabled by ifdef). This ensures
+    // that they will be copied over when save our settings to disk.
+    foreach (const QString &key, settings->allKeys())
+        data.insert(key, settings->value(key));
+
+    return settings->fileName();
+}
+
+QString TransactionalSettings::serialize(const QString &name, const QVariantHash &data)
+{
+    SettingsPtr settings = createSettings(name);
+    for (auto i = data.begin(); i != data.end(); ++i)
+        settings->setValue(i.key(), i.value());
+
+    settings->sync(); // Important to get error status
+    QSettings::Status status = settings->status();
+    if (status != QSettings::NoError) {
+        if (status == QSettings::AccessError)
+            Logger::instance()->addMessage(QObject::tr("An access error occurred while trying to write the configuration file."), Log::CRITICAL);
+        else
+            Logger::instance()->addMessage(QObject::tr("A format error occurred while trying to write the configuration file."), Log::CRITICAL);
+        return QString();
     }
+    return settings->fileName();
 }