Merge branch 'i897-timeout'

FIXES #897

src/libcalamares/utils/CalamaresUtils.cpp

@@ -366,6 +366,22 @@ getInteger( const QVariantMap& map, const QString& key, int d )
     return result;
 }
 
+double
+getDouble( const QVariantMap& map, const QString& key, double d )
+{
+    double result = d;
+    if ( map.contains( key ) )
+    {
+        auto v = map.value( key );
+        if ( v.type() == QVariant::Int )
+            result = v.toInt();
+        else if ( v.type() == QVariant::Double )
+            result = v.toDouble();
+    }
+
+    return result;
+}
+
 QVariantMap
 getSubMap( const QVariantMap& map, const QString& key, bool& success )
 {

src/libcalamares/utils/CalamaresUtils.h

@@ -110,10 +110,15 @@ namespace CalamaresUtils
     DLLEXPORT QString getString( const QVariantMap& map, const QString& key );
 
     /**
-     * Get an integer value from a mapping; returns @p default if no value.
+     * Get an integer value from a mapping; returns @p d if no value.
      */
     DLLEXPORT int getInteger( const QVariantMap& map, const QString& key, int d );
 
+    /**
+     * Get a double value from a mapping (integers are converted); returns @p d if no value.
+     */
+    DLLEXPORT double getDouble( const QVariantMap& map, const QString& key, double d );
+
     /**
      * Returns a sub-map (i.e. a nested map) from the given mapping with the
      * given key. @p success is set to true if the @p key exists

src/libcalamares/utils/CommandList.cpp

@@ -21,19 +21,41 @@
 #include "GlobalStorage.h"
 #include "JobQueue.h"
 
+#include "utils/CalamaresUtils.h"
 #include "utils/CalamaresUtilsSystem.h"
 #include "utils/Logger.h"
 
 #include <QVariantList>
 
-static QStringList get_variant_stringlist( const QVariantList& l )
+namespace CalamaresUtils
+{
+
+static CommandLine get_variant_object( const QVariantMap& m )
 {
-    QStringList retl;
+    QString command = CalamaresUtils::getString( m, "command" );
+    int timeout = CalamaresUtils::getInteger( m, "timeout", CommandLine::TimeoutNotSet );
+
+    if ( !command.isEmpty() )
+        return CommandLine( command, timeout );
+    cDebug() << "WARNING Bad CommandLine element" << m;
+    return CommandLine();
+}
+
+static CommandList_t get_variant_stringlist( const QVariantList& l )
+{
+    CommandList_t retl;
     unsigned int c = 0;
     for ( const auto& v : l )
     {
         if ( v.type() == QVariant::String )
-            retl.append( v.toString() );
+            retl.append( CommandLine( v.toString(), CommandLine::TimeoutNotSet ) );
+        else if ( v.type() == QVariant::Map )
+        {
+            auto c( get_variant_object( v.toMap() ) );
+            if ( c.isValid() )
+                retl.append( c );
+            // Otherwise warning is already given
+        }
         else
             cDebug() << "WARNING Bad CommandList element" << c << v.type() << v;
         ++c;
@@ -41,16 +63,14 @@ static QStringList get_variant_stringlist( const QVariantList& l )
     return retl;
 }
 
-namespace CalamaresUtils
-{
-
-CommandList::CommandList( bool doChroot )
+CommandList::CommandList( bool doChroot, int timeout )
     : m_doChroot( doChroot )
+    , m_timeout( timeout )
 {
 }
 
-CommandList::CommandList::CommandList( const QVariant& v, bool doChroot )
-    : CommandList( doChroot )
+CommandList::CommandList::CommandList( const QVariant& v, bool doChroot, int timeout )
+    : CommandList( doChroot, timeout )
 {
     if ( v.type() == QVariant::List )
     {
@@ -62,6 +82,13 @@ CommandList::CommandList::CommandList( const QVariant& v, bool doChroot )
     }
     else if ( v.type() == QVariant::String )
         append( v.toString() );
+    else if ( v.type() == QVariant::Map )
+    {
+        auto c( get_variant_object( v.toMap() ) );
+        if ( c.isValid() )
+            append( c );
+        // Otherwise warning is already given
+    }
     else
         cDebug() << "WARNING: CommandList does not understand variant" << v.type();
 }
@@ -90,31 +117,38 @@ Calamares::JobResult CommandList::run( const QObject* parent )
 
     for ( CommandList::const_iterator i = cbegin(); i != cend(); ++i )
     {
-        QString processed_cmd = *i;
-        processed_cmd.replace( "@@ROOT@@", root );  // FIXME?
+        QString processed_cmd = i->command();
+        processed_cmd.replace( "@@ROOT@@", root );
         bool suppress_result = false;
         if ( processed_cmd.startsWith( '-' ) )
         {
             suppress_result = true;
-            processed_cmd.remove( 0, 1 );  // Drop the -  // FIXME?
+            processed_cmd.remove( 0, 1 );  // Drop the -
         }
 
         QStringList shell_cmd { "/bin/sh", "-c" };
         shell_cmd << processed_cmd;
 
+        int timeout = i->timeout() >= 0 ? i->timeout() : m_timeout;
         ProcessResult r = System::runCommand(
-                              location, shell_cmd, QString(), QString(), 10 );
+                              location, shell_cmd, QString(), QString(), timeout );
 
         if ( r.getExitCode() != 0 )
         {
             if ( suppress_result )
                 cDebug() << "Error code" << r.getExitCode() << "ignored by CommandList configuration.";
             else
-                return r.explainProcess( parent, processed_cmd, 10 );
+                return r.explainProcess( parent, processed_cmd, timeout );
         }
     }
 
     return Calamares::JobResult::ok();
 }
 
+void
+CommandList::append( const QString& s )
+{
+    append( CommandLine( s, m_timeout ) );
+}
+
 }  // namespace

src/libcalamares/utils/CommandList.h

@@ -27,11 +27,60 @@
 namespace CalamaresUtils
 {
 
-class CommandList : protected QStringList
+/**
+ * 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 >
+{
+    enum { TimeoutNotSet = -1 };
+
+    /// An invalid command line
+    CommandLine()
+        : QPair< QString, int >( QString(), TimeoutNotSet )
+    {
+    }
+
+    CommandLine( const QString& s )
+        : QPair< QString, int >( s, TimeoutNotSet )
+    {
+    }
+
+    CommandLine( const QString& s, int t )
+        : QPair< QString, int >( s, t)
+    {
+    }
+
+    QString command() const
+    {
+        return first;
+    }
+
+    int timeout() const
+    {
+        return second;
+    }
+
+    bool isValid() const
+    {
+        return !first.isEmpty();
+    }
+} ;
+
+/** @brief Abbreviation, used internally. */
+using CommandList_t = QList< CommandLine >;
+
+/**
+ * A list of commands; the list may have its own default timeout
+ * for commands (which is then applied to each individual command
+ * that doesn't have one of its own).
+ */
+class CommandList : protected CommandList_t
 {
 public:
-    CommandList( bool doChroot = true );
-    CommandList( const QVariant& v, bool doChroot = true );
+    /** @brief empty command-list with timeout to apply to entries. */
+    CommandList( bool doChroot = true, int timeout = 10 );
+    CommandList( const QVariant& v, bool doChroot = true, int timeout = 10 );
     ~CommandList();
 
     bool doChroot() const
@@ -41,14 +90,20 @@ public:
 
     Calamares::JobResult run( const QObject* parent );
 
-    using QStringList::isEmpty;
-    using QStringList::count;
-    using QStringList::cbegin;
-    using QStringList::cend;
-    using QStringList::const_iterator;
+    using CommandList_t::isEmpty;
+    using CommandList_t::count;
+    using CommandList_t::cbegin;
+    using CommandList_t::cend;
+    using CommandList_t::const_iterator;
+    using CommandList_t::at;
+
+protected:
+    using CommandList_t::append;
+    void append( const QString& );
 
 private:
     bool m_doChroot;
+    int m_timeout;
 } ;
 
 }  // namespace

src/modules/contextualprocess/ContextualProcessJob.cpp

@@ -97,12 +97,15 @@ ContextualProcessJob::exec()
 void
 ContextualProcessJob::setConfigurationMap( const QVariantMap& configurationMap )
 {
-    m_dontChroot = CalamaresUtils::getBool( configurationMap, "dontChroot", false );
+    bool dontChroot = CalamaresUtils::getBool( configurationMap, "dontChroot", false );
+    int timeout = CalamaresUtils::getInteger( configurationMap, "timeout", 10 );
+    if ( timeout < 1 )
+        timeout = 10;
 
     for ( QVariantMap::const_iterator iter = configurationMap.cbegin(); iter != configurationMap.cend(); ++iter )
     {
         QString variableName = iter.key();
-        if ( variableName.isEmpty() || ( variableName == "dontChroot" ) )
+        if ( variableName.isEmpty() || ( variableName == "dontChroot" ) || ( variableName == "timeout" ) )
             continue;
 
         if ( iter.value().type() != QVariant::Map )
@@ -121,7 +124,7 @@ ContextualProcessJob::setConfigurationMap( const QVariantMap& configurationMap )
                 continue;
             }
 
-            CalamaresUtils::CommandList* commands = new CalamaresUtils::CommandList( valueiter.value(), !m_dontChroot );
+            CalamaresUtils::CommandList* commands = new CalamaresUtils::CommandList( valueiter.value(), !dontChroot, timeout );
 
             if ( commands->count() > 0 )
             {

src/modules/contextualprocess/ContextualProcessJob.h

@@ -45,7 +45,6 @@ public:
 
 private:
     QList<ContextualProcessBinding*> m_commands;
-    bool m_dontChroot;
 };
 
 CALAMARES_PLUGIN_FACTORY_DECLARATION( ContextualProcessJobFactory )

src/modules/contextualprocess/contextualprocess.conf

@@ -4,22 +4,16 @@
 # When a given global value (string) equals a given value, then
 # the associated command is executed.
 #
-# Configuration consists of keys for global variable names,
-# and the sub-keys are strings to compare to the variable's value.
-# If the variable has that particular value, the corresponding
-# value is executed as a shell command in the target environment.
+# The special top-level keys *dontChroot* and *timeout* have
+# meaning just like in shellprocess.conf. They are excluded from
+# the comparison with global variables.
 #
-# You can check for an empty value with "".
-#
-# The special configuration key *dontChroot* specifies whether
-# the commands are run in the target system (default, value *false*),
-# or in the host system. This key is not used for comparisons
-# with global configuration values.
+# Configuration consists of keys for global variable names (except
+# *dontChroot* and *timeout*), and the sub-keys are strings to compare
+# to the variable's value. If the variable has that particular value, the
+# corresponding value (script) is executed.
 #
-# If a command starts with "-" (a single minus sign), then the
-# return value of the command following the - is ignored; otherwise,
-# a failing command will abort the installation. This is much like
-# make's use of - in a command.
+# You can check for an empty value with "".
 #
 # Global configuration variables are not checked in a deterministic
 # order, so do not rely on commands from one variable-check to
@@ -28,9 +22,15 @@
 # done in a deterministic order, but all of the value-checks
 # for a given variable happen together.
 #
+# The values after a value sub-keys are the same kinds of values
+# as can be given to the *script* key in the shellprocess module.
+# See shellprocess.conf for documentation on valid values.
 ---
 dontChroot: false
 firmwareType:
-    efi:    "-pkg remove efi-firmware"
+    efi:
+        -   "-pkg remove efi-firmware"
+        -   command: "-mkinitramfsrd -abgn"
+            timeout: 120  # This is slow
     bios:   "-pkg remove bios-firmware"
     "":     "/bin/false no-firmware-type-set"

src/modules/shellprocess/ShellProcessJob.cpp

@@ -34,7 +34,6 @@
 ShellProcessJob::ShellProcessJob( QObject* parent )
     : Calamares::CppJob( parent )
     , m_commands( nullptr )
-    , m_dontChroot( false )
 {
 }
 
@@ -70,11 +69,14 @@ ShellProcessJob::exec()
 void
 ShellProcessJob::setConfigurationMap( const QVariantMap& configurationMap )
 {
-    m_dontChroot = CalamaresUtils::getBool( configurationMap, "dontChroot", false );
+    bool dontChroot = CalamaresUtils::getBool( configurationMap, "dontChroot", false );
+    int timeout = CalamaresUtils::getInteger( configurationMap, "timeout", 10 );
+    if ( timeout < 1 )
+        timeout = 10;
 
     if ( configurationMap.contains( "script" ) )
     {
-        m_commands = new CalamaresUtils::CommandList( configurationMap.value( "script" ), !m_dontChroot );
+        m_commands = new CalamaresUtils::CommandList( configurationMap.value( "script" ), !dontChroot, timeout );
         if ( m_commands->isEmpty() )
             cDebug() << "ShellProcessJob: \"script\" contains no commands for" << moduleInstanceKey();
     }

src/modules/shellprocess/ShellProcessJob.h

@@ -46,7 +46,6 @@ public:
 
 private:
     CalamaresUtils::CommandList* m_commands;
-    bool m_dontChroot;
 };
 
 CALAMARES_PLUGIN_FACTORY_DECLARATION( ShellProcessJobFactory )

src/modules/shellprocess/Tests.cpp

@@ -64,7 +64,9 @@ ShellProcessTests::testProcessListSampleConfig()
     CommandList cl(
         CalamaresUtils::yamlMapToVariant( doc ).toMap().value( "script" ) );
     QVERIFY( !cl.isEmpty() );
-    QCOMPARE( cl.count(), 2 );
+    QCOMPARE( cl.count(), 3 );
+    QCOMPARE( cl.at(0).timeout(), -1 );
+    QCOMPARE( cl.at(2).timeout(), 3600 );  // slowloris
 }
 
 void ShellProcessTests::testProcessListFromList()
@@ -102,6 +104,8 @@ script: "ls /tmp"
         CalamaresUtils::yamlMapToVariant( doc ).toMap().value( "script" ) );
     QVERIFY( !cl.isEmpty() );
     QCOMPARE( cl.count(), 1 );
+    QCOMPARE( cl.at(0).timeout(), 10 );
+    QCOMPARE( cl.at(0).command(), QStringLiteral( "ls /tmp" ) );
 
     // Not a string
     doc = YAML::Load( R"(---
@@ -113,3 +117,35 @@ script: false
     QCOMPARE( cl1.count(), 0 );
 
 }
+
+void ShellProcessTests::testProcessFromObject()
+{
+    YAML::Node doc = YAML::Load( R"(---
+script:
+    command: "ls /tmp"
+    timeout: 20
+)" );
+    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).command(), QStringLiteral( "ls /tmp" ) );
+}
+
+void ShellProcessTests::testProcessListFromObject()
+{
+    YAML::Node doc = YAML::Load( R"(---
+script:
+    - command: "ls /tmp"
+      timeout: 12
+    - "-/bin/false"
+)" );
+    CommandList cl(
+        CalamaresUtils::yamlMapToVariant( doc ).toMap().value( "script" ) );
+    QVERIFY( !cl.isEmpty() );
+    QCOMPARE( cl.count(), 2 );
+    QCOMPARE( cl.at(0).timeout(), 12 );
+    QCOMPARE( cl.at(0).command(), QStringLiteral( "ls /tmp" ) );
+    QCOMPARE( cl.at(1).timeout(), -1 );  // not set
+}

src/modules/shellprocess/Tests.h

@@ -36,6 +36,10 @@ private Q_SLOTS:
     void testProcessListFromList();
     // Create from a simple YAML string
     void testProcessListFromString();
+    // Create from a single complex YAML
+    void testProcessFromObject();
+    // Create from a complex YAML list
+    void testProcessListFromObject();
 };
 
 #endif

src/modules/shellprocess/shellprocess.conf

@@ -8,12 +8,27 @@
 # system from the point of view of the command (for chrooted
 # commands, that will be */*).
 #
+# The (global) timeout for the command list can be set with
+# the *timeout* key. The value is a time in seconds, default
+# is 10 seconds if not set.
+#
 # If a command starts with "-" (a single minus sign), then the
 # return value of the command following the - is ignored; otherwise,
 # a failing command will abort the installation. This is much like
 # make's use of - in a command.
+#
+# The value of *script* may be:
+#   - a single string; this is one command that is executed.
+#   - a list of strings; these are executed one at a time, by
+#     separate shells (/bin/sh -c is invoked for each command).
+#   - an object, specifying a key *command* and (optionally)
+#     a key *timeout* to set the timeout for this specific
+#     command differently from the global setting.
 ---
 dontChroot: false
+timeout: 10
 script:
     - "-touch @@ROOT@@/tmp/thingy"
     - "/usr/bin/false"
+    - command: "/usr/local/bin/slowloris"
+      timeout: 3600