From 61b78d88953f01ec0933c50f21a6fb92df16446a Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 7 Jun 2019 11:46:08 +0200 Subject: [PATCH 01/10] [libcalamares] Stop job threads before exit - This solves a crash where the thread is destroyed while still running (e.g. cancelling during install). - The thread might not cooperate in being terminated, but then we have a bigger problem anyway (and Calamares will still crash on exit). FIXES #1164 --- src/libcalamares/JobQueue.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/libcalamares/JobQueue.cpp b/src/libcalamares/JobQueue.cpp index 6ef055ffc..3a76aa099 100644 --- a/src/libcalamares/JobQueue.cpp +++ b/src/libcalamares/JobQueue.cpp @@ -44,7 +44,7 @@ public: } virtual ~JobThread() override; - + void setJobs( const JobList& jobs ) { m_jobs = jobs; @@ -157,6 +157,14 @@ JobQueue::JobQueue( QObject* parent ) JobQueue::~JobQueue() { + if ( m_thread->isRunning() ) + { + m_thread->terminate(); + if ( !m_thread->wait(300) ) + cError() << "Could not terminate job thread (expect a crash now)."; + delete m_thread; + } + delete m_storage; } From 92d03c2cf7af34487c3a46f83671dbcc54506f95 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 7 Jun 2019 12:10:22 +0200 Subject: [PATCH 02/10] [libcalamares] Introduce enum class for special process exit values - Replace magic numbers like -3 with named enum values (NoWorkingDirectory, for -3). - Downside is big-ugly static_casts, but that's what you get for having an int as return value for processes. --- .../utils/CalamaresUtilsSystem.cpp | 26 +++++++++---------- src/libcalamares/utils/CalamaresUtilsSystem.h | 24 +++++++++++------ 2 files changed, 29 insertions(+), 21 deletions(-) diff --git a/src/libcalamares/utils/CalamaresUtilsSystem.cpp b/src/libcalamares/utils/CalamaresUtilsSystem.cpp index 1b603a7e7..0d227c88f 100644 --- a/src/libcalamares/utils/CalamaresUtilsSystem.cpp +++ b/src/libcalamares/utils/CalamaresUtilsSystem.cpp @@ -114,14 +114,14 @@ System::mount( const QString& devicePath, const QString& options ) { if ( devicePath.isEmpty() || mountPoint.isEmpty() ) - return -3; + return static_cast(ProcessResult::Code::NoWorkingDirectory); QDir mountPointDir( mountPoint ); if ( !mountPointDir.exists() ) { bool ok = mountPointDir.mkpath( mountPoint ); if ( !ok ) - return -3; + return static_cast(ProcessResult::Code::NoWorkingDirectory); } QString program( "mount" ); @@ -147,14 +147,14 @@ System::runCommand( QString output; if ( !Calamares::JobQueue::instance() ) - return -3; + return ProcessResult::Code::NoWorkingDirectory; Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage(); if ( ( location == System::RunLocation::RunInTarget ) && ( !gs || !gs->contains( "rootMountPoint" ) ) ) { cWarning() << "No rootMountPoint in global storage"; - return -3; + return ProcessResult::Code::NoWorkingDirectory; } QProcess process; @@ -167,7 +167,7 @@ System::runCommand( if ( !QDir( destDir ).exists() ) { cWarning() << "rootMountPoint points to a dir which does not exist"; - return -3; + return ProcessResult::Code::NoWorkingDirectory; } program = "chroot"; @@ -190,7 +190,7 @@ System::runCommand( process.setWorkingDirectory( QDir( workingPath ).absolutePath() ); else cWarning() << "Invalid working directory:" << workingPath; - return -3; + return ProcessResult::Code::NoWorkingDirectory; } cDebug() << "Running" << program << RedactedList( arguments ); @@ -198,7 +198,7 @@ System::runCommand( if ( !process.waitForStarted() ) { cWarning() << "Process failed to start" << process.error(); - return -2; + return ProcessResult::Code::FailedToStart; } if ( !stdInput.isEmpty() ) @@ -211,7 +211,7 @@ System::runCommand( { cWarning().noquote().nospace() << "Timed out. Output so far:\n" << process.readAllStandardOutput(); - return -4; + return ProcessResult::Code::TimedOut; } output.append( QString::fromLocal8Bit( process.readAllStandardOutput() ).trimmed() ); @@ -219,7 +219,7 @@ System::runCommand( if ( process.exitStatus() == QProcess::CrashExit ) { cWarning().noquote().nospace() << "Process crashed. Output so far:\n" << output; - return -1; + return ProcessResult::Code::Crashed; } auto r = process.exitCode(); @@ -306,22 +306,22 @@ ProcessResult::explainProcess( int ec, const QString& command, const QString& ou ? QCoreApplication::translate( "ProcessResult", "\nThere was no output from the command.") : (QCoreApplication::translate( "ProcessResult", "\nOutput:\n") + output); - if ( ec == -1 ) //Crash! + if ( ec == static_cast(ProcessResult::Code::Crashed) ) //Crash! return JobResult::error( QCoreApplication::translate( "ProcessResult", "External command crashed." ), QCoreApplication::translate( "ProcessResult", "Command %1 crashed." ) .arg( command ) + outputMessage ); - if ( ec == -2 ) + if ( ec == static_cast(ProcessResult::Code::FailedToStart) ) return JobResult::error( QCoreApplication::translate( "ProcessResult", "External command failed to start." ), QCoreApplication::translate( "ProcessResult", "Command %1 failed to start." ) .arg( command ) ); - if ( ec == -3 ) + if ( ec == static_cast(ProcessResult::Code::NoWorkingDirectory) ) return JobResult::error( QCoreApplication::translate( "ProcessResult", "Internal error when starting command." ), QCoreApplication::translate( "ProcessResult", "Bad parameters for process job call." ) ); - if ( ec == -4 ) + if ( ec == static_cast(ProcessResult::Code::TimedOut) ) return JobResult::error( QCoreApplication::translate( "ProcessResult", "External command failed to finish." ), QCoreApplication::translate( "ProcessResult", "Command %1 failed to finish in %2 seconds." ) .arg( command ) diff --git a/src/libcalamares/utils/CalamaresUtilsSystem.h b/src/libcalamares/utils/CalamaresUtilsSystem.h index 6809859ee..c17d52e93 100644 --- a/src/libcalamares/utils/CalamaresUtilsSystem.h +++ b/src/libcalamares/utils/CalamaresUtilsSystem.h @@ -32,8 +32,16 @@ namespace CalamaresUtils class ProcessResult : public QPair< int, QString > { public: + enum class Code : int + { + Crashed = -1, // Must match special return values from QProcess + FailedToStart = -2, // Must match special return values from QProcess + NoWorkingDirectory = -3, + TimedOut = -4 + } ; + /** @brief Implicit one-argument constructor has no output, only a return code */ - ProcessResult( int r ) : QPair< int, QString >( r, QString() ) {} + ProcessResult( Code r ) : QPair< int, QString >( static_cast(r), QString() ) {} ProcessResult( int r, QString s ) : QPair< int, QString >( r, s ) {} int getExitCode() const { return first; } @@ -93,9 +101,9 @@ public: * @param filesystemName the name of the filesystem (optional). * @param options any additional options as passed to mount -o (optional). * @returns the program's exit code, or: - * -1 = QProcess crash - * -2 = QProcess cannot start - * -3 = bad arguments + * Crashed = QProcess crash + * FailedToStart = QProcess cannot start + * NoWorkingDirectory = bad arguments */ DLLEXPORT int mount( const QString& devicePath, const QString& mountPoint, @@ -120,10 +128,10 @@ public: * * @returns the program's exit code and its output (if any). Special * exit codes (which will never have any output) are: - * -1 = QProcess crash - * -2 = QProcess cannot start - * -3 = bad arguments - * -4 = QProcess timeout + * Crashed = QProcess crash + * FailedToStart = QProcess cannot start + * NoWorkingDirectory = bad arguments + * TimedOut = QProcess timeout */ static DLLEXPORT ProcessResult runCommand( RunLocation location, From 6055f08affb3e17085044fd14e7d157e558c0637 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 7 Jun 2019 12:30:38 +0200 Subject: [PATCH 03/10] [libcalamares] Refactor ProcessJob - Use the system runCommand() instead of a 90% copy of it. This **does** change the overall command to `env /bin/sh -c` rather than running only `/bin/sh -c`, though. --- src/libcalamares/ProcessJob.cpp | 86 +++++---------------------------- src/libcalamares/ProcessJob.h | 7 +-- 2 files changed, 14 insertions(+), 79 deletions(-) diff --git a/src/libcalamares/ProcessJob.cpp b/src/libcalamares/ProcessJob.cpp index 3cf4eec49..bfdf19860 100644 --- a/src/libcalamares/ProcessJob.cpp +++ b/src/libcalamares/ProcessJob.cpp @@ -67,83 +67,23 @@ ProcessJob::prettyStatusMessage() const JobResult ProcessJob::exec() { - int ec = 0; - QString output; + using CalamaresUtils::System; + if ( m_runInChroot ) - ec = CalamaresUtils::System::instance()-> - targetEnvOutput( m_command, - output, + return CalamaresUtils::System::instance()-> + targetEnvCommand( { m_command }, m_workingPath, QString(), - m_timeoutSec ); + m_timeoutSec ) + .explainProcess( m_command, m_timeoutSec ); else - ec = callOutput( m_command, - output, - m_workingPath, - QString(), - m_timeoutSec ); - - return CalamaresUtils::ProcessResult::explainProcess( ec, m_command, output, m_timeoutSec ); -} - - -int -ProcessJob::callOutput( const QString& command, - QString& output, - const QString& workingPath, - const QString& stdInput, - int timeoutSec ) -{ - output.clear(); - - QProcess process; - process.setProgram( "/bin/sh" ); - process.setArguments( { "-c", command } ); - process.setProcessChannelMode( QProcess::MergedChannels ); - - if ( !workingPath.isEmpty() ) - { - if ( QDir( workingPath ).exists() ) - process.setWorkingDirectory( QDir( workingPath ).absolutePath() ); - else - { - cWarning() << "Invalid working directory:" << workingPath; - return -3; - } - } - - cDebug() << "Running" << command; - process.start(); - if ( !process.waitForStarted() ) - { - cWarning() << "Process failed to start" << process.error(); - return -2; - } - - if ( !stdInput.isEmpty() ) - { - process.write( stdInput.toLocal8Bit() ); - process.closeWriteChannel(); - } - - if ( !process.waitForFinished( timeoutSec ? ( timeoutSec * 1000 ) : -1 ) ) - { - cWarning() << "Timed out. output so far:"; - output.append( QString::fromLocal8Bit( process.readAllStandardOutput() ).trimmed() ); - cWarning() << output; - return -4; - } - - output.append( QString::fromLocal8Bit( process.readAllStandardOutput() ).trimmed() ); - - if ( process.exitStatus() == QProcess::CrashExit ) - { - cWarning() << "Process crashed"; - return -1; - } - - cDebug() << "Finished. Exit code:" << process.exitCode(); - return process.exitCode(); + return + System::runCommand( System::RunLocation::RunInHost, + { "/bin/sh", "-c", m_command }, + m_workingPath, + QString(), + m_timeoutSec ) + .explainProcess( m_command, m_timeoutSec ); } } // namespace Calamares diff --git a/src/libcalamares/ProcessJob.h b/src/libcalamares/ProcessJob.h index d01dbb676..224ebdaf0 100644 --- a/src/libcalamares/ProcessJob.h +++ b/src/libcalamares/ProcessJob.h @@ -1,7 +1,7 @@ /* === This file is part of Calamares - === * * Copyright 2014, Teo Mrnjavac - * Copyright 2017-2018, Adriaan de Groot + * Copyright 2017-2019, Adriaan de Groot * * Calamares is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -40,11 +40,6 @@ public: JobResult exec() override; private: - int callOutput( const QString& command, - QString& output, - const QString& workingPath = QString(), - const QString& stdInput = QString(), - int timeoutSec = 0 ); QString m_command; QString m_workingPath; bool m_runInChroot; From b587d77e31a7e95589918cbcef6654176216eb92 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 7 Jun 2019 12:33:31 +0200 Subject: [PATCH 04/10] [libcalamares] Fix untranslatable string. - This would substitue an untranslated "in chroot" into the translated string, which is weird. --- src/libcalamares/ProcessJob.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libcalamares/ProcessJob.cpp b/src/libcalamares/ProcessJob.cpp index bfdf19860..47fcee05d 100644 --- a/src/libcalamares/ProcessJob.cpp +++ b/src/libcalamares/ProcessJob.cpp @@ -48,10 +48,8 @@ ProcessJob::~ProcessJob() QString ProcessJob::prettyName() const { - //TODO: show something more meaningful - return tr( "Run command %1 %2" ) - .arg( m_command ) - .arg( m_runInChroot ? "in chroot." : " ." ); + return ( m_runInChroot ? tr( "Run command '%1' in target system." ) : tr( " Run command '%1'." ) ) + .arg( m_command ); } From d7f513412168a8fc614c2c93609fb54e131b34e2 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 7 Jun 2019 13:32:31 +0200 Subject: [PATCH 05/10] [libcalamares] Be more verbose in error situations - runCommand can return NoWorkingDirectory in multiple places, make sure the log contains a more specific reason. --- src/libcalamares/utils/CalamaresUtilsSystem.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/libcalamares/utils/CalamaresUtilsSystem.cpp b/src/libcalamares/utils/CalamaresUtilsSystem.cpp index 0d227c88f..c74243ecc 100644 --- a/src/libcalamares/utils/CalamaresUtilsSystem.cpp +++ b/src/libcalamares/utils/CalamaresUtilsSystem.cpp @@ -114,14 +114,24 @@ System::mount( const QString& devicePath, const QString& options ) { if ( devicePath.isEmpty() || mountPoint.isEmpty() ) + { + if ( devicePath.isEmpty() ) + cWarning() << "Can't mount an empty device."; + if ( mountPoint.isEmpty() ) + cWarning() << "Can't mount on an empty mountpoint."; + return static_cast(ProcessResult::Code::NoWorkingDirectory); + } QDir mountPointDir( mountPoint ); if ( !mountPointDir.exists() ) { bool ok = mountPointDir.mkpath( mountPoint ); if ( !ok ) + { + cWarning() << "Could not create mountpoint" << mountPoint; return static_cast(ProcessResult::Code::NoWorkingDirectory); + } } QString program( "mount" ); @@ -147,7 +157,10 @@ System::runCommand( QString output; if ( !Calamares::JobQueue::instance() ) + { + cError() << "No JobQueue"; return ProcessResult::Code::NoWorkingDirectory; + } Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage(); if ( ( location == System::RunLocation::RunInTarget ) && From 07a59bd09cb3ec5c4737164d62f8a51cfcd4ec6e Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 7 Jun 2019 13:37:46 +0200 Subject: [PATCH 06/10] [libcalamares] All commands with workingDirectory failed - This is the same as EFAIL: a block is indented as if it's a multi- line else block. This isn't Python though, and the return always applies. - Add the necessary braces. - Apparently noone uses this code path (until ProcessJob was re- factored to do so). --- src/libcalamares/utils/CalamaresUtilsSystem.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/libcalamares/utils/CalamaresUtilsSystem.cpp b/src/libcalamares/utils/CalamaresUtilsSystem.cpp index c74243ecc..739249cd0 100644 --- a/src/libcalamares/utils/CalamaresUtilsSystem.cpp +++ b/src/libcalamares/utils/CalamaresUtilsSystem.cpp @@ -202,8 +202,10 @@ System::runCommand( if ( QDir( workingPath ).exists() ) process.setWorkingDirectory( QDir( workingPath ).absolutePath() ); else + { cWarning() << "Invalid working directory:" << workingPath; return ProcessResult::Code::NoWorkingDirectory; + } } cDebug() << "Running" << program << RedactedList( arguments ); From 5a835f32b8e9190dafc72b904f76ec7a474200dd Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 7 Jun 2019 13:56:16 +0200 Subject: [PATCH 07/10] [libcalamares] Start extending tests to runCommand() --- src/libcalamares/Tests.cpp | 21 +++++++++++++++++++++ src/libcalamares/Tests.h | 2 ++ 2 files changed, 23 insertions(+) diff --git a/src/libcalamares/Tests.cpp b/src/libcalamares/Tests.cpp index 615cb51a7..40cb480e9 100644 --- a/src/libcalamares/Tests.cpp +++ b/src/libcalamares/Tests.cpp @@ -18,9 +18,12 @@ #include "Tests.h" +#include "utils/CalamaresUtilsSystem.h" #include "utils/Logger.h" #include "utils/Yaml.h" +#include + #include QTEST_GUILESS_MAIN( LibCalamaresTests ) @@ -113,3 +116,21 @@ LibCalamaresTests::testLoadSaveYamlExtended() } QFile::remove( "out.yaml" ); } + +void +LibCalamaresTests::testCommands() +{ + using CalamaresUtils::System; + auto r = System::runCommand( + System::RunLocation::RunInHost, + { "/bin/ls", "/tmp" } + ); + + QVERIFY( r.getExitCode() == 0 ); + + QTemporaryFile tf( "/tmp/calamares-test-XXXXXX" ); + QVERIFY( tf.open() ); + QVERIFY( !tf.fileName().isEmpty() ); + + QVERIFY( r.getOutput().contains( tf.fileName() ) ); +} diff --git a/src/libcalamares/Tests.h b/src/libcalamares/Tests.h index 8d0aee1ff..5cdb3912b 100644 --- a/src/libcalamares/Tests.h +++ b/src/libcalamares/Tests.h @@ -34,6 +34,8 @@ private Q_SLOTS: void testLoadSaveYaml(); // Just settings.conf void testLoadSaveYamlExtended(); // Do a find() in the src dir + + void testCommands(); }; #endif From 7be33b8196758c72c023a77f30ce335e4d96b5d2 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 7 Jun 2019 15:53:27 +0200 Subject: [PATCH 08/10] [libcalamares] runCommand doesn't need queue or settings - JobQueue is only needed to get global settings, which are needed when running in the target; for host commands, allow running without a queue. - Settings is needed for the value of debugsettings; assume if there's no settings object, that we're in a test and should print debugging information. --- src/libcalamares/utils/CalamaresUtilsSystem.cpp | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/libcalamares/utils/CalamaresUtilsSystem.cpp b/src/libcalamares/utils/CalamaresUtilsSystem.cpp index 739249cd0..e2c6e9dec 100644 --- a/src/libcalamares/utils/CalamaresUtilsSystem.cpp +++ b/src/libcalamares/utils/CalamaresUtilsSystem.cpp @@ -156,13 +156,8 @@ System::runCommand( { QString output; - if ( !Calamares::JobQueue::instance() ) - { - cError() << "No JobQueue"; - return ProcessResult::Code::NoWorkingDirectory; - } + Calamares::GlobalStorage* gs = Calamares::JobQueue::instance() ? Calamares::JobQueue::instance()->globalStorage() : nullptr; - Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage(); if ( ( location == System::RunLocation::RunInTarget ) && ( !gs || !gs->contains( "rootMountPoint" ) ) ) { @@ -239,7 +234,8 @@ System::runCommand( auto r = process.exitCode(); cDebug() << "Finished. Exit code:" << r; - if ( ( r != 0 ) || Calamares::Settings::instance()->debugMode() ) + bool showDebug = ( !Calamares::Settings::instance() ) || ( Calamares::Settings::instance()->debugMode() ); + if ( ( r != 0 ) || showDebug ) { cDebug() << "Target cmd:" << RedactedList( args ); cDebug().noquote().nospace() << "Target output:\n" << output; From 4e13f780f188440c83d7b0c095aec05c93ea636c Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 7 Jun 2019 16:00:37 +0200 Subject: [PATCH 09/10] [libcalamares] Expand tests for runCommand - try both with and without a working-directory set, this would have shown up the problem with bad indentation much earlier. --- src/libcalamares/Tests.cpp | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/src/libcalamares/Tests.cpp b/src/libcalamares/Tests.cpp index 40cb480e9..3b7624537 100644 --- a/src/libcalamares/Tests.cpp +++ b/src/libcalamares/Tests.cpp @@ -132,5 +132,29 @@ LibCalamaresTests::testCommands() QVERIFY( tf.open() ); QVERIFY( !tf.fileName().isEmpty() ); - QVERIFY( r.getOutput().contains( tf.fileName() ) ); + QFileInfo tfn( tf.fileName() ); + QVERIFY( !r.getOutput().contains( tfn.fileName() ) ); + + // Run ls again, now that the file exists + r = System::runCommand( + System::RunLocation::RunInHost, + { "/bin/ls", "/tmp" } + ); + QVERIFY( r.getOutput().contains( tfn.fileName() ) ); + + // .. and without a working directory set, assume builddir != /tmp + r = System::runCommand( + System::RunLocation::RunInHost, + { "/bin/ls" } + ); + QVERIFY( !r.getOutput().contains( tfn.fileName() ) ); + + r = System::runCommand( + System::RunLocation::RunInHost, + { "/bin/ls" }, + "/tmp" + ); + QVERIFY( r.getOutput().contains( tfn.fileName() ) ); + + } From 4f221b41d196cc3527cedeabb77a9d1cc0fd1167 Mon Sep 17 00:00:00 2001 From: Adriaan de Groot Date: Fri, 7 Jun 2019 16:11:54 +0200 Subject: [PATCH 10/10] [libcalamares] Close stdin on process jobs - This avoids processes that wait on stdin, and e.g. improves reaction to having just "cat" (no file) in a command, or a package manager that asks for input. --- src/libcalamares/utils/CalamaresUtilsSystem.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcalamares/utils/CalamaresUtilsSystem.cpp b/src/libcalamares/utils/CalamaresUtilsSystem.cpp index e2c6e9dec..5990fbc42 100644 --- a/src/libcalamares/utils/CalamaresUtilsSystem.cpp +++ b/src/libcalamares/utils/CalamaresUtilsSystem.cpp @@ -214,8 +214,8 @@ System::runCommand( if ( !stdInput.isEmpty() ) { process.write( stdInput.toLocal8Bit() ); - process.closeWriteChannel(); } + process.closeWriteChannel(); if ( !process.waitForFinished( timeoutSec ? ( timeoutSec * 1000 ) : -1 ) ) {