Merge lp:~marcustomlinson/unity-scopes-shell/lp-1552082 into lp:unity-scopes-shell

[Marcus Tomlinson]

Replace QSettings with unity::util::IniParser

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:297
http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-ci/422/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-amd64-ci/116/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-armhf-ci/116/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-i386-ci/116/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-amd64-ci/79/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-armhf-ci/79/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-i386-ci/79/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-shell-ci/422/rebuild

review: Needs Fixing (continuous-integration)

[Paweł Stołowski (stolowski)]

QSettings implements advisory file locking (see http://doc.qt.io/qt-5.5/qsettings.html - Accessing Settings from multiple threads or processes) and we rely on locking in SettingsDB in unity-scopes-api, so I think we're missing this here when not using QSettings. I think this should be considered and perhaps implemented manually?

review: Needs Information

[Marcus Tomlinson (marcustomlinson)]

You're too right, I'm missing thread safety in the unity-api code. Will do.

[lp:~marcustomlinson/unity-scopes-shell/lp-1552082]

298. By Marcus Tomlinson on 2016-03-14

Addressed review comments

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:298
http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-ci/423/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-amd64-ci/117/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-armhf-ci/117/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-i386-ci/117/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-amd64-ci/80/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-armhf-ci/80/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-i386-ci/80/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-shell-ci/423/rebuild

review: Needs Fixing (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

> QSettings implements advisory file locking (see
> http://doc.qt.io/qt-5.5/qsettings.html - Accessing Settings from multiple
> threads or processes) and we rely on locking in SettingsDB in unity-scopes-
> api, so I think we're missing this here when not using QSettings. I think this
> should be considered and perhaps implemented manually?

Alright, this has been fixed in the unity-api MP now.

[Paweł Stołowski (stolowski)]

Thanks! The changes look good, but we need to consider overall impact and edge cases. I don't mean to make your life hard, but this worries me slightly:

37 + catch(const unity::FileException&)
38 + {
39 + // File was not found, so we create an empty one.
40 + auto f = fopen(filePath, "w");
41 + fclose(f);
42 +
43 + m_settings.reset(new unity::util::IniParser(filePath));
44 + }

Before, QSettings would never throw obviously. With the proposed changes to IniParser, an exception from the ctor means it failed on open *or* on obtaining the read lock. Now, when the latter happens, we will try to overwrite the file which may or may not succeed (and potentially overwrite user changes doing so) but we will also try to create IniParser instance, which may throw again and this is not handled (makes shell crash).

I think we should try hard not to loose user changes, failure in obtaining read lock may be a transient issue. This really is a problem, last time I touched settings here and in unity-scopes-api was related to fixing a tricky issue where scope would occasionally go blank when changing the settings (which triggered scope refresh), just because of occasional exception on file access. On a second thought, I think the existing code may be guily of never looking at QSettings::status (but QSettings seems to be doing a good job).

[Michi Henning (michihenning)]

> Before, QSettings would never throw obviously. With the proposed changes to
> IniParser, an exception from the ctor means it failed on open *or* on
> obtaining the read lock. Now, when the latter happens, we will try to
> overwrite the file which may or may not succeed (and potentially overwrite
> user changes doing so) but we will also try to create IniParser instance,
> which may throw again and this is not handled (makes shell crash).

Currently, the ini parser doesn't try to get a lock. Instead, this happens inside SettingsDB in scopes API. I think we need to use the same locking that is done by SettingsDB.cpp in scopes API. The ini parser itself shouldn't do any locking.

[lp:~marcustomlinson/unity-scopes-shell/lp-1552082]

299. By Marcus Tomlinson on 2016-03-17

Added file locking

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:299
http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-ci/428/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-amd64-ci/122/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-armhf-ci/122/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-i386-ci/122/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-amd64-ci/85/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-armhf-ci/85/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-i386-ci/85/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-shell-ci/428/rebuild

review: Needs Fixing (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

> Thanks! The changes look good, but we need to consider overall impact and edge
> cases. I don't mean to make your life hard, but this worries me slightly:
>
> 37 + catch(const unity::FileException&)
> 38 + {
> 39 + // File was not found, so we create an empty one.
> 40 + auto f = fopen(filePath, "w");
> 41 + fclose(f);
> 42 +
> 43 + m_settings.reset(new unity::util::IniParser(filePath));
> 44 + }
>
> Before, QSettings would never throw obviously. With the proposed changes to
> IniParser, an exception from the ctor means it failed on open *or* on
> obtaining the read lock. Now, when the latter happens, we will try to
> overwrite the file which may or may not succeed (and potentially overwrite
> user changes doing so) but we will also try to create IniParser instance,
> which may throw again and this is not handled (makes shell crash).
>
> I think we should try hard not to loose user changes, failure in obtaining
> read lock may be a transient issue. This really is a problem, last time I
> touched settings here and in unity-scopes-api was related to fixing a tricky
> issue where scope would occasionally go blank when changing the settings
> (which triggered scope refresh), just because of occasional exception on file
> access. On a second thought, I think the existing code may be guily of never
> looking at QSettings::status (but QSettings seems to be doing a good job).

Alright, I've removed file locking from unity-api and implemented here to retain behavioural compatibility with QSettings.

[Paweł Stołowski (stolowski)]

Thanks, good stuff! Just one more observation (see the inline comment).
Also, can you make sure settingsChanged() signal is only emited when actual change was succesful (in the old code we had only one case where stuff could go wrong in settings_timout() and it wouldn't emit settingsChanged; now we have new exceptions to deal with but they only result in a qWarning and signal is emited) ?

review: Needs Fixing

[lp:~marcustomlinson/unity-scopes-shell/lp-1552082]

300. By Marcus Tomlinson on 2016-03-17

Address review comments

301. By Marcus Tomlinson on 2016-03-17

A little cleaning up

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:301
http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-ci/429/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-amd64-ci/123/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-armhf-ci/123/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-i386-ci/123/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-amd64-ci/86/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-armhf-ci/86/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-i386-ci/86/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-shell-ci/429/rebuild

review: Needs Fixing (continuous-integration)

[lp:~marcustomlinson/unity-scopes-shell/lp-1552082]

302. By Marcus Tomlinson on 2016-03-29

Bump

303. By Marcus Tomlinson on 2016-03-29

Merge trunk

[Paweł Stołowski (stolowski)]

Sorry for nitpicking, looks really good, just one more inline comment. And a question: is there a reason we attempt to create an empty ini file in the ctor, but if we fail (m_settings is null in #112 and #165) we only try to create ini parser and not the file?

review: Needs Fixing

[lp:~marcustomlinson/unity-scopes-shell/lp-1552082]

304. By Marcus Tomlinson on 2016-03-30

Address review comments

[Marcus Tomlinson (marcustomlinson)]

> Sorry for nitpicking, looks really good, just one more inline comment. And a
> question: is there a reason we attempt to create an empty ini file in the
> ctor, but if we fail (m_settings is null in #112 and #165) we only try to
> create ini parser and not the file?

Damn, yeah, I forgot to carry the file creation logic over to the !m_settings cases.

I've addressed this + the inline comment.

[Paweł Stołowski (stolowski)]

Yep, looks good, thanks!

review: Approve

=== modified file 'debian/control.in'
--- debian/control.in	2016-03-07 08:29:53 +0000
+++ debian/control.in	2016-03-30 12:34:35 +0000
@@ -8,7 +8,7 @@
                dh-python,
                libboost-python-dev,
                libboost-regex-dev,
-               libunity-api-dev (>= 7.108),
+               libunity-api-dev (>= 7.109),
                libunity-scopes-dev (>= 1.0.4~),
                libgsettings-qt-dev (>= 0.1),
                libqtdbustest1-dev (>= 0.2),

=== modified file 'src/Unity/settingsmodel.cpp'
--- src/Unity/settingsmodel.cpp	2015-10-27 09:14:07 +0000
+++ src/Unity/settingsmodel.cpp	2016-03-30 12:34:35 +0000
@@ -21,14 +21,58 @@
 #include "localization.h"
 #include "utils.h"
 
+#include <unity/util/ResourcePtr.h>
+#include <unity/UnityExceptions.h>
+
 #include <QDebug>
 #include <QDir>
 #include <QTextCodec>
 #include <QTimer>
 
+#include <fcntl.h>
+#include <unistd.h>
+
 using namespace scopes_ng;
 namespace sc = unity::scopes;
 
+namespace
+{
+
+typedef unity::util::ResourcePtr<int, std::function<void(int)>> FileLock;
+
+static FileLock unixLock(const QString& path, bool writeLock)
+{
+    FileLock fileLock(::open(path.toUtf8(), writeLock ? O_WRONLY : O_RDONLY), [](int fd)
+    {
+        if (fd != -1)
+        {
+            close(fd);
+        }
+    });
+
+    if (fileLock.get() == -1)
+    {
+        throw unity::FileException("Couldn't open file " + path.toStdString(), errno);
+    }
+
+    struct flock fl;
+    fl.l_whence = SEEK_SET;
+    fl.l_start = 0;
+    fl.l_len = 0;
+    fl.l_type = writeLock ? F_WRLCK : F_RDLCK;
+
+    if (::fcntl(fileLock.get(), F_SETLKW, &fl) != 0)
+    {
+        throw unity::FileException("Couldn't get file lock for " + path.toStdString(), errno);
+    }
+
+    return fileLock;
+}
+
+static const char* GROUP_NAME = "General";
+
+}  // namespace
+
 SettingsModel::SettingsModel(const QDir& configDir, const QString& scopeId,
         const QVariant& settingsDefinitions, QObject* parent,
         int settingsTimeout)
@@ -38,8 +82,17 @@
     configDir.mkpath(scopeId);
     QDir databaseDir = configDir.filePath(scopeId);
 
-    m_settings.reset(new QSettings(databaseDir.filePath(QStringLiteral("settings.ini")), QSettings::IniFormat));
-    m_settings->setIniCodec("UTF-8");
+    m_settings_path = databaseDir.filePath(QStringLiteral("settings.ini"));
+
+    try
+    {
+        tryLoadSettings();
+    }
+    catch(const unity::FileException& e)
+    {
+        // Something has gone wrong, at this point we'll just have to continue with a null m_settings.
+        qWarning() << "SettingsModel::SettingsModel: Failed to read settings file:" << e.what();
+    }
 
     for (const auto &it : settingsDefinitions.toList())
     {
@@ -122,7 +175,37 @@
                 break;
             case Roles::RoleValue:
             {
-                result = m_settings->value(data->id, data->defaultValue);
+                try
+                {
+                    tryLoadSettings();
+                    switch (data->variantType)
+                    {
+                        case QVariant::Bool:
+                            result = m_settings->get_boolean(GROUP_NAME, data->id.toStdString());
+                            break;
+                        case QVariant::UInt:
+                            result = m_settings->get_int(GROUP_NAME, data->id.toStdString());
+                            break;
+                        case QVariant::Double:
+                            result = m_settings->get_double(GROUP_NAME, data->id.toStdString());
+                            break;
+                        case QVariant::String:
+                            result = m_settings->get_string(GROUP_NAME, data->id.toStdString()).c_str();
+                            break;
+                        default:
+                            result = data->defaultValue;
+                    }
+                }
+                catch(const unity::FileException& e)
+                {
+                    qWarning() << "SettingsModel::data: Failed to read settings file:" << e.what();
+                    result = data->defaultValue;
+                }
+                catch(const unity::LogicException&)
+                {
+                    qWarning() << "SettingsModel::data: Failed to get a value for setting:" << data->id;
+                    result = data->defaultValue;
+                }
                 result.convert(data->variantType);
                 break;
             }
@@ -164,8 +247,6 @@
 
 QVariant SettingsModel::value(const QString& id) const
 {
-    m_settings->sync();
-
     // Check for the setting id in the child scopes list first, in case the
     // aggregator is incorrectly using a scope id as a settings as well.
     if (m_child_scopes_data_by_id.contains(id))
@@ -181,7 +262,38 @@
     else if (m_data_by_id.contains(id))
     {
         QSharedPointer<Data> data = m_data_by_id[id];
-        auto result = m_settings->value(data->id, data->defaultValue);
+        QVariant result;
+        try
+        {
+            tryLoadSettings();
+            switch (data->variantType)
+            {
+                case QVariant::Bool:
+                    result = m_settings->get_boolean(GROUP_NAME, data->id.toStdString());
+                    break;
+                case QVariant::UInt:
+                    result = m_settings->get_int(GROUP_NAME, data->id.toStdString());
+                    break;
+                case QVariant::Double:
+                    result = m_settings->get_double(GROUP_NAME, data->id.toStdString());
+                    break;
+                case QVariant::String:
+                    result = m_settings->get_string(GROUP_NAME, data->id.toStdString()).c_str();
+                    break;
+                default:
+                    result = data->defaultValue;
+            }
+        }
+        catch(const unity::FileException& e)
+        {
+            qWarning() << "SettingsModel::value: Failed to read settings file:" << e.what();
+            result = data->defaultValue;
+        }
+        catch(const unity::LogicException&)
+        {
+            qWarning() << "SettingsModel::value: Failed to get a value for setting:" << data->id;
+            result = data->defaultValue;
+        }
         result.convert(data->variantType);
         return result;
     }
@@ -352,13 +464,62 @@
     }
     else if (m_data_by_id.contains(setting_id))
     {
-        m_settings->setValue(setting_id, value);
-        m_settings->sync(); // make sure the change to setting value is synced to fs
+        try
+        {
+            tryLoadSettings();
+            switch (value.type())
+            {
+                case QVariant::Bool:
+                    m_settings->set_boolean(GROUP_NAME, setting_id.toStdString(), value.toBool());
+                    break;
+                case QVariant::Int:
+                case QVariant::UInt:
+                    m_settings->set_int(GROUP_NAME, setting_id.toStdString(), value.toUInt());
+                    break;
+                case QVariant::Double:
+                    m_settings->set_double(GROUP_NAME, setting_id.toStdString(), value.toDouble());
+                    break;
+                case QVariant::String:
+                    m_settings->set_string(GROUP_NAME, setting_id.toStdString(), value.toString().toStdString());
+                    break;
+                default:
+                    qWarning() << "SettingsModel::settings_timeout: Invalid value type for setting:" << setting_id;
+            }
+            FileLock lock = unixLock(m_settings_path, true);
+            m_settings->sync(); // make sure the change to setting value is synced to fs
+
+            Q_EMIT settingsChanged();
+        }
+        catch(const unity::FileException& e)
+        {
+            qWarning() << "SettingsModel::settings_timeout: Failed to write settings file:" << e.what();
+        }
+        catch(const unity::LogicException&)
+        {
+            qWarning() << "SettingsModel::settings_timeout: Failed to set a value for setting:" << setting_id;
+        }
     }
     else
     {
         qWarning() << "No such setting:" << setting_id;
     }
-
-    Q_EMIT settingsChanged();
+}
+
+void SettingsModel::tryLoadSettings() const
+{
+    if (!m_settings)
+    {
+        QFileInfo checkFile(m_settings_path);
+        if (!checkFile.exists() || !checkFile.isFile())
+        {
+            // Config file does not exist, so we create an empty one.
+            if (!QFile(m_settings_path).open(QFile::WriteOnly))
+            {
+                throw unity::FileException("Could not create an empty settings file at: " + m_settings_path.toStdString(), -1);
+            }
+        }
+
+        FileLock lock = unixLock(m_settings_path, false);
+        m_settings.reset(new unity::util::IniParser(m_settings_path.toUtf8()));
+    }
 }

=== modified file 'src/Unity/settingsmodel.h'
--- src/Unity/settingsmodel.h	2015-10-27 09:14:07 +0000
+++ src/Unity/settingsmodel.h	2016-03-30 12:34:35 +0000
@@ -25,11 +25,11 @@
 #include <unity/scopes/Scope.h>
 #include <unity/scopes/ScopeMetadata.h>
 #include <unity/shell/scopes/SettingsModelInterface.h>
+#include <unity/util/IniParser.h>
 
 #include <QAbstractListModel>
 #include <QList>
 #include <QSharedPointer>
-#include <QSettings>
 
 QT_BEGIN_NAMESPACE
 class QDir;
@@ -89,14 +89,18 @@
 protected Q_SLOTS:
     void settings_timeout();
 
+private:
+    void tryLoadSettings() const;
+
 protected:
     QString m_scopeId;
     unity::scopes::ScopeProxy m_scopeProxy;
     int m_settingsTimeout;
 
+    QString m_settings_path;
     QList<QSharedPointer<Data>> m_data;
     QMap<QString, QSharedPointer<Data>> m_data_by_id;
-    QScopedPointer<QSettings> m_settings;
+    mutable QScopedPointer<unity::util::IniParser> m_settings;
     QMap<QString, QSharedPointer<QTimer>> m_timers;
 
     QList<QSharedPointer<Data>> m_child_scopes_data;