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
=== modified file 'tools/CMakeLists.txt'
--- tools/CMakeLists.txt 2014-03-21 14:31:17 +0000
+++ tools/CMakeLists.txt 2015-07-01 10:55:15 +0000
@@ -1,6 +1,7 @@
1add_executable(${SCOPE_TOOL}1add_executable(${SCOPE_TOOL}
2 scopetool.cpp2 scopetool.cpp
3 registry-tracker.cpp3 registry-tracker.cpp
4 unix-signal-handler.cpp
4 )5 )
56
6qt5_use_modules(${SCOPE_TOOL} Qml Quick)7qt5_use_modules(${SCOPE_TOOL} Qml Quick)
78
=== modified file 'tools/scopetool.cpp'
--- tools/scopetool.cpp 2014-09-04 11:36:55 +0000
+++ tools/scopetool.cpp 2015-07-01 10:55:15 +0000
@@ -32,6 +32,7 @@
32// local32// local
33#include <paths.h>33#include <paths.h>
34#include "registry-tracker.h"34#include "registry-tracker.h"
35#include "unix-signal-handler.h"
3536
3637
37int main(int argc, char *argv[])38int main(int argc, char *argv[])
@@ -96,6 +97,11 @@
9697
97 view->show();98 view->show();
9899
100 UnixSignalHandler signal_handler([]{
101 QGuiApplication::exit(0);
102 });
103 signal_handler.setupUnixSignalHandlers();
104
99 int result = application->exec();105 int result = application->exec();
100106
101 delete view;107 delete view;
102108
=== added file 'tools/unix-signal-handler.cpp'
--- tools/unix-signal-handler.cpp 1970-01-01 00:00:00 +0000
+++ tools/unix-signal-handler.cpp 2015-07-01 10:55:15 +0000
@@ -0,0 +1,141 @@
1/*
2 * Copyright (C) 2015 Canonical, Ltd.
3 *
4 * This program is free software: you can redistribute it and/or modify it
5 * under the terms of the GNU General Public License version 3, as published
6 * by the Free Software Foundation.
7 *
8 * This program is distributed in the hope that it will be useful, but
9 * WITHOUT ANY WARRANTY; without even the implied warranties of
10 * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
11 * PURPOSE. See the GNU General Public License for more details.
12 *
13 * You should have received a copy of the GNU General Public License along
14 * with this program. If not, see <http://www.gnu.org/licenses/>.
15 *
16 * Author: Marcus Tomlinson <marcus.tomlinson@canonical.com>
17 */
18
19#include "unix-signal-handler.h"
20
21#include <QDebug>
22
23#include <csignal>
24#include <sys/socket.h>
25#include <unistd.h>
26
27int UnixSignalHandler::sighupFd[2];
28int UnixSignalHandler::sigintFd[2];
29int UnixSignalHandler::sigtermFd[2];
30
31UnixSignalHandler::UnixSignalHandler(const std::function<void()>& f, QObject *parent)
32 : QObject(parent),
33 m_func(f)
34{
35 if (::socketpair(AF_UNIX, SOCK_STREAM, 0, sighupFd))
36 {
37 qFatal("Couldn't create HUP socketpair");
38 }
39 if (::socketpair(AF_UNIX, SOCK_STREAM, 0, sigintFd))
40 {
41 qFatal("Couldn't create INT socketpair");
42 }
43 if (::socketpair(AF_UNIX, SOCK_STREAM, 0, sigtermFd))
44 {
45 qFatal("Couldn't create TERM socketpair");
46 }
47
48 m_socketNotifierHup = new QSocketNotifier(sighupFd[1], QSocketNotifier::Read, this);
49 connect(m_socketNotifierHup, &QSocketNotifier::activated, this, &UnixSignalHandler::handleSigHup);
50 m_socketNotifierInt = new QSocketNotifier(sigintFd[1], QSocketNotifier::Read, this);
51 connect(m_socketNotifierInt, &QSocketNotifier::activated, this, &UnixSignalHandler::handleSigInt);
52 m_socketNotifierTerm = new QSocketNotifier(sigtermFd[1], QSocketNotifier::Read, this);
53 connect(m_socketNotifierTerm, &QSocketNotifier::activated, this, &UnixSignalHandler::handleSigTerm);
54}
55
56void UnixSignalHandler::hupSignalHandler(int)
57{
58 char a = 1;
59 ::write(sighupFd[0], &a, sizeof(a));
60}
61
62void UnixSignalHandler::intSignalHandler(int)
63{
64 char a = 1;
65 ::write(sigintFd[0], &a, sizeof(a));
66}
67
68void UnixSignalHandler::termSignalHandler(int)
69{
70 char a = 1;
71 ::write(sigtermFd[0], &a, sizeof(a));
72}
73
74int UnixSignalHandler::setupUnixSignalHandlers()
75{
76 struct sigaction sighup, sigint, sigterm;
77
78 sighup.sa_handler = UnixSignalHandler::hupSignalHandler;
79 sigemptyset(&sighup.sa_mask);
80 sighup.sa_flags = 0;
81 sighup.sa_flags |= SA_RESTART;
82
83 if (sigaction(SIGHUP, &sighup, 0) > 0)
84 {
85 return 1;
86 }
87
88 sigint.sa_handler = UnixSignalHandler::intSignalHandler;
89 sigemptyset(&sigint.sa_mask);
90 sigint.sa_flags = 0;
91 sigint.sa_flags |= SA_RESTART;
92
93 if (sigaction(SIGINT, &sigint, 0) > 0)
94 {
95 return 2;
96 }
97
98 sigterm.sa_handler = UnixSignalHandler::termSignalHandler;
99 sigemptyset(&sigterm.sa_mask);
100 sigterm.sa_flags |= SA_RESTART;
101
102 if (sigaction(SIGTERM, &sigterm, 0) > 0)
103 {
104 return 3;
105 }
106
107 return 0;
108}
109
110void UnixSignalHandler::handleSigHup()
111{
112 m_socketNotifierHup->setEnabled(false);
113 char tmp;
114 ::read(sighupFd[1], &tmp, sizeof(tmp));
115
116 m_func();
117
118 m_socketNotifierHup->setEnabled(true);
119}
120
121void UnixSignalHandler::handleSigInt()
122{
123 m_socketNotifierInt->setEnabled(false);
124 char tmp;
125 ::read(sigintFd[1], &tmp, sizeof(tmp));
126
127 m_func();
128
129 m_socketNotifierInt->setEnabled(true);
130}
131
132void UnixSignalHandler::handleSigTerm()
133{
134 m_socketNotifierTerm->setEnabled(false);
135 char tmp;
136 ::read(sigtermFd[1], &tmp, sizeof(tmp));
137
138 m_func();
139
140 m_socketNotifierTerm->setEnabled(true);
141}
0142
=== added file 'tools/unix-signal-handler.h'
--- tools/unix-signal-handler.h 1970-01-01 00:00:00 +0000
+++ tools/unix-signal-handler.h 2015-07-01 10:55:15 +0000
@@ -0,0 +1,54 @@
1/*
2 * Copyright (C) 2015 Canonical, Ltd.
3 *
4 * This program is free software: you can redistribute it and/or modify it
5 * under the terms of the GNU General Public License version 3, as published
6 * by the Free Software Foundation.
7 *
8 * This program is distributed in the hope that it will be useful, but
9 * WITHOUT ANY WARRANTY; without even the implied warranties of
10 * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
11 * PURPOSE. See the GNU General Public License for more details.
12 *
13 * You should have received a copy of the GNU General Public License along
14 * with this program. If not, see <http://www.gnu.org/licenses/>.
15 *
16 * Author: Marcus Tomlinson <marcus.tomlinson@canonical.com>
17 */
18
19#pragma once
20
21#include <QObject>
22#include <QSocketNotifier>
23
24#include <functional>
25
26class UnixSignalHandler: public QObject {
27Q_OBJECT
28
29public:
30 UnixSignalHandler(const std::function<void()>& f, QObject *parent = 0);
31 ~UnixSignalHandler() = default;
32
33 static int setupUnixSignalHandlers();
34
35protected Q_SLOTS:
36 void handleSigHup();
37 void handleSigInt();
38 void handleSigTerm();
39
40protected:
41 static void hupSignalHandler(int unused);
42 static void intSignalHandler(int unused);
43 static void termSignalHandler(int unused);
44
45 static int sighupFd[2];
46 static int sigintFd[2];
47 static int sigtermFd[2];
48
49 std::function<void()> m_func;
50
51 QSocketNotifier *m_socketNotifierHup;
52 QSocketNotifier *m_socketNotifierInt;
53 QSocketNotifier *m_socketNotifierTerm;
54};

Subscribers

People subscribed via source and target branches