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 <hjk@qt.io>

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 (installShellScript()) {
-                if (m_shellScriptState == State::FailedToStart)
-                    closeShellProcess();
-            } else {
                 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("<missing>")) {
-        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;

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,

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)
+FINAL_OUT=$(mktemp -u) || exit 2
-mkfifo "$FINAL_OUT"
+mkfifo "$FINAL_OUT" || exit 3
 
 finalOutput() {
     local fileInputBuffer

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

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