Merge lp:~marcustomlinson/unity8/scope-tool-unix-signals into lp:unity8

Proposed by Marcus Tomlinson on 2015-07-01
Status: Merged
Approved by: Albert Astals Cid on 2015-07-03
Approved revision: 1833
Merged at revision: 1866
Proposed branch: lp:~marcustomlinson/unity8/scope-tool-unix-signals
Merge into: lp:unity8
Diff against target: 240 lines (+202/-0)
4 files modified
tools/CMakeLists.txt (+1/-0)
tools/scopetool.cpp (+6/-0)
tools/unix-signal-handler.cpp (+141/-0)
tools/unix-signal-handler.h (+54/-0)
To merge this branch: bzr merge lp:~marcustomlinson/unity8/scope-tool-unix-signals
Reviewer Review Type Date Requested Status
Albert Astals Cid (community) 2015-07-01 Approve on 2015-07-03
Michi Henning (community) Approve on 2015-07-03
PS Jenkins bot (community) continuous-integration Needs Fixing on 2015-07-01
Review via email: mp+263497@code.launchpad.net

Commit message

Handle TERM, HUP, and INT signals in unity-scope-tool

Description of the change

 * Are there any related MPs required for this MP to build/function as expected? Please list.
No

 * Did you perform an exploratory manual test run of your code change and any related functionality?
Yes

 * Did you make sure that your branch does not contain spurious tags?
Yes

 * If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
N/A

 * If you changed the UI, has there been a design review?
N/A

To post a comment you must log in.
Albert Astals Cid (aacid) wrote :

Please read https://wiki.ubuntu.com/Process/Merges/Checklists/Unity8 and add the checklist as description

Albert Astals Cid (aacid) wrote :

Of this affects unity-scope-tool does this affect unity8 too?

review: Needs Information
Marcus Tomlinson (marcustomlinson) wrote :

> Of this affects unity-scope-tool does this affect unity8 too?

No, only unity-scope-tool, as that process creates its own isolated registry process. Unity8 talks to the long-running system registry instance.

Albert Astals Cid (aacid) wrote :

Any reason we have those sockects and we just don't exit from hupSignalHandler & friends?

review: Needs Information
Marcus Tomlinson (marcustomlinson) wrote :

> Any reason we have those sockects and we just don't exit from hupSignalHandler
> & friends?

hupSignalHandler & friends are static so we can't access m_func from them.

Marcus Tomlinson (marcustomlinson) wrote :

> > Any reason we have those sockects and we just don't exit from
> hupSignalHandler
> > & friends?
>
> hupSignalHandler & friends are static so we can't access m_func from them.

If it soothes your soul a little, this class was implemented according to the following Qt doc: http://doc.qt.io/qt-5/unix-signals.html

Michi Henning (michihenning) wrote :

The old adage still applies: pretty much the only thing UNIX signals are good for is shooting a process dead. Try to do anything else whatsoever with them, and things very rapidly get very complicated and buggy :-(

You might be able to use prctl(PR_SET_PDEATHSIG, SIGTERM) do propagate parent death.
Making the registry a process group leader with setpgid() would also be helpful, I suspect.

Marcus Tomlinson (marcustomlinson) wrote :

> FAILED: Continuous integration, rev:1833
> http://jenkins.qa.ubuntu.com/job/unity8-ci/5918/
> Executed test runs:
> FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-
> touch/239/console
> SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/198
> SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/199
> FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-
> wily-mako/161/console
> SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-
> wily-armhf/239
> deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-
> wily-armhf/239/artifact/work/output/*zip*/output.zip
> SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21612
>
> Click here to trigger a rebuild:
> http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5918/rebuild

Autopilot tests are failing with: "EnvironmentError: Unsupported device, autodetect fails device". Known issue? Surely not introduced by this change.

Albert Astals Cid (aacid) wrote :

Yeah, autopilot doesn't use scopetool at all.

So do you want to leave this at is is or are considering Michi's suggestions?

Marcus Tomlinson (marcustomlinson) wrote :

> Yeah, autopilot doesn't use scopetool at all.
>
> So do you want to leave this at is is or are considering Michi's suggestions?

Well, all I am doing is terminating upon receiving a signal, so nothing complex. + I have tested it and all works as expected. Don't think another approach is really necessary. So yeah, lets go with the change as it is.

Michi Henning (michihenning) wrote :

Fine by me. Signals suck...

review: Approve
Albert Astals Cid (aacid) wrote :

 * Did you perform an exploratory manual test run of the code change and any related functionality?
Yes

 * Did CI run pass?
No, but this doesn't even go through CI

 * Did you make sure that the branch does not contain spurious tags?
Yes

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tools/CMakeLists.txt'
2--- tools/CMakeLists.txt 2014-03-21 14:31:17 +0000
3+++ tools/CMakeLists.txt 2015-07-01 10:55:15 +0000
4@@ -1,6 +1,7 @@
5 add_executable(${SCOPE_TOOL}
6 scopetool.cpp
7 registry-tracker.cpp
8+ unix-signal-handler.cpp
9 )
10
11 qt5_use_modules(${SCOPE_TOOL} Qml Quick)
12
13=== modified file 'tools/scopetool.cpp'
14--- tools/scopetool.cpp 2014-09-04 11:36:55 +0000
15+++ tools/scopetool.cpp 2015-07-01 10:55:15 +0000
16@@ -32,6 +32,7 @@
17 // local
18 #include <paths.h>
19 #include "registry-tracker.h"
20+#include "unix-signal-handler.h"
21
22
23 int main(int argc, char *argv[])
24@@ -96,6 +97,11 @@
25
26 view->show();
27
28+ UnixSignalHandler signal_handler([]{
29+ QGuiApplication::exit(0);
30+ });
31+ signal_handler.setupUnixSignalHandlers();
32+
33 int result = application->exec();
34
35 delete view;
36
37=== added file 'tools/unix-signal-handler.cpp'
38--- tools/unix-signal-handler.cpp 1970-01-01 00:00:00 +0000
39+++ tools/unix-signal-handler.cpp 2015-07-01 10:55:15 +0000
40@@ -0,0 +1,141 @@
41+/*
42+ * Copyright (C) 2015 Canonical, Ltd.
43+ *
44+ * This program is free software: you can redistribute it and/or modify it
45+ * under the terms of the GNU General Public License version 3, as published
46+ * by the Free Software Foundation.
47+ *
48+ * This program is distributed in the hope that it will be useful, but
49+ * WITHOUT ANY WARRANTY; without even the implied warranties of
50+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
51+ * PURPOSE. See the GNU General Public License for more details.
52+ *
53+ * You should have received a copy of the GNU General Public License along
54+ * with this program. If not, see <http://www.gnu.org/licenses/>.
55+ *
56+ * Author: Marcus Tomlinson <marcus.tomlinson@canonical.com>
57+ */
58+
59+#include "unix-signal-handler.h"
60+
61+#include <QDebug>
62+
63+#include <csignal>
64+#include <sys/socket.h>
65+#include <unistd.h>
66+
67+int UnixSignalHandler::sighupFd[2];
68+int UnixSignalHandler::sigintFd[2];
69+int UnixSignalHandler::sigtermFd[2];
70+
71+UnixSignalHandler::UnixSignalHandler(const std::function<void()>& f, QObject *parent)
72+ : QObject(parent),
73+ m_func(f)
74+{
75+ if (::socketpair(AF_UNIX, SOCK_STREAM, 0, sighupFd))
76+ {
77+ qFatal("Couldn't create HUP socketpair");
78+ }
79+ if (::socketpair(AF_UNIX, SOCK_STREAM, 0, sigintFd))
80+ {
81+ qFatal("Couldn't create INT socketpair");
82+ }
83+ if (::socketpair(AF_UNIX, SOCK_STREAM, 0, sigtermFd))
84+ {
85+ qFatal("Couldn't create TERM socketpair");
86+ }
87+
88+ m_socketNotifierHup = new QSocketNotifier(sighupFd[1], QSocketNotifier::Read, this);
89+ connect(m_socketNotifierHup, &QSocketNotifier::activated, this, &UnixSignalHandler::handleSigHup);
90+ m_socketNotifierInt = new QSocketNotifier(sigintFd[1], QSocketNotifier::Read, this);
91+ connect(m_socketNotifierInt, &QSocketNotifier::activated, this, &UnixSignalHandler::handleSigInt);
92+ m_socketNotifierTerm = new QSocketNotifier(sigtermFd[1], QSocketNotifier::Read, this);
93+ connect(m_socketNotifierTerm, &QSocketNotifier::activated, this, &UnixSignalHandler::handleSigTerm);
94+}
95+
96+void UnixSignalHandler::hupSignalHandler(int)
97+{
98+ char a = 1;
99+ ::write(sighupFd[0], &a, sizeof(a));
100+}
101+
102+void UnixSignalHandler::intSignalHandler(int)
103+{
104+ char a = 1;
105+ ::write(sigintFd[0], &a, sizeof(a));
106+}
107+
108+void UnixSignalHandler::termSignalHandler(int)
109+{
110+ char a = 1;
111+ ::write(sigtermFd[0], &a, sizeof(a));
112+}
113+
114+int UnixSignalHandler::setupUnixSignalHandlers()
115+{
116+ struct sigaction sighup, sigint, sigterm;
117+
118+ sighup.sa_handler = UnixSignalHandler::hupSignalHandler;
119+ sigemptyset(&sighup.sa_mask);
120+ sighup.sa_flags = 0;
121+ sighup.sa_flags |= SA_RESTART;
122+
123+ if (sigaction(SIGHUP, &sighup, 0) > 0)
124+ {
125+ return 1;
126+ }
127+
128+ sigint.sa_handler = UnixSignalHandler::intSignalHandler;
129+ sigemptyset(&sigint.sa_mask);
130+ sigint.sa_flags = 0;
131+ sigint.sa_flags |= SA_RESTART;
132+
133+ if (sigaction(SIGINT, &sigint, 0) > 0)
134+ {
135+ return 2;
136+ }
137+
138+ sigterm.sa_handler = UnixSignalHandler::termSignalHandler;
139+ sigemptyset(&sigterm.sa_mask);
140+ sigterm.sa_flags |= SA_RESTART;
141+
142+ if (sigaction(SIGTERM, &sigterm, 0) > 0)
143+ {
144+ return 3;
145+ }
146+
147+ return 0;
148+}
149+
150+void UnixSignalHandler::handleSigHup()
151+{
152+ m_socketNotifierHup->setEnabled(false);
153+ char tmp;
154+ ::read(sighupFd[1], &tmp, sizeof(tmp));
155+
156+ m_func();
157+
158+ m_socketNotifierHup->setEnabled(true);
159+}
160+
161+void UnixSignalHandler::handleSigInt()
162+{
163+ m_socketNotifierInt->setEnabled(false);
164+ char tmp;
165+ ::read(sigintFd[1], &tmp, sizeof(tmp));
166+
167+ m_func();
168+
169+ m_socketNotifierInt->setEnabled(true);
170+}
171+
172+void UnixSignalHandler::handleSigTerm()
173+{
174+ m_socketNotifierTerm->setEnabled(false);
175+ char tmp;
176+ ::read(sigtermFd[1], &tmp, sizeof(tmp));
177+
178+ m_func();
179+
180+ m_socketNotifierTerm->setEnabled(true);
181+}
182
183=== added file 'tools/unix-signal-handler.h'
184--- tools/unix-signal-handler.h 1970-01-01 00:00:00 +0000
185+++ tools/unix-signal-handler.h 2015-07-01 10:55:15 +0000
186@@ -0,0 +1,54 @@
187+/*
188+ * Copyright (C) 2015 Canonical, Ltd.
189+ *
190+ * This program is free software: you can redistribute it and/or modify it
191+ * under the terms of the GNU General Public License version 3, as published
192+ * by the Free Software Foundation.
193+ *
194+ * This program is distributed in the hope that it will be useful, but
195+ * WITHOUT ANY WARRANTY; without even the implied warranties of
196+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
197+ * PURPOSE. See the GNU General Public License for more details.
198+ *
199+ * You should have received a copy of the GNU General Public License along
200+ * with this program. If not, see <http://www.gnu.org/licenses/>.
201+ *
202+ * Author: Marcus Tomlinson <marcus.tomlinson@canonical.com>
203+ */
204+
205+#pragma once
206+
207+#include <QObject>
208+#include <QSocketNotifier>
209+
210+#include <functional>
211+
212+class UnixSignalHandler: public QObject {
213+Q_OBJECT
214+
215+public:
216+ UnixSignalHandler(const std::function<void()>& f, QObject *parent = 0);
217+ ~UnixSignalHandler() = default;
218+
219+ static int setupUnixSignalHandlers();
220+
221+protected Q_SLOTS:
222+ void handleSigHup();
223+ void handleSigInt();
224+ void handleSigTerm();
225+
226+protected:
227+ static void hupSignalHandler(int unused);
228+ static void intSignalHandler(int unused);
229+ static void termSignalHandler(int unused);
230+
231+ static int sighupFd[2];
232+ static int sigintFd[2];
233+ static int sigtermFd[2];
234+
235+ std::function<void()> m_func;
236+
237+ QSocketNotifier *m_socketNotifierHup;
238+ QSocketNotifier *m_socketNotifierInt;
239+ QSocketNotifier *m_socketNotifierTerm;
240+};

Subscribers

People subscribed via source and target branches