From 9ddd1e1d362e259aeb291031c27c0021c7f922bd Mon Sep 17 00:00:00 2001 From: Marcus Tillmanns Date: Tue, 21 Mar 2023 11:28:54 +0100 Subject: [PATCH] Utils: Improve DeviceShell error handling In case mktemp or mkfifo fails, the device shell script would incorrectly print both success and failure messages. Also cleaning State enum, removing unnecessary values. Fixes possible crash if creation of master process fails. Fixes: QTCREATORBUG-28928 Change-Id: I75fef54dc791b2b0a403bab19dab6813b62643ac Reviewed-by: hjk --- src/libs/utils/deviceshell.cpp | 24 +++++++++++-------- src/libs/utils/deviceshell.h | 2 +- src/libs/utils/scripts/deviceshell.sh | 4 ++-- src/plugins/remotelinux/linuxdevice.cpp | 4 +++- .../utils/deviceshell/tst_deviceshell.cpp | 2 +- 5 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/libs/utils/deviceshell.cpp b/src/libs/utils/deviceshell.cpp index 283e9789c00..18e816d38b2 100644 --- a/src/libs/utils/deviceshell.cpp +++ b/src/libs/utils/deviceshell.cpp @@ -62,7 +62,6 @@ DeviceShell::~DeviceShell() */ RunResult DeviceShell::runInShell(const CommandLine &cmd, const QByteArray &stdInData) { - QTC_ASSERT(m_shellProcess, return {}); Q_ASSERT(QThread::currentThread() != &m_thread); return run(cmd, stdInData); @@ -75,7 +74,7 @@ QStringList DeviceShell::missingFeatures() const { return m_missingFeatures; } RunResult DeviceShell::run(const CommandLine &cmd, const QByteArray &stdInData) { // If the script failed to install, use QtcProcess directly instead. - bool useProcess = m_shellScriptState == State::NoScript; + bool useProcess = m_shellScriptState == State::Failed; // Transferring large amounts of stdInData is slow via the shell script. // Use QtcProcess directly if the size exceeds 100kb. @@ -197,10 +196,7 @@ bool DeviceShell::start() return false; } - if (!installShellScript()) { - if (m_shellScriptState == State::FailedToStart) - closeShellProcess(); - } else { + if (installShellScript()) { connect(m_shellProcess.get(), &QtcProcess::readyReadStandardOutput, m_shellProcess.get(), @@ -213,6 +209,10 @@ bool DeviceShell::start() qCWarning(deviceShellLog) << "Received unexpected output on stderr:" << stdErr; }); + } else if (m_shellProcess->isRunning()) { + m_shellProcess->kill(); + m_shellProcess.reset(); + return false; } connect(m_shellProcess.get(), &QtcProcess::done, m_shellProcess.get(), [this] { @@ -247,7 +247,7 @@ bool DeviceShell::checkCommand(const QByteArray &command) } QByteArray out = m_shellProcess->readAllRawStandardOutput(); if (out.contains("")) { - m_shellScriptState = State::NoScript; + m_shellScriptState = State::Failed; qCWarning(deviceShellLog) << "Command" << command << "was not found"; m_missingFeatures.append(QString::fromUtf8(command)); return false; @@ -259,7 +259,7 @@ bool DeviceShell::checkCommand(const QByteArray &command) bool DeviceShell::installShellScript() { if (m_forceFailScriptInstallation) { - m_shellScriptState = State::NoScript; + m_shellScriptState = State::Failed; return false; } @@ -290,15 +290,19 @@ bool DeviceShell::installShellScript() } QByteArray out = m_shellProcess->readAllRawStandardError(); - if (out.contains("SCRIPT_INSTALLED")) { + if (out.contains("SCRIPT_INSTALLED") && !out.contains("ERROR_INSTALL_SCRIPT")) { m_shellScriptState = State::Succeeded; return true; } if (out.contains("ERROR_INSTALL_SCRIPT")) { - m_shellScriptState = State::NoScript; + m_shellScriptState = State::Failed; qCWarning(deviceShellLog) << "Failed installing device shell script"; return false; } + if (!out.isEmpty()) { + qCWarning(deviceShellLog) + << "Unexpected output while installing device shell script:" << out; + } } return true; diff --git a/src/libs/utils/deviceshell.h b/src/libs/utils/deviceshell.h index 713f9f09e2e..83af1db365e 100644 --- a/src/libs/utils/deviceshell.h +++ b/src/libs/utils/deviceshell.h @@ -28,7 +28,7 @@ class QTCREATOR_UTILS_EXPORT DeviceShell : public QObject Q_OBJECT public: - enum class State { FailedToStart = -1, Unknown = 0, Succeeded = 1, NoScript = 2 }; + enum class State { Failed = -1, Unknown = 0, Succeeded = 1 }; enum class ParseType { StdOut, diff --git a/src/libs/utils/scripts/deviceshell.sh b/src/libs/utils/scripts/deviceshell.sh index 7281d550171..6ed7014d79a 100644 --- a/src/libs/utils/scripts/deviceshell.sh +++ b/src/libs/utils/scripts/deviceshell.sh @@ -1,7 +1,7 @@ # Copyright (C) 2022 The Qt Company Ltd. # SPDX-License-Identifier: LicenseRef-Qt-Commercial OR GPL-3.0-only WITH Qt-GPL-exception-1.0 -FINAL_OUT=$(mktemp -u) -mkfifo "$FINAL_OUT" +FINAL_OUT=$(mktemp -u) || exit 2 +mkfifo "$FINAL_OUT" || exit 3 finalOutput() { local fileInputBuffer diff --git a/src/plugins/remotelinux/linuxdevice.cpp b/src/plugins/remotelinux/linuxdevice.cpp index 781147a1e2e..5d0d2d6b92e 100644 --- a/src/plugins/remotelinux/linuxdevice.cpp +++ b/src/plugins/remotelinux/linuxdevice.cpp @@ -222,7 +222,9 @@ void SshSharedConnection::emitConnected() void SshSharedConnection::emitError(QProcess::ProcessError error, const QString &errorString) { m_state = QProcess::NotRunning; - ProcessResultData resultData = m_masterProcess->resultData(); + ProcessResultData resultData{-1, QProcess::CrashExit, QProcess::UnknownError, {}}; + if (m_masterProcess) + resultData = m_masterProcess->resultData(); resultData.m_error = error; resultData.m_errorString = errorString; emit disconnected(resultData); diff --git a/tests/auto/utils/deviceshell/tst_deviceshell.cpp b/tests/auto/utils/deviceshell/tst_deviceshell.cpp index d767d5a4799..6080f99bf7c 100644 --- a/tests/auto/utils/deviceshell/tst_deviceshell.cpp +++ b/tests/auto/utils/deviceshell/tst_deviceshell.cpp @@ -353,7 +353,7 @@ private slots: QFETCH(CommandLine, cmdLine); TestShell shell(cmdLine, true); - QCOMPARE(shell.state(), DeviceShell::State::NoScript); + QCOMPARE(shell.state(), DeviceShell::State::Failed); const RunResult result = shell.runInShell({"echo", {"Hello"}}); QCOMPARE(result.exitCode, 0);