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

Proposed by Marcus Tomlinson on 2016-03-14
Status: Merged
Approved by: Paweł Stołowski on 2016-03-30
Approved revision: 304
Merged at revision: 301
Proposed branch: lp:~marcustomlinson/unity-scopes-shell/lp-1552082
Merge into: lp:unity-scopes-shell
Diff against target: 287 lines (+178/-13)
3 files modified
debian/control.in (+1/-1)
src/Unity/settingsmodel.cpp (+171/-10)
src/Unity/settingsmodel.h (+6/-2)
To merge this branch: bzr merge lp:~marcustomlinson/unity-scopes-shell/lp-1552082
Reviewer Review Type Date Requested Status
Paweł Stołowski 2016-03-14 Approve on 2016-03-30
PS Jenkins bot (community) continuous-integration Needs Fixing on 2016-03-17
Review via email: mp+288918@code.launchpad.net

Commit message

Replace QSettings with unity::util::IniParser

To post a comment you must log in.
Paweł Stołowski (stolowski) wrote :

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) wrote :

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

298. By Marcus Tomlinson on 2016-03-14

Addressed review comments

Marcus Tomlinson (marcustomlinson) wrote :

> 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) wrote :

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) wrote :

> 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.

299. By Marcus Tomlinson on 2016-03-17

Added file locking

Marcus Tomlinson (marcustomlinson) wrote :

> 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) wrote :

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
300. By Marcus Tomlinson on 2016-03-17

Address review comments

301. By Marcus Tomlinson on 2016-03-17

A little cleaning up

302. By Marcus Tomlinson on 2016-03-29

Bump

303. By Marcus Tomlinson on 2016-03-29

Merge trunk

Paweł Stołowski (stolowski) wrote :

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
304. By Marcus Tomlinson on 2016-03-30

Address review comments

Marcus Tomlinson (marcustomlinson) wrote :

> 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) wrote :

Yep, looks good, thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/control.in'
2--- debian/control.in 2016-03-07 08:29:53 +0000
3+++ debian/control.in 2016-03-30 12:34:35 +0000
4@@ -8,7 +8,7 @@
5 dh-python,
6 libboost-python-dev,
7 libboost-regex-dev,
8- libunity-api-dev (>= 7.108),
9+ libunity-api-dev (>= 7.109),
10 libunity-scopes-dev (>= 1.0.4~),
11 libgsettings-qt-dev (>= 0.1),
12 libqtdbustest1-dev (>= 0.2),
13
14=== modified file 'src/Unity/settingsmodel.cpp'
15--- src/Unity/settingsmodel.cpp 2015-10-27 09:14:07 +0000
16+++ src/Unity/settingsmodel.cpp 2016-03-30 12:34:35 +0000
17@@ -21,14 +21,58 @@
18 #include "localization.h"
19 #include "utils.h"
20
21+#include <unity/util/ResourcePtr.h>
22+#include <unity/UnityExceptions.h>
23+
24 #include <QDebug>
25 #include <QDir>
26 #include <QTextCodec>
27 #include <QTimer>
28
29+#include <fcntl.h>
30+#include <unistd.h>
31+
32 using namespace scopes_ng;
33 namespace sc = unity::scopes;
34
35+namespace
36+{
37+
38+typedef unity::util::ResourcePtr<int, std::function<void(int)>> FileLock;
39+
40+static FileLock unixLock(const QString& path, bool writeLock)
41+{
42+ FileLock fileLock(::open(path.toUtf8(), writeLock ? O_WRONLY : O_RDONLY), [](int fd)
43+ {
44+ if (fd != -1)
45+ {
46+ close(fd);
47+ }
48+ });
49+
50+ if (fileLock.get() == -1)
51+ {
52+ throw unity::FileException("Couldn't open file " + path.toStdString(), errno);
53+ }
54+
55+ struct flock fl;
56+ fl.l_whence = SEEK_SET;
57+ fl.l_start = 0;
58+ fl.l_len = 0;
59+ fl.l_type = writeLock ? F_WRLCK : F_RDLCK;
60+
61+ if (::fcntl(fileLock.get(), F_SETLKW, &fl) != 0)
62+ {
63+ throw unity::FileException("Couldn't get file lock for " + path.toStdString(), errno);
64+ }
65+
66+ return fileLock;
67+}
68+
69+static const char* GROUP_NAME = "General";
70+
71+} // namespace
72+
73 SettingsModel::SettingsModel(const QDir& configDir, const QString& scopeId,
74 const QVariant& settingsDefinitions, QObject* parent,
75 int settingsTimeout)
76@@ -38,8 +82,17 @@
77 configDir.mkpath(scopeId);
78 QDir databaseDir = configDir.filePath(scopeId);
79
80- m_settings.reset(new QSettings(databaseDir.filePath(QStringLiteral("settings.ini")), QSettings::IniFormat));
81- m_settings->setIniCodec("UTF-8");
82+ m_settings_path = databaseDir.filePath(QStringLiteral("settings.ini"));
83+
84+ try
85+ {
86+ tryLoadSettings();
87+ }
88+ catch(const unity::FileException& e)
89+ {
90+ // Something has gone wrong, at this point we'll just have to continue with a null m_settings.
91+ qWarning() << "SettingsModel::SettingsModel: Failed to read settings file:" << e.what();
92+ }
93
94 for (const auto &it : settingsDefinitions.toList())
95 {
96@@ -122,7 +175,37 @@
97 break;
98 case Roles::RoleValue:
99 {
100- result = m_settings->value(data->id, data->defaultValue);
101+ try
102+ {
103+ tryLoadSettings();
104+ switch (data->variantType)
105+ {
106+ case QVariant::Bool:
107+ result = m_settings->get_boolean(GROUP_NAME, data->id.toStdString());
108+ break;
109+ case QVariant::UInt:
110+ result = m_settings->get_int(GROUP_NAME, data->id.toStdString());
111+ break;
112+ case QVariant::Double:
113+ result = m_settings->get_double(GROUP_NAME, data->id.toStdString());
114+ break;
115+ case QVariant::String:
116+ result = m_settings->get_string(GROUP_NAME, data->id.toStdString()).c_str();
117+ break;
118+ default:
119+ result = data->defaultValue;
120+ }
121+ }
122+ catch(const unity::FileException& e)
123+ {
124+ qWarning() << "SettingsModel::data: Failed to read settings file:" << e.what();
125+ result = data->defaultValue;
126+ }
127+ catch(const unity::LogicException&)
128+ {
129+ qWarning() << "SettingsModel::data: Failed to get a value for setting:" << data->id;
130+ result = data->defaultValue;
131+ }
132 result.convert(data->variantType);
133 break;
134 }
135@@ -164,8 +247,6 @@
136
137 QVariant SettingsModel::value(const QString& id) const
138 {
139- m_settings->sync();
140-
141 // Check for the setting id in the child scopes list first, in case the
142 // aggregator is incorrectly using a scope id as a settings as well.
143 if (m_child_scopes_data_by_id.contains(id))
144@@ -181,7 +262,38 @@
145 else if (m_data_by_id.contains(id))
146 {
147 QSharedPointer<Data> data = m_data_by_id[id];
148- auto result = m_settings->value(data->id, data->defaultValue);
149+ QVariant result;
150+ try
151+ {
152+ tryLoadSettings();
153+ switch (data->variantType)
154+ {
155+ case QVariant::Bool:
156+ result = m_settings->get_boolean(GROUP_NAME, data->id.toStdString());
157+ break;
158+ case QVariant::UInt:
159+ result = m_settings->get_int(GROUP_NAME, data->id.toStdString());
160+ break;
161+ case QVariant::Double:
162+ result = m_settings->get_double(GROUP_NAME, data->id.toStdString());
163+ break;
164+ case QVariant::String:
165+ result = m_settings->get_string(GROUP_NAME, data->id.toStdString()).c_str();
166+ break;
167+ default:
168+ result = data->defaultValue;
169+ }
170+ }
171+ catch(const unity::FileException& e)
172+ {
173+ qWarning() << "SettingsModel::value: Failed to read settings file:" << e.what();
174+ result = data->defaultValue;
175+ }
176+ catch(const unity::LogicException&)
177+ {
178+ qWarning() << "SettingsModel::value: Failed to get a value for setting:" << data->id;
179+ result = data->defaultValue;
180+ }
181 result.convert(data->variantType);
182 return result;
183 }
184@@ -352,13 +464,62 @@
185 }
186 else if (m_data_by_id.contains(setting_id))
187 {
188- m_settings->setValue(setting_id, value);
189- m_settings->sync(); // make sure the change to setting value is synced to fs
190+ try
191+ {
192+ tryLoadSettings();
193+ switch (value.type())
194+ {
195+ case QVariant::Bool:
196+ m_settings->set_boolean(GROUP_NAME, setting_id.toStdString(), value.toBool());
197+ break;
198+ case QVariant::Int:
199+ case QVariant::UInt:
200+ m_settings->set_int(GROUP_NAME, setting_id.toStdString(), value.toUInt());
201+ break;
202+ case QVariant::Double:
203+ m_settings->set_double(GROUP_NAME, setting_id.toStdString(), value.toDouble());
204+ break;
205+ case QVariant::String:
206+ m_settings->set_string(GROUP_NAME, setting_id.toStdString(), value.toString().toStdString());
207+ break;
208+ default:
209+ qWarning() << "SettingsModel::settings_timeout: Invalid value type for setting:" << setting_id;
210+ }
211+ FileLock lock = unixLock(m_settings_path, true);
212+ m_settings->sync(); // make sure the change to setting value is synced to fs
213+
214+ Q_EMIT settingsChanged();
215+ }
216+ catch(const unity::FileException& e)
217+ {
218+ qWarning() << "SettingsModel::settings_timeout: Failed to write settings file:" << e.what();
219+ }
220+ catch(const unity::LogicException&)
221+ {
222+ qWarning() << "SettingsModel::settings_timeout: Failed to set a value for setting:" << setting_id;
223+ }
224 }
225 else
226 {
227 qWarning() << "No such setting:" << setting_id;
228 }
229-
230- Q_EMIT settingsChanged();
231+}
232+
233+void SettingsModel::tryLoadSettings() const
234+{
235+ if (!m_settings)
236+ {
237+ QFileInfo checkFile(m_settings_path);
238+ if (!checkFile.exists() || !checkFile.isFile())
239+ {
240+ // Config file does not exist, so we create an empty one.
241+ if (!QFile(m_settings_path).open(QFile::WriteOnly))
242+ {
243+ throw unity::FileException("Could not create an empty settings file at: " + m_settings_path.toStdString(), -1);
244+ }
245+ }
246+
247+ FileLock lock = unixLock(m_settings_path, false);
248+ m_settings.reset(new unity::util::IniParser(m_settings_path.toUtf8()));
249+ }
250 }
251
252=== modified file 'src/Unity/settingsmodel.h'
253--- src/Unity/settingsmodel.h 2015-10-27 09:14:07 +0000
254+++ src/Unity/settingsmodel.h 2016-03-30 12:34:35 +0000
255@@ -25,11 +25,11 @@
256 #include <unity/scopes/Scope.h>
257 #include <unity/scopes/ScopeMetadata.h>
258 #include <unity/shell/scopes/SettingsModelInterface.h>
259+#include <unity/util/IniParser.h>
260
261 #include <QAbstractListModel>
262 #include <QList>
263 #include <QSharedPointer>
264-#include <QSettings>
265
266 QT_BEGIN_NAMESPACE
267 class QDir;
268@@ -89,14 +89,18 @@
269 protected Q_SLOTS:
270 void settings_timeout();
271
272+private:
273+ void tryLoadSettings() const;
274+
275 protected:
276 QString m_scopeId;
277 unity::scopes::ScopeProxy m_scopeProxy;
278 int m_settingsTimeout;
279
280+ QString m_settings_path;
281 QList<QSharedPointer<Data>> m_data;
282 QMap<QString, QSharedPointer<Data>> m_data_by_id;
283- QScopedPointer<QSettings> m_settings;
284+ mutable QScopedPointer<unity::util::IniParser> m_settings;
285 QMap<QString, QSharedPointer<QTimer>> m_timers;
286
287 QList<QSharedPointer<Data>> m_child_scopes_data;

Subscribers

People subscribed via source and target branches

to all changes: