From 708b5a69046a04359bc790f2f04204fa376fec18 Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Mon, 23 Apr 2018 01:42:25 +0800 Subject: [PATCH 1/3] Refactor SettingsStorage class Make use of (i.e. returning) QFile::rename operation status Make log message more verbose Add const Remove empty lines Inline typedef --- src/base/settingsstorage.cpp | 58 ++++++++++++++++++------------------ 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/base/settingsstorage.cpp b/src/base/settingsstorage.cpp index a4c6aa8fb..9fcd84dff 100644 --- a/src/base/settingsstorage.cpp +++ b/src/base/settingsstorage.cpp @@ -62,15 +62,12 @@ namespace QString deserialize(const QString &name, QVariantHash &data); QString serialize(const QString &name, const QVariantHash &data); - QString m_name; + const QString m_name; }; - typedef QHash MappingTable; - QString mapKey(const QString &key) { - static const MappingTable keyMapping = { - + static const QHash keyMapping = { {"BitTorrent/Session/MaxRatioAction", "Preferences/Bittorrent/MaxRatioAction"}, {"BitTorrent/Session/DefaultSavePath", "Preferences/Downloads/SavePath"}, {"BitTorrent/Session/TempPath", "Preferences/Downloads/TempPath"}, @@ -147,7 +144,6 @@ namespace {"AddNewTorrentDialog/TopLevel", "Preferences/Downloads/NewAdditionDialogFront"}, {"State/BannedIPs", "Preferences/IPFilter/BannedIPs"} - }; return keyMapping.value(key, key); @@ -211,7 +207,7 @@ QVariant SettingsStorage::loadValue(const QString &key, const QVariant &defaultV void SettingsStorage::storeValue(const QString &key, const QVariant &value) { - QString realKey = mapKey(key); + const QString realKey = mapKey(key); QWriteLocker locker(&m_lock); if (m_data.value(realKey) != value) { m_dirty = true; @@ -222,7 +218,7 @@ void SettingsStorage::storeValue(const QString &key, const QVariant &value) void SettingsStorage::removeValue(const QString &key) { - QString realKey = mapKey(key); + const QString realKey = mapKey(key); QWriteLocker locker(&m_lock); if (m_data.contains(realKey)) { m_dirty = true; @@ -234,25 +230,24 @@ void SettingsStorage::removeValue(const QString &key) QVariantHash TransactionalSettings::read() { QVariantHash res; - bool writeBackNeeded = false; - QString newPath = deserialize(m_name + QLatin1String("_new"), res); + + const 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 transferred // 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; + Logger::instance()->addMessage(QObject::tr("Detected unclean program exit. Using fallback file to restore settings: %1") + .arg(Utils::Fs::toNativePath(newPath)) + , Log::WARNING); + + Utils::Fs::forceRemove(newPath); + write(res); } else { deserialize(m_name, res); } - Utils::Fs::forceRemove(newPath); - - if (writeBackNeeded) - write(res); - return res; } @@ -263,7 +258,7 @@ bool TransactionalSettings::write(const QVariantHash &data) // 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); + const QString newPath = serialize(m_name + QLatin1String("_new"), data); if (newPath.isEmpty()) { Utils::Fs::forceRemove(newPath); return false; @@ -272,10 +267,9 @@ bool TransactionalSettings::write(const QVariantHash &data) QString finalPath = newPath; int index = finalPath.lastIndexOf("_new", -1, Qt::CaseInsensitive); finalPath.remove(index, 4); - Utils::Fs::forceRemove(finalPath); - QFile::rename(newPath, finalPath); - return true; + Utils::Fs::forceRemove(finalPath); + return QFile::rename(newPath, finalPath); } QString TransactionalSettings::deserialize(const QString &name, QVariantHash &data) @@ -301,13 +295,19 @@ QString TransactionalSettings::serialize(const QString &name, const QVariantHash 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(); + + switch (settings->status()) { + case QSettings::NoError: + return settings->fileName(); + case QSettings::AccessError: + Logger::instance()->addMessage(QObject::tr("An access error occurred while trying to write the configuration file."), Log::CRITICAL); + break; + case QSettings::FormatError: + Logger::instance()->addMessage(QObject::tr("A format error occurred while trying to write the configuration file."), Log::CRITICAL); + break; + default: + Logger::instance()->addMessage(QObject::tr("An unknown error occurred while trying to write the configuration file."), Log::CRITICAL); + break; } - return settings->fileName(); + return QString(); } From 099314d17fdff0fd245103e6eb75265e04301577 Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Mon, 23 Apr 2018 01:53:48 +0800 Subject: [PATCH 2/3] Make settings file recovery more robust We should not blindly remove the leftover settings file, as the following write() operation could fail and the user would lost all settings. We should try renaming it instead. --- src/base/settingsstorage.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/base/settingsstorage.cpp b/src/base/settingsstorage.cpp index 9fcd84dff..b20474d3e 100644 --- a/src/base/settingsstorage.cpp +++ b/src/base/settingsstorage.cpp @@ -241,8 +241,12 @@ QVariantHash TransactionalSettings::read() .arg(Utils::Fs::toNativePath(newPath)) , Log::WARNING); - Utils::Fs::forceRemove(newPath); - write(res); + QString finalPath = newPath; + int index = finalPath.lastIndexOf("_new", -1, Qt::CaseInsensitive); + finalPath.remove(index, 4); + + Utils::Fs::forceRemove(finalPath); + QFile::rename(newPath, finalPath); } else { deserialize(m_name, res); From f6d74e39960d9374d11b4b69e118dc4110e5b557 Mon Sep 17 00:00:00 2001 From: Chocobo1 Date: Mon, 23 Apr 2018 02:23:51 +0800 Subject: [PATCH 3/3] Retry saving settings when operation failed --- src/base/settingsstorage.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/base/settingsstorage.cpp b/src/base/settingsstorage.cpp index b20474d3e..99c38c3ac 100644 --- a/src/base/settingsstorage.cpp +++ b/src/base/settingsstorage.cpp @@ -196,6 +196,7 @@ bool SettingsStorage::save() return true; } + m_timer.start(); return false; }