From 3610a99ebb8066ca30bee08775cc72de2397fad8 Mon Sep 17 00:00:00 2001 From: Robert Griebl Date: Wed, 1 Mar 2023 12:01:03 +0100 Subject: appman-controller: detect broken D-Bus connections and quit with error In case the AM crashed or the bus died during an install-package command, the appman-controller would have waited for a taskFail / taskFinished signal infinitely. We now detect a D-Bus service owner change (the AM crashed) and a disconnected D-Bus (the bus died). Even though the D-Bus daemon dying sounds unlikely, it is the norm when running the AM with --dbus=auto (the default). If the AM crashes (or gets terminated by an IDE), the private dbus-daemon just quits. This makes the controller much more suitable for scripting. Also fixed a bunch of clang/clazy warnings in this file. Change-Id: I01a3772fd8773d707984a07d38cbce1d7ab36c94 Reviewed-by: Dominik Holland (cherry picked from commit ff5f0d9c4d14042eb020a8ba5cb9ee7e51195a65) Reviewed-by: Qt Cherry-pick Bot --- src/tools/controller/controller.cpp | 146 ++++++++++++++++++++++++++---------- 1 file changed, 107 insertions(+), 39 deletions(-) diff --git a/src/tools/controller/controller.cpp b/src/tools/controller/controller.cpp index 5c2b5d8b..35123386 100644 --- a/src/tools/controller/controller.cpp +++ b/src/tools/controller/controller.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -32,8 +33,10 @@ QT_USE_NAMESPACE_AM -class DBus : public QObject // clazy:exclude=missing-qobject-macro +class DBus : public QObject { + Q_OBJECT + public: DBus() { @@ -65,6 +68,9 @@ public: m_packager = new IoQtPackageManagerInterface(qSL("io.qt.ApplicationManager"), qSL("/PackageManager"), conn, this); } +signals: + void disconnected(QString reason); + private: QDBusConnection connectTo(const QString &iface) Q_DECL_NOEXCEPT_EXPR(false) { @@ -91,9 +97,53 @@ private: throw Exception(Error::IO, "Could not connect to the application manager D-Bus interface %1 at %2: %3") .arg(iface, dbus, conn.lastError().message()); } + + installDisconnectWatcher(conn, qSL("io.qt.ApplicationManager")); return conn; } + void installDisconnectWatcher(const QDBusConnection &conn, const QString &serviceName) + { + if (m_disconnectedEmitted) + return; + + if (!m_connections.contains(conn.name())) { + auto *watcher = new QDBusServiceWatcher(serviceName, conn, QDBusServiceWatcher::WatchForOwnerChange, this); + connect(watcher, &QDBusServiceWatcher::serviceOwnerChanged, + this, [this](const QString &, const QString &, const QString &) { + disconnectDetected(qSL("owner changed")); + }); + m_connections.append(conn.name()); + } + + // serviceOwnerChanged does not work if the bus-daemon process dies (as is the case when + // the AM starts its own session bus in --dbus=auto mode and then later crashes, killing + // the bus-daemon with it). + // QDBusConnection::isConnected() does not have a change signal, so we have to poll. + if (!m_disconnectTimer) { + m_disconnectTimer = new QTimer(this); + connect(m_disconnectTimer, &QTimer::timeout, this, [this]() { + for (const auto &name : std::as_const(m_connections)) { + if (!QDBusConnection(name).isConnected()) { + disconnectDetected(qSL("bus died")); + break; + } + } + }); + m_disconnectTimer->start(500); + } + } + + void disconnectDetected(const QString &reason) + { + if (!m_disconnectedEmitted) { + emit disconnected(reason); + m_disconnectedEmitted = true; + if (m_disconnectTimer) + m_disconnectTimer->stop(); + } + } + public: IoQtPackageManagerInterface *packager() const { @@ -109,6 +159,9 @@ private: IoQtPackageManagerInterface *m_packager = nullptr; IoQtApplicationManagerInterface *m_manager = nullptr; QString m_instanceId; + QStringList m_connections; + QTimer *m_disconnectTimer = nullptr; + bool m_disconnectedEmitted = false; }; static class DBus dbus; @@ -186,7 +239,7 @@ static void showPackage(const QString &packageId, bool asJson = false) Q_DECL_NO static void installPackage(const QString &packageUrl, bool acknowledge) Q_DECL_NOEXCEPT_EXPR(false); static void removePackage(const QString &packageId, bool keepDocuments, bool force) Q_DECL_NOEXCEPT_EXPR(false); static void listInstallationTasks() Q_DECL_NOEXCEPT_EXPR(false); -static void cancelInstallationTask(bool all, const QString &taskId) Q_DECL_NOEXCEPT_EXPR(false); +static void cancelInstallationTask(bool all, const QString &singleTaskId) Q_DECL_NOEXCEPT_EXPR(false); static void listInstallationLocations() Q_DECL_NOEXCEPT_EXPR(false); static void showInstallationLocation(bool asJson = false) Q_DECL_NOEXCEPT_EXPR(false); static void listInstances() Q_DECL_NOEXCEPT_EXPR(false); @@ -238,7 +291,7 @@ int main(int argc, char *argv[]) ThrowingApplication a(argc, argv); QByteArray desc = "\n\nAvailable commands are:\n"; - uint longestName = 0; + size_t longestName = 0; for (uint i = 0; i < sizeof(commandTable) / sizeof(commandTable[0]); ++i) longestName = qMax(longestName, qstrlen(commandTable[i].name)); for (uint i = 0; i < sizeof(commandTable) / sizeof(commandTable[0]); ++i) { @@ -293,8 +346,8 @@ int main(int argc, char *argv[]) clp.addPositionalArgument(qSL("document-url"), qSL("The optional document-url."), qSL("[document-url]")); clp.process(a); - int args = clp.positionalArguments().size(); - if (args < 2 || args > 3) + int args = int(clp.positionalArguments().size()); + if ((args < 2) || (args > 3)) clp.showHelp(1); QMap stdRedirections; @@ -324,8 +377,8 @@ int main(int argc, char *argv[]) clp.addPositionalArgument(qSL("document-url"), qSL("The optional document-url."), qSL("[document-url]")); clp.process(a); - int args = clp.positionalArguments().size(); - if (args < 3 || args > 4) + int args = int(clp.positionalArguments().size()); + if ((args < 3) || (args > 4)) clp.showHelp(1); QMap stdRedirections; @@ -511,7 +564,7 @@ int main(int argc, char *argv[]) } catch (const Exception &e) { fprintf(stderr, "ERROR: %s\n", qPrintable(e.errorString())); - return 1; + return int(e.errorCode()); } } @@ -552,6 +605,12 @@ void startOrDebugApplication(const QString &debugWrapper, const QString &appId, static bool isStarted = false; if (!stdRedirections.isEmpty()) { + // just bail out, if the AM or bus dies + QObject::connect(&dbus, &DBus::disconnected, + qApp, [](const QString &reason) { + throw Exception(Error::IO, "application might not be running: lost connection to the D-Bus service (%1)").arg(reason); + }); + // in case application quits -> quit the controller QObject::connect(dbus.manager(), &IoQtApplicationManagerInterface::applicationRunStateChanged, qApp, [appId](const QString &id, uint runState) { @@ -588,18 +647,14 @@ void startOrDebugApplication(const QString &debugWrapper, const QString &appId, } isStarted = reply.value(); - if (stdRedirections.isEmpty()) { + if (stdRedirections.isEmpty() || !isStarted) { qApp->exit(isStarted ? 0 : 2); } else { - if (!isStarted) { - qApp->exit(2); - } else { - InterruptHandler::install([appId](int) { - auto reply = dbus.manager()->stopApplication(appId, true); - reply.waitForFinished(); - qApp->exit(1); - }); - } + InterruptHandler::install([appId](int) { + auto stopReply = dbus.manager()->stopApplication(appId, true); + stopReply.waitForFinished(); + qApp->exit(1); + }); } } @@ -718,14 +773,19 @@ void installPackage(const QString &package, bool acknowledge) Q_DECL_NOEXCEPT_EX dbus.connectToManager(); dbus.connectToPackager(); + // just bail out, if the AM or bus dies + QObject::connect(&dbus, &DBus::disconnected, + qApp, [](const QString &reason) { + throw Exception(Error::IO, "package might not be installed: lost connection to the D-Bus service (%1)").arg(reason); + }); + // all the async lambdas below need to share this variable static QString installationId; // as soon as we have the manifest available: get the app id and acknowledge the installation - if (acknowledge) { QObject::connect(dbus.packager(), &IoQtPackageManagerInterface::taskRequestingInstallationAcknowledge, - [](const QString &taskId, const QVariantMap &metadata) { + qApp, [](const QString &taskId, const QVariantMap &metadata) { if (taskId != installationId) return; QString packageId = metadata.value(qSL("packageId")).toString(); @@ -737,18 +797,16 @@ void installPackage(const QString &package, bool acknowledge) Q_DECL_NOEXCEPT_EX } // on failure: quit - QObject::connect(dbus.packager(), &IoQtPackageManagerInterface::taskFailed, - [](const QString &taskId, int errorCode, const QString &errorString) { + qApp, [](const QString &taskId, int errorCode, const QString &errorString) { if (taskId != installationId) return; throw Exception(Error::IO, "failed to install package: %1 (code: %2)").arg(errorString).arg(errorCode); }); // on success - QObject::connect(dbus.packager(), &IoQtPackageManagerInterface::taskFinished, - [](const QString &taskId) { + qApp, [](const QString &taskId) { if (taskId != installationId) return; fprintf(stdout, "Package installation finished successfully.\n"); @@ -770,8 +828,8 @@ void installPackage(const QString &package, bool acknowledge) Q_DECL_NOEXCEPT_EX InterruptHandler::install([](int) { fprintf(stdout, "Cancelling package installation.\n"); - auto reply = dbus.packager()->cancelTask(installationId); - reply.waitForFinished(); + auto cancelReply = dbus.packager()->cancelTask(installationId); + cancelReply.waitForFinished(); qApp->exit(1); }); } @@ -783,30 +841,33 @@ void removePackage(const QString &packageId, bool keepDocuments, bool force) Q_D dbus.connectToManager(); dbus.connectToPackager(); + // just bail out, if the AM or bus dies + QObject::connect(&dbus, &DBus::disconnected, + qApp, [](const QString &reason) { + throw Exception(Error::IO, "package might not be removed: lost connection to the D-Bus service (%1)").arg(reason); + }); + // both the async lambdas below need to share this variables static QString installationId; // on failure: quit - QObject::connect(dbus.packager(), &IoQtPackageManagerInterface::taskFailed, - [](const QString &taskId, int errorCode, const QString &errorString) { + qApp, [](const QString &taskId, int errorCode, const QString &errorString) { if (taskId != installationId) return; throw Exception(Error::IO, "failed to remove package: %1 (code: %2)").arg(errorString).arg(errorCode); }); // on success - QObject::connect(dbus.packager(), &IoQtPackageManagerInterface::taskFinished, - [](const QString &taskId) { + qApp, [](const QString &taskId) { if (taskId != installationId) return; fprintf(stdout, "Package removal finished successfully.\n"); qApp->quit(); }); - // start the package installation - + // start the package removal auto reply = dbus.packager()->removePackage(packageId, keepDocuments, force); reply.waitForFinished(); if (reply.isError()) @@ -833,11 +894,17 @@ void listInstallationTasks() Q_DECL_NOEXCEPT_EXPR(false) } -void cancelInstallationTask(bool all, const QString &taskId) Q_DECL_NOEXCEPT_EXPR(false) +void cancelInstallationTask(bool all, const QString &singleTaskId) Q_DECL_NOEXCEPT_EXPR(false) { dbus.connectToPackager(); - // both the async lambdas below need to share this variables + // just bail out, if the AM or bus dies + QObject::connect(&dbus, &DBus::disconnected, + qApp, [](const QString &reason) { + throw Exception(Error::IO, "installation task(s) might not be canceled: lost connection to the D-Bus service (%1)").arg(reason); + }); + + // both the async lambdas below need to share these variables static QStringList cancelTaskIds; static int result = 0; @@ -850,19 +917,19 @@ void cancelInstallationTask(bool all, const QString &taskId) Q_DECL_NOEXCEPT_EXP throw Exception(Error::IO, "failed to call activeTaskIds via DBus: %1").arg(reply.error().message()); const auto taskIds = reply.value(); + cancelTaskIds.reserve(taskIds.size()); for (const auto &taskId : taskIds) cancelTaskIds << taskId; } else { - cancelTaskIds << taskId; + cancelTaskIds << singleTaskId; } if (cancelTaskIds.isEmpty()) qApp->quit(); // on task failure - QObject::connect(dbus.packager(), &IoQtPackageManagerInterface::taskFailed, - [](const QString &taskId, int errorCode, const QString &errorString) { + qApp, [](const QString &taskId, int errorCode, const QString &errorString) { if (cancelTaskIds.removeOne(taskId)) { if (errorCode != int(Error::Canceled)) { fprintf(stdout, "Could not cancel task %s anymore - the installation task already failed (%s).\n", @@ -877,9 +944,8 @@ void cancelInstallationTask(bool all, const QString &taskId) Q_DECL_NOEXCEPT_EXP }); // on success - QObject::connect(dbus.packager(), &IoQtPackageManagerInterface::taskFinished, - [](const QString &taskId) { + qApp, [](const QString &taskId) { if (cancelTaskIds.removeOne(taskId)) { fprintf(stdout, "Could not cancel task %s anymore - the installation task already finished successfully.\n", qPrintable(taskId)); @@ -972,3 +1038,5 @@ void injectIntentRequest(const QString &intentId, bool isBroadcast, qApp->quit(); } + +#include "controller.moc" -- cgit v1.2.1