Merge branch 'strictly-typed-timeout'

src/libcalamares/ProcessJob.cpp

@@ -31,7 +31,7 @@ namespace Calamares {
 ProcessJob::ProcessJob( const QString& command,
                         const QString& workingPath,
                         bool runInChroot,
-                        int secondsTimeout,
+                        std::chrono::seconds secondsTimeout,
                         QObject* parent )
     : Job( parent )
     , m_command( command )

src/libcalamares/ProcessJob.h

@@ -22,6 +22,8 @@
 
 #include "Job.h"
 
+#include <chrono>
+
 namespace Calamares {
 
 class ProcessJob : public Job
@@ -31,7 +33,7 @@ public:
     explicit ProcessJob( const QString& command,
                          const QString& workingPath,
                          bool runInChroot = false,
-                         int secondsTimeout = 30,
+                         std::chrono::seconds secondsTimeout = std::chrono::seconds(30),
                          QObject* parent = nullptr );
     virtual ~ProcessJob() override;
 
@@ -43,7 +45,7 @@ private:
     QString m_command;
     QString m_workingPath;
     bool m_runInChroot;
-    int m_timeoutSec;
+    std::chrono::seconds m_timeoutSec;
 };
 
 } // namespace Calamares

src/libcalamares/PythonJobApi.cpp

@@ -89,11 +89,13 @@ _target_env_command(
     const std::string& stdin,
     int timeout )
 {
+    // Since Python doesn't give us the type system for distinguishing
+    // seconds from other integral types, massage to seconds here.
     return CalamaresUtils::System::instance()->
         targetEnvCommand( args,
                           QString(),
                           QString::fromStdString( stdin ),
-                          timeout );
+                          std::chrono::seconds( timeout ) );
 }
 
 int

src/libcalamares/utils/CalamaresUtilsSystem.cpp

@@ -154,7 +154,7 @@ System::runCommand(
     const QStringList& args,
     const QString& workingPath,
     const QString& stdInput,
-    int timeoutSec )
+    std::chrono::seconds timeoutSec )
 {
     QString output;
 
@@ -219,7 +219,7 @@ System::runCommand(
     }
     process.closeWriteChannel();
 
-    if ( !process.waitForFinished( timeoutSec ? ( timeoutSec * 1000 ) : -1 ) )
+    if ( !process.waitForFinished( timeoutSec > std::chrono::seconds::zero() ? ( std::chrono::milliseconds( timeoutSec ).count() ) : -1 ) )
     {
         cWarning().noquote().nospace() << "Timed out. Output so far:\n" <<
             process.readAllStandardOutput();
@@ -371,7 +371,7 @@ System::doChroot() const
 }
 
 Calamares::JobResult
-ProcessResult::explainProcess( int ec, const QString& command, const QString& output, int timeout )
+ProcessResult::explainProcess( int ec, const QString& command, const QString& output, std::chrono::seconds timeout )
 {
     using Calamares::JobResult;
 
@@ -401,7 +401,7 @@ ProcessResult::explainProcess( int ec, const QString& command, const QString& ou
         return JobResult::error( QCoreApplication::translate( "ProcessResult", "External command failed to finish." ),
                                  QCoreApplication::translate( "ProcessResult", "Command <i>%1</i> failed to finish in %2 seconds." )
                                     .arg( command )
-                                    .arg( timeout )
+                                    .arg( timeout.count() )
                                     + outputMessage );
 
     //Any other exit code

src/libcalamares/utils/CalamaresUtilsSystem.h

@@ -27,6 +27,8 @@
 #include <QPair>
 #include <QString>
 
+#include <chrono>
+
 namespace CalamaresUtils
 {
 class ProcessResult : public QPair< int, QString >
@@ -60,16 +62,16 @@ public:
      * @param timeout   Timeout passed to the process runner, for explaining
      *                  error code -4 (timeout).
      */
-    static Calamares::JobResult explainProcess( int errorCode, const QString& command, const QString& output, int timeout );
+    static Calamares::JobResult explainProcess( int errorCode, const QString& command, const QString& output, std::chrono::seconds timeout );
 
     /// @brief Convenience wrapper for explainProcess()
-    inline Calamares::JobResult explainProcess( const QString& command, int timeout ) const
+    inline Calamares::JobResult explainProcess( const QString& command, std::chrono::seconds timeout ) const
     {
         return explainProcess( getExitCode(), command, getOutput(), timeout );
     }
 
     /// @brief Convenience wrapper for explainProcess()
-    inline Calamares::JobResult explainProcess( const QStringList& command, int timeout ) const
+    inline Calamares::JobResult explainProcess( const QStringList& command, std::chrono::seconds timeout ) const
     {
         return explainProcess( getExitCode(), command.join( ' ' ), getOutput(), timeout );
     }
@@ -138,7 +140,7 @@ public:
         const QStringList &args,
         const QString& workingPath = QString(),
         const QString& stdInput = QString(),
-        int timeoutSec = 0 );
+        std::chrono::seconds timeoutSec = std::chrono::seconds(0) );
 
     /** @brief Convenience wrapper for runCommand().
      *  Runs the command in the location specified through the boolean
@@ -149,7 +151,7 @@ public:
         const QStringList &args,
         const QString& workingPath = QString(),
         const QString& stdInput = QString(),
-        int timeoutSec = 0 )
+        std::chrono::seconds timeoutSec = std::chrono::seconds(0) )
     {
         return runCommand(
             m_doChroot ? RunLocation::RunInTarget : RunLocation::RunInHost,
@@ -163,7 +165,7 @@ public:
     inline int targetEnvCall( const QStringList& args,
                               const QString& workingPath = QString(),
                               const QString& stdInput = QString(),
-                              int timeoutSec = 0 )
+                              std::chrono::seconds timeoutSec = std::chrono::seconds(0) )
     {
         return targetEnvCommand( args, workingPath, stdInput, timeoutSec ).first;
     }
@@ -172,7 +174,7 @@ public:
     inline int targetEnvCall( const QString& command,
                               const QString& workingPath = QString(),
                               const QString& stdInput = QString(),
-                              int timeoutSec = 0 )
+                              std::chrono::seconds timeoutSec = std::chrono::seconds(0) )
     {
         return targetEnvCall( QStringList{ command }, workingPath, stdInput, timeoutSec );
     }
@@ -185,7 +187,7 @@ public:
                                 QString& output,
                                 const QString& workingPath = QString(),
                                 const QString& stdInput = QString(),
-                                int timeoutSec = 0 )
+                                std::chrono::seconds timeoutSec = std::chrono::seconds(0) )
     {
         auto r = targetEnvCommand( args, workingPath, stdInput, timeoutSec );
         output = r.second;
@@ -200,7 +202,7 @@ public:
                                 QString& output,
                                 const QString& workingPath = QString(),
                                 const QString& stdInput = QString(),
-                                int timeoutSec = 0 )
+                                std::chrono::seconds timeoutSec = std::chrono::seconds(0) )
     {
         return targetEnvOutput( QStringList{ command }, output, workingPath, stdInput, timeoutSec );
     }

src/libcalamares/utils/CommandList.cpp

@@ -35,10 +35,10 @@ namespace CalamaresUtils
 static CommandLine get_variant_object( const QVariantMap& m )
 {
     QString command = CalamaresUtils::getString( m, "command" );
-    int timeout = CalamaresUtils::getInteger( m, "timeout", CommandLine::TimeoutNotSet );
+    int timeout = CalamaresUtils::getInteger( m, "timeout", -1 );
 
     if ( !command.isEmpty() )
-        return CommandLine( command, timeout );
+        return CommandLine( command, timeout >= 0 ? std::chrono::seconds( timeout ) : CommandLine::TimeoutNotSet() );
     cWarning() << "Bad CommandLine element" << m;
     return CommandLine();
 }
@@ -50,7 +50,7 @@ static CommandList_t get_variant_stringlist( const QVariantList& l )
     for ( const auto& v : l )
     {
         if ( v.type() == QVariant::String )
-            retl.append( CommandLine( v.toString(), CommandLine::TimeoutNotSet ) );
+            retl.append( CommandLine( v.toString(), CommandLine::TimeoutNotSet() ) );
         else if ( v.type() == QVariant::Map )
         {
             auto command( get_variant_object( v.toMap() ) );
@@ -65,13 +65,13 @@ static CommandList_t get_variant_stringlist( const QVariantList& l )
     return retl;
 }
 
-CommandList::CommandList( bool doChroot, int timeout )
+CommandList::CommandList( bool doChroot, std::chrono::seconds timeout )
     : m_doChroot( doChroot )
     , m_timeout( timeout )
 {
 }
 
-CommandList::CommandList::CommandList( const QVariant& v, bool doChroot, int timeout )
+CommandList::CommandList::CommandList( const QVariant& v, bool doChroot, std::chrono::seconds timeout )
     : CommandList( doChroot, timeout )
 {
     if ( v.type() == QVariant::List )
@@ -155,7 +155,7 @@ Calamares::JobResult CommandList::run()
         QStringList shell_cmd { "/bin/sh", "-c" };
         shell_cmd << processed_cmd;
 
-        int timeout = i->timeout() >= 0 ? i->timeout() : m_timeout;
+        std::chrono::seconds timeout = i->timeout() >= std::chrono::seconds::zero() ? i->timeout() : m_timeout;
         ProcessResult r = System::runCommand(
                               location, shell_cmd, QString(), QString(), timeout );
 

src/libcalamares/utils/CommandList.h

@@ -24,6 +24,8 @@
 #include <QStringList>
 #include <QVariant>
 
+#include <chrono>
+
 namespace CalamaresUtils
 {
 
@@ -31,23 +33,23 @@ namespace CalamaresUtils
  * Each command can have an associated timeout in seconds. The timeout
  * defaults to 10 seconds. Provide some convenience naming and construction.
  */
-struct CommandLine : public QPair< QString, int >
+struct CommandLine : public QPair< QString, std::chrono::seconds >
 {
-    enum { TimeoutNotSet = -1 };
+    static inline constexpr std::chrono::seconds TimeoutNotSet() { return std::chrono::seconds(-1); }
 
     /// An invalid command line
     CommandLine()
-        : QPair< QString, int >( QString(), TimeoutNotSet )
+        : QPair( QString(), TimeoutNotSet() )
     {
     }
 
     CommandLine( const QString& s )
-        : QPair< QString, int >( s, TimeoutNotSet )
+        : QPair( s, TimeoutNotSet() )
     {
     }
 
-    CommandLine( const QString& s, int t )
+    CommandLine( const QString& s, std::chrono::seconds t )
-        : QPair< QString, int >( s, t)
+        : QPair( s, t)
     {
     }
 
@@ -56,7 +58,7 @@ struct CommandLine : public QPair< QString, int >
         return first;
     }
 
-    int timeout() const
+    std::chrono::seconds timeout() const
     {
         return second;
     }
@@ -82,8 +84,8 @@ class CommandList : protected CommandList_t
 {
 public:
     /** @brief empty command-list with timeout to apply to entries. */
-    CommandList( bool doChroot = true, int timeout = 10 );
+    CommandList( bool doChroot = true, std::chrono::seconds timeout = std::chrono::seconds( 10 ) );
-    CommandList( const QVariant& v, bool doChroot = true, int timeout = 10 );
+    CommandList( const QVariant& v, bool doChroot = true, std::chrono::seconds timeout = std::chrono::seconds( 10 ) );
     ~CommandList();
 
     bool doChroot() const
@@ -106,7 +108,7 @@ protected:
 
 private:
     bool m_doChroot;
-    int m_timeout;
+    std::chrono::seconds m_timeout;
 } ;
 
 }  // namespace

src/libcalamaresui/modulesystem/ProcessJobModule.cpp

@@ -72,10 +72,13 @@ ProcessJobModule::initFrom( const QVariantMap& moduleDescriptor )
         m_command = moduleDescriptor.value( "command" ).toString();
     }
 
-    m_secondsTimeout = 30;
+    m_secondsTimeout = std::chrono::seconds( 30 );
     if ( moduleDescriptor.contains( "timeout" ) && !moduleDescriptor.value( "timeout" ).isNull() )
     {
-        m_secondsTimeout = moduleDescriptor.value( "timeout" ).toInt();
+        int sec = moduleDescriptor.value( "timeout" ).toInt();
+        if ( sec < 0 )
+            sec = 0;
+        m_secondsTimeout = std::chrono::seconds( sec );
     }
 
     m_runInChroot = false;
@@ -88,7 +91,7 @@ ProcessJobModule::initFrom( const QVariantMap& moduleDescriptor )
 
 ProcessJobModule::ProcessJobModule()
     : Module()
-    , m_secondsTimeout( 30 )
+    , m_secondsTimeout( std::chrono::seconds( 30 ) )
     , m_runInChroot( false )
 {
 }

src/libcalamaresui/modulesystem/ProcessJobModule.h

@@ -24,6 +24,8 @@
 
 #include "UiDllMacro.h"
 
+#include <chrono>
+
 namespace Calamares
 {
 
@@ -46,7 +48,7 @@ private:
 
     QString m_command;
     QString m_workingPath;
-    int m_secondsTimeout;
+    std::chrono::seconds m_secondsTimeout;
     bool m_runInChroot;
     job_ptr m_job;
 };

src/modules/contextualprocess/ContextualProcessJob.cpp

@@ -170,7 +170,7 @@ ContextualProcessJob::setConfigurationMap( const QVariantMap& configurationMap )
                 continue;
             }
 
-            CalamaresUtils::CommandList* commands = new CalamaresUtils::CommandList( valueiter.value(), !dontChroot, timeout );
+            CalamaresUtils::CommandList* commands = new CalamaresUtils::CommandList( valueiter.value(), !dontChroot, std::chrono::seconds( timeout ) );
 
             binding->append( valueString, commands );
         }

src/modules/initcpio/InitcpioJob.cpp

@@ -74,8 +74,8 @@ InitcpioJob::exec()
 
     cDebug() << "Updating initramfs with kernel" << m_kernel;
     auto r = CalamaresUtils::System::instance()->targetEnvCommand(
-        { "mkinitcpio", "-p", m_kernel }, QString(), QString(), 0 );
+        { "mkinitcpio", "-p", m_kernel }, QString(), QString() /* no timeout , 0 */ );
-    return r.explainProcess( "mkinitcpio", 10 );
+    return r.explainProcess( "mkinitcpio", std::chrono::seconds( 10 ) /* fake timeout */ );
 }
 
 void
@@ -89,7 +89,7 @@ InitcpioJob::setConfigurationMap( const QVariantMap& configurationMap )
     else if ( m_kernel == "$uname" )
     {
         auto r = CalamaresUtils::System::runCommand(
-            CalamaresUtils::System::RunLocation::RunInHost, { "/bin/uname", "-r" }, QString(), QString(), 3 );
+            CalamaresUtils::System::RunLocation::RunInHost, { "/bin/uname", "-r" }, QString(), QString(), std::chrono::seconds( 3 ) );
         if ( r.getExitCode() == 0 )
         {
             m_kernel = r.getOutput();

src/modules/initramfs/InitramfsJob.cpp

@@ -63,8 +63,8 @@ InitramfsJob::exec()
 
     // And then do the ACTUAL work.
     auto r = CalamaresUtils::System::instance()->targetEnvCommand(
-        { "update-initramfs", "-k", m_kernel, "-c", "-t" }, QString(), QString(), 0 );
+        { "update-initramfs", "-k", m_kernel, "-c", "-t" }, QString(), QString() /* no timeout, 0 */ );
-    return r.explainProcess( "update-initramfs", 10 );
+    return r.explainProcess( "update-initramfs", std::chrono::seconds( 10 ) /* fake timeout */ );
 }
 
 
@@ -79,7 +79,7 @@ InitramfsJob::setConfigurationMap( const QVariantMap& configurationMap )
     else if ( m_kernel == "$uname" )
     {
         auto r = CalamaresUtils::System::runCommand(
-            CalamaresUtils::System::RunLocation::RunInHost, { "/bin/uname", "-r" }, QString(), QString(), 3 );
+            CalamaresUtils::System::RunLocation::RunInHost, { "/bin/uname", "-r" }, QString(), QString(), std::chrono::seconds( 3 ) );
         if ( r.getExitCode() == 0 )
         {
             m_kernel = r.getOutput();

src/modules/luksbootkeyfile/LuksBootKeyFileJob.cpp

@@ -130,7 +130,7 @@ static bool
 setupLuks( const LuksDevice& d )
 {
     auto r = CalamaresUtils::System::instance()->targetEnvCommand(
-        { "cryptsetup", "luksAddKey", d.device, keyfile }, QString(), d.passphrase, 15 );
+        { "cryptsetup", "luksAddKey", d.device, keyfile }, QString(), d.passphrase, std::chrono::seconds( 15 ) );
     if ( r.getExitCode() != 0 )
     {
         cWarning() << "Could not configure LUKS keyfile on" << d.device << ':' << r.getOutput() << "(exit code"

src/modules/shellprocess/ShellProcessJob.cpp

@@ -75,7 +75,7 @@ ShellProcessJob::setConfigurationMap( const QVariantMap& configurationMap )
 
     if ( configurationMap.contains( "script" ) )
     {
-        m_commands = new CalamaresUtils::CommandList( configurationMap.value( "script" ), !dontChroot, timeout );
+        m_commands = new CalamaresUtils::CommandList( configurationMap.value( "script" ), !dontChroot, std::chrono::seconds( timeout ) );
         if ( m_commands->isEmpty() )
             cDebug() << "ShellProcessJob: \"script\" contains no commands for" << moduleInstanceKey();
     }

src/modules/shellprocess/Tests.cpp

@@ -34,6 +34,8 @@
 QTEST_GUILESS_MAIN( ShellProcessTests )
 
 using CommandList = CalamaresUtils::CommandList;
+using std::operator""s;
+
 
 ShellProcessTests::ShellProcessTests()
 {
@@ -68,8 +70,9 @@ ShellProcessTests::testProcessListSampleConfig()
         CalamaresUtils::yamlMapToVariant( doc ).toMap().value( "script" ) );
     QVERIFY( !cl.isEmpty() );
     QCOMPARE( cl.count(), 3 );
-    QCOMPARE( cl.at(0).timeout(), -1 );
+
-    QCOMPARE( cl.at(2).timeout(), 3600 );  // slowloris
+    QCOMPARE( cl.at(0).timeout(), CalamaresUtils::CommandLine::TimeoutNotSet() );
+    QCOMPARE( cl.at(2).timeout(), 3600s );  // slowloris
 }
 
 void ShellProcessTests::testProcessListFromList()
@@ -105,9 +108,10 @@ script: "ls /tmp"
 )" );
     CommandList cl(
         CalamaresUtils::yamlMapToVariant( doc ).toMap().value( "script" ) );
+
     QVERIFY( !cl.isEmpty() );
     QCOMPARE( cl.count(), 1 );
-    QCOMPARE( cl.at(0).timeout(), 10 );
+    QCOMPARE( cl.at(0).timeout(), 10s );
     QCOMPARE( cl.at(0).command(), QStringLiteral( "ls /tmp" ) );
 
     // Not a string
@@ -118,7 +122,6 @@ script: false
         CalamaresUtils::yamlMapToVariant( doc ).toMap().value( "script" ) );
     QVERIFY( cl1.isEmpty() );
     QCOMPARE( cl1.count(), 0 );
-
 }
 
 void ShellProcessTests::testProcessFromObject()
@@ -130,9 +133,10 @@ script:
 )" );
     CommandList cl(
         CalamaresUtils::yamlMapToVariant( doc ).toMap().value( "script" ) );
+
     QVERIFY( !cl.isEmpty() );
     QCOMPARE( cl.count(), 1 );
-    QCOMPARE( cl.at(0).timeout(), 20 );
+    QCOMPARE( cl.at(0).timeout(), 20s );
     QCOMPARE( cl.at(0).command(), QStringLiteral( "ls /tmp" ) );
 }
 
@@ -148,9 +152,9 @@ script:
         CalamaresUtils::yamlMapToVariant( doc ).toMap().value( "script" ) );
     QVERIFY( !cl.isEmpty() );
     QCOMPARE( cl.count(), 2 );
-    QCOMPARE( cl.at(0).timeout(), 12 );
+    QCOMPARE( cl.at(0).timeout(), 12s );
     QCOMPARE( cl.at(0).command(), QStringLiteral( "ls /tmp" ) );
-    QCOMPARE( cl.at(1).timeout(), -1 );  // not set
+    QCOMPARE( cl.at(1).timeout(), CalamaresUtils::CommandLine::TimeoutNotSet() );  // not set
 }
 
 void ShellProcessTests::testRootSubstitution()
@@ -182,30 +186,31 @@ script:
     QVERIFY( gs != nullptr );
 
     qDebug() << "Expect WARNING, ERROR, WARNING";
+
     // Doesn't use @@ROOT@@, so no failures
-    QVERIFY( bool(CommandList(plainScript, false, 10 ).run()) );
+    QVERIFY( bool(CommandList(plainScript, false, 10s ).run()) );
 
     // Doesn't use @@ROOT@@, but does chroot, so fails
-    QVERIFY( !bool(CommandList(plainScript, true, 10 ).run()) );
+    QVERIFY( !bool(CommandList(plainScript, true, 10s ).run()) );
 
     // Does use @@ROOT@@, which is not set, so fails
-    QVERIFY( !bool(CommandList(rootScript, false, 10 ).run()) );
+    QVERIFY( !bool(CommandList(rootScript, false, 10s ).run()) );
     // .. fails for two reasons
-    QVERIFY( !bool(CommandList(rootScript, true, 10 ).run()) );
+    QVERIFY( !bool(CommandList(rootScript, true, 10s ).run()) );
 
     gs->insert( "rootMountPoint", "/tmp" );
     // Now that the root is set, two variants work .. still can't
     // chroot, unless the rootMountPoint contains a full system,
     // *and* we're allowed to chroot (ie. running tests as root).
     qDebug() << "Expect no output.";
-    QVERIFY( bool(CommandList(plainScript, false, 10 ).run()) );
+    QVERIFY( bool(CommandList(plainScript, false, 10s ).run()) );
-    QVERIFY( bool(CommandList(rootScript, false, 10 ).run()) );
+    QVERIFY( bool(CommandList(rootScript, false, 10s ).run()) );
 
     qDebug() << "Expect ERROR";
     // But no user set yet
-    QVERIFY( !bool(CommandList(userScript, false, 10 ).run()) );
+    QVERIFY( !bool(CommandList(userScript, false, 10s ).run()) );
 
     // Now play dangerous games with shell expansion
     gs->insert( "username", "`id -u`" );
-    QVERIFY( bool(CommandList(userScript, false, 10 ).run()) );
+    QVERIFY( bool(CommandList(userScript, false, 10s ).run()) );
 }

src/modules/tracking/TrackingJobs.cpp

@@ -125,7 +125,7 @@ sed -i "s,URI =.*,URI = http://releases.neon.kde.org/meta-release/${MACHINE_ID},
 sed -i "s,URI_LTS =.*,URI_LTS = http://releases.neon.kde.org/meta-release-lts/${MACHINE_ID}," /etc/update-manager/meta-release
 true
 )x"),
-        1);
+        std::chrono::seconds( 1 ) );
 
     if ( r == 0 )
         return Calamares::JobResult::ok();

src/modules/users/CreateUserJob.cpp

@@ -158,7 +158,7 @@ CreateUserJob::exec()
     if ( commandResult.getExitCode() )
     {
         cError() << "useradd failed" << commandResult.getExitCode();
-        return commandResult.explainProcess( useradd, 10 /* bogus timeout */ );
+        return commandResult.explainProcess( useradd, std::chrono::seconds( 10 ) /* bogus timeout */ );
     }
 
     commandResult = CalamaresUtils::System::instance()->targetEnvCommand(
@@ -166,7 +166,7 @@ CreateUserJob::exec()
     if ( commandResult.getExitCode() )
     {
         cError() << "usermod failed" << commandResult.getExitCode();
-        return commandResult.explainProcess( "usermod", 10 );
+        return commandResult.explainProcess( "usermod", std::chrono::seconds( 10 ) /* bogus timeout */ );
     }
 
     QString userGroup = QString( "%1:%2" ).arg( m_userName ).arg( m_userName );
@@ -176,7 +176,7 @@ CreateUserJob::exec()
     if ( commandResult.getExitCode() )
     {
         cError() << "chown failed" << commandResult.getExitCode();
-        return commandResult.explainProcess( "chown", 10 );
+        return commandResult.explainProcess( "chown", std::chrono::seconds( 10 ) /* bogus timeout */ );
     }
 
     return Calamares::JobResult::ok();