From cc09e7e8340385bffaf8d93c303cdd9082c1d5ee Mon Sep 17 00:00:00 2001 From: Eugene Shalygin Date: Tue, 15 Mar 2016 06:09:17 +0100 Subject: [PATCH] refactor SettingsStorage class 1. Extract "transaction" support for QSettings into separate class TransactionalSettings. 2. Define macrto with explicit name for the case when this "transaction" support is needed. 3. A bit optimize QHash <-> QSettings copying: replace assign with insert() and remove repetitive key lookups. 4. In save() check dirty status before getting the lock too. The changes from items 1 and 2 make text more structured and the logic of the SettingsStorage class gets separated from the implementation level task of guarding the settings serialization. The changes in 3 and 4 do not make the app much faster, but neither make any harm to the code readability. --- src/base/settingsstorage.cpp | 226 +++++++++++++++++++++-------------- 1 file changed, 135 insertions(+), 91 deletions(-) diff --git a/src/base/settingsstorage.cpp b/src/base/settingsstorage.cpp index cbd34452f..9df5f0799 100644 --- a/src/base/settingsstorage.cpp +++ b/src/base/settingsstorage.cpp @@ -27,6 +27,9 @@ * exception statement from your version. */ +#include "settingsstorage.h" + +#include #include #include #include @@ -34,18 +37,47 @@ #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; + 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 MappingTable; @@ -93,48 +125,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())); @@ -164,54 +158,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; - -#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(); - -#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; + TransactionalSettings settings(QLatin1String("qBittorrent")); + if (settings.write(m_data)) { + m_dirty = false; + return true; } - delete settings; - 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; + return false; } QVariant SettingsStorage::loadValue(const QString &key, const QVariant &defaultValue) const @@ -226,8 +183,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(); } } @@ -237,7 +194,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(); +}