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

Proposed by Marcus Tomlinson
Status: Merged
Approved by: Albert Astals Cid
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) Approve
Michi Henning (community) Approve
PS Jenkins bot (community) continuous-integration Needs Fixing
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.
Revision history for this message
Albert Astals Cid (aacid) wrote :

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

Revision history for this message
Albert Astals Cid (aacid) wrote :

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

review: Needs Information
Revision history for this message
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.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Albert Astals Cid (aacid) wrote :

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

review: Needs Information
Revision history for this message
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.

Revision history for this message
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

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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?

Revision history for this message
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.

Revision history for this message
Michi Henning (michihenning) wrote :

Fine by me. Signals suck...

review: Approve
Revision history for this message
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