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

[Marcus Tomlinson]

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

 * 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

[Albert Astals Cid (aacid)]

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

[Albert Astals Cid (aacid)]

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

review: Needs Information

[Marcus Tomlinson (marcustomlinson)]

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

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1833
http://jenkins.qa.ubuntu.com/job/unity8-ci/5909/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-wily-touch/229/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-amd64-ci/189
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity8-wily-i386-ci/190
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-wily-mako/153/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/229
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-wily-armhf/229/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/21592

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/5909/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

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

review: Needs Fixing (continuous-integration)

[Albert Astals Cid (aacid)]

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

review: Needs Information

[Marcus Tomlinson (marcustomlinson)]

> 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)]

> > 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)]

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)]

> 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)]

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)]

> 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)]

Fine by me. Signals suck...

review: Approve

[Albert Astals Cid (aacid)]

 * 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

=== 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 @@
 add_executable(${SCOPE_TOOL}
     scopetool.cpp
     registry-tracker.cpp
+    unix-signal-handler.cpp
     )
 
 qt5_use_modules(${SCOPE_TOOL} Qml Quick)

=== 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 @@
 // local
 #include <paths.h>
 #include "registry-tracker.h"
+#include "unix-signal-handler.h"
 
 
 int main(int argc, char *argv[])
@@ -96,6 +97,11 @@
 
     view->show();
 
+    UnixSignalHandler signal_handler([]{
+        QGuiApplication::exit(0);
+    });
+    signal_handler.setupUnixSignalHandlers();
+
     int result = application->exec();
 
     delete view;

=== 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 @@
+/*
+ * Copyright (C) 2015 Canonical, Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Author: Marcus Tomlinson <[e-mail]>
+ */
+
+#include "unix-signal-handler.h"
+
+#include <QDebug>
+
+#include <csignal>
+#include <sys/socket.h>
+#include <unistd.h>
+
+int UnixSignalHandler::sighupFd[2];
+int UnixSignalHandler::sigintFd[2];
+int UnixSignalHandler::sigtermFd[2];
+
+UnixSignalHandler::UnixSignalHandler(const std::function<void()>& f, QObject *parent)
+    : QObject(parent),
+      m_func(f)
+{
+    if (::socketpair(AF_UNIX, SOCK_STREAM, 0, sighupFd))
+    {
+        qFatal("Couldn't create HUP socketpair");
+    }
+    if (::socketpair(AF_UNIX, SOCK_STREAM, 0, sigintFd))
+    {
+        qFatal("Couldn't create INT socketpair");
+    }
+    if (::socketpair(AF_UNIX, SOCK_STREAM, 0, sigtermFd))
+    {
+        qFatal("Couldn't create TERM socketpair");
+    }
+
+    m_socketNotifierHup = new QSocketNotifier(sighupFd[1], QSocketNotifier::Read, this);
+    connect(m_socketNotifierHup, &QSocketNotifier::activated, this, &UnixSignalHandler::handleSigHup);
+    m_socketNotifierInt = new QSocketNotifier(sigintFd[1], QSocketNotifier::Read, this);
+    connect(m_socketNotifierInt, &QSocketNotifier::activated, this, &UnixSignalHandler::handleSigInt);
+    m_socketNotifierTerm = new QSocketNotifier(sigtermFd[1], QSocketNotifier::Read, this);
+    connect(m_socketNotifierTerm, &QSocketNotifier::activated, this, &UnixSignalHandler::handleSigTerm);
+}
+
+void UnixSignalHandler::hupSignalHandler(int)
+{
+    char a = 1;
+    ::write(sighupFd[0], &a, sizeof(a));
+}
+
+void UnixSignalHandler::intSignalHandler(int)
+{
+    char a = 1;
+    ::write(sigintFd[0], &a, sizeof(a));
+}
+
+void UnixSignalHandler::termSignalHandler(int)
+{
+    char a = 1;
+    ::write(sigtermFd[0], &a, sizeof(a));
+}
+
+int UnixSignalHandler::setupUnixSignalHandlers()
+{
+    struct sigaction sighup, sigint, sigterm;
+
+    sighup.sa_handler = UnixSignalHandler::hupSignalHandler;
+    sigemptyset(&sighup.sa_mask);
+    sighup.sa_flags = 0;
+    sighup.sa_flags |= SA_RESTART;
+
+    if (sigaction(SIGHUP, &sighup, 0) > 0)
+    {
+        return 1;
+    }
+
+    sigint.sa_handler = UnixSignalHandler::intSignalHandler;
+    sigemptyset(&sigint.sa_mask);
+    sigint.sa_flags = 0;
+    sigint.sa_flags |= SA_RESTART;
+
+    if (sigaction(SIGINT, &sigint, 0) > 0)
+    {
+        return 2;
+    }
+
+    sigterm.sa_handler = UnixSignalHandler::termSignalHandler;
+    sigemptyset(&sigterm.sa_mask);
+    sigterm.sa_flags |= SA_RESTART;
+
+    if (sigaction(SIGTERM, &sigterm, 0) > 0)
+    {
+        return 3;
+    }
+
+    return 0;
+}
+
+void UnixSignalHandler::handleSigHup()
+{
+    m_socketNotifierHup->setEnabled(false);
+    char tmp;
+    ::read(sighupFd[1], &tmp, sizeof(tmp));
+
+    m_func();
+
+    m_socketNotifierHup->setEnabled(true);
+}
+
+void UnixSignalHandler::handleSigInt()
+{
+    m_socketNotifierInt->setEnabled(false);
+    char tmp;
+    ::read(sigintFd[1], &tmp, sizeof(tmp));
+
+    m_func();
+
+    m_socketNotifierInt->setEnabled(true);
+}
+
+void UnixSignalHandler::handleSigTerm()
+{
+    m_socketNotifierTerm->setEnabled(false);
+    char tmp;
+    ::read(sigtermFd[1], &tmp, sizeof(tmp));
+
+    m_func();
+
+    m_socketNotifierTerm->setEnabled(true);
+}

=== 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 @@
+/*
+ * Copyright (C) 2015 Canonical, Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3, as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranties of
+ * MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+ * PURPOSE.  See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Author: Marcus Tomlinson <[e-mail]>
+ */
+
+#pragma once
+
+#include <QObject>
+#include <QSocketNotifier>
+
+#include <functional>
+
+class UnixSignalHandler: public QObject {
+Q_OBJECT
+
+public:
+    UnixSignalHandler(const std::function<void()>& f, QObject *parent = 0);
+    ~UnixSignalHandler() = default;
+
+    static int setupUnixSignalHandlers();
+
+protected Q_SLOTS:
+    void handleSigHup();
+    void handleSigInt();
+    void handleSigTerm();
+
+protected:
+    static void hupSignalHandler(int unused);
+    static void intSignalHandler(int unused);
+    static void termSignalHandler(int unused);
+
+    static int sighupFd[2];
+    static int sigintFd[2];
+    static int sigtermFd[2];
+
+    std::function<void()> m_func;
+
+    QSocketNotifier *m_socketNotifierHup;
+    QSocketNotifier *m_socketNotifierInt;
+    QSocketNotifier *m_socketNotifierTerm;
+};