Merge branch 'issue-1851' into calamares

FIXES #1851

CHANGES-3.2

@@ -20,6 +20,9 @@ This release contains contributions from (alphabetically by first name):
    if the SSID or username contained non-ASCII characters **and** the
    default Python text-file encoding was set to ASCII. The files are
    now read and written in UTF-8, explicitly. #1848
+ - *preservefiles* was missing some necessary features, needed for it
+   to replace the deprecated log-file-saving functionality in the *umount*
+   module. (Thanks Erik and Joe for testing) #1851
 
 
 # 3.2.49.1 (2021-12-11) #

src/modules/preservefiles/CMakeLists.txt

@@ -3,14 +3,20 @@
 #   SPDX-FileCopyrightText: 2020 Adriaan de Groot <groot@kde.org>
 #   SPDX-License-Identifier: BSD-2-Clause
 #
-include_directories( ${PROJECT_BINARY_DIR}/src/libcalamaresui )
-
 calamares_add_plugin( preservefiles
     TYPE job
     EXPORT_MACRO PLUGINDLLEXPORT_PRO
     SOURCES
+        Item.cpp
         PreserveFiles.cpp
     # REQUIRES mount  # To set the rootMountPoint
     SHARED_LIB
     EMERGENCY
 )
+
+calamares_add_test(
+    preservefilestest
+    SOURCES
+        Item.cpp
+        Tests.cpp
+)

src/modules/preservefiles/Item.cpp

@@ -0,0 +1,159 @@
+/* === This file is part of Calamares - <https://calamares.io> ===
+ *
+ *  SPDX-FileCopyrightText: 2018, 2021 Adriaan de Groot <groot@kde.org>
+ *  SPDX-License-Identifier: GPL-3.0-or-later
+ *
+ */
+
+#include "Item.h"
+
+#include "GlobalStorage.h"
+#include "JobQueue.h"
+#include "utils/CalamaresUtilsSystem.h"
+#include "utils/Logger.h"
+#include "utils/Units.h"
+#include "utils/Variant.h"
+
+#include <QFile>
+
+using namespace CalamaresUtils::Units;
+
+static bool
+copy_file( const QString& source, const QString& dest )
+{
+    QFile sourcef( source );
+    if ( !sourcef.open( QFile::ReadOnly ) )
+    {
+        cWarning() << "Could not read" << source;
+        return false;
+    }
+
+    QFile destf( dest );
+    if ( !destf.open( QFile::WriteOnly ) )
+    {
+        sourcef.close();
+        cWarning() << "Could not open" << destf.fileName() << "for writing; could not copy" << source;
+        return false;
+    }
+
+    QByteArray b;
+    do
+    {
+        b = sourcef.read( 1_MiB );
+        destf.write( b );
+    } while ( b.count() > 0 );
+
+    sourcef.close();
+    destf.close();
+
+    return true;
+}
+
+Item
+Item::fromVariant( const QVariant& v, const CalamaresUtils::Permissions& defaultPermissions )
+{
+    if ( v.type() == QVariant::String )
+    {
+        QString filename = v.toString();
+        if ( !filename.isEmpty() )
+        {
+            return { filename, filename, defaultPermissions, ItemType::Path, false };
+        }
+        else
+        {
+            cWarning() << "Empty filename for preservefiles, item" << v;
+            return {};
+        }
+    }
+    else if ( v.type() == QVariant::Map )
+    {
+        const auto map = v.toMap();
+
+        CalamaresUtils::Permissions perm( defaultPermissions );
+        ItemType t = ItemType::None;
+        bool optional = CalamaresUtils::getBool( map, "optional", false );
+
+        {
+            QString perm_string = map[ "perm" ].toString();
+            if ( !perm_string.isEmpty() )
+            {
+                perm = CalamaresUtils::Permissions( perm_string );
+            }
+        }
+
+        {
+            QString from = map[ "from" ].toString();
+            t = ( from == "log" ) ? ItemType::Log : ( from == "config" ) ? ItemType::Config : ItemType::None;
+
+            if ( t == ItemType::None && !map[ "src" ].toString().isEmpty() )
+            {
+                t = ItemType::Path;
+            }
+        }
+
+        QString dest = map[ "dest" ].toString();
+        if ( dest.isEmpty() )
+        {
+            cWarning() << "Empty dest for preservefiles, item" << v;
+            return {};
+        }
+
+        switch ( t )
+        {
+        case ItemType::Config:
+            return { QString(), dest, perm, t, optional };
+        case ItemType::Log:
+            return { QString(), dest, perm, t, optional };
+        case ItemType::Path:
+            return { map[ "src" ].toString(), dest, perm, t, optional };
+        case ItemType::None:
+            cWarning() << "Invalid type for preservefiles, item" << v;
+            return {};
+        }
+    }
+    cWarning() << "Invalid type for preservefiles, item" << v;
+    return {};
+}
+
+
+bool
+Item::exec( const std::function< QString( QString ) >& replacements ) const
+{
+    QString expanded_dest = replacements( dest );
+    QString full_dest = CalamaresUtils::System::instance()->targetPath( expanded_dest );
+
+    bool success = false;
+    switch ( m_type )
+    {
+    case ItemType::None:
+        cWarning() << "Invalid item for preservefiles skipped.";
+        return false;
+    case ItemType::Config:
+        if ( !( success = Calamares::JobQueue::instance()->globalStorage()->saveJson( full_dest ) ) )
+        {
+            cWarning() << "Could not write a JSON dump of global storage to" << full_dest;
+        }
+        break;
+    case ItemType::Log:
+        if ( !( success = copy_file( Logger::logFile(), full_dest ) ) )
+        {
+            cWarning() << "Could not preserve log file to" << full_dest;
+        }
+        break;
+    case ItemType::Path:
+        if ( !( success = copy_file( source, full_dest ) ) )
+        {
+            cWarning() << "Could not preserve" << source << "to" << full_dest;
+        }
+        break;
+    }
+    if ( !success )
+    {
+        CalamaresUtils::System::instance()->removeTargetFile( expanded_dest );
+        return false;
+    }
+    else
+    {
+        return perm.apply( full_dest );
+    }
+}

src/modules/preservefiles/Item.h

@@ -0,0 +1,76 @@
+/* === This file is part of Calamares - <https://calamares.io> ===
+ *
+ *  SPDX-FileCopyrightText: 2018, 2021 Adriaan de Groot <groot@kde.org>
+ *  SPDX-License-Identifier: GPL-3.0-or-later
+ *
+ */
+#ifndef PRESERVEFILES_ITEM_H
+#define PRESERVEFILES_ITEM_H
+
+#include "utils/Permissions.h"
+
+#include <QString>
+#include <QVariant>
+
+#include <memory>
+
+enum class ItemType
+{
+    None,
+    Path,
+    Log,
+    Config
+};
+
+/** @brief Represents one item to copy
+ *
+ * All item types need a destination (to place the data), this is
+ * intepreted within the target system. All items need a permission,
+ * which is applied to the data once written.
+ *
+ * The source may be a path, but not all types need a source.
+ */
+class Item
+{
+    QString source;
+    QString dest;
+    CalamaresUtils::Permissions perm;
+    ItemType m_type = ItemType::None;
+    bool m_optional = false;
+
+public:
+    Item( const QString& src, const QString& d, CalamaresUtils::Permissions p, ItemType t, bool optional )
+        : source( src )
+        , dest( d )
+        , perm( std::move( p ) )
+        , m_type( t )
+        , m_optional( optional )
+    {
+    }
+
+    Item()
+        : m_type( ItemType::None )
+    {
+    }
+
+    operator bool() const { return m_type != ItemType::None; }
+    ItemType type() const { return m_type; }
+    bool isOptional() const { return m_optional; }
+
+    bool exec( const std::function< QString( QString ) >& replacements ) const;
+
+
+    /** @brief Create an Item -- or one of its subclasses -- from @p v
+     *
+     * Depending on the structure and contents of @p v, a pointer
+     * to an Item is returned. If @p v cannot be interpreted meaningfully,
+     * then a nullptr is returned.
+     *
+     * When the entry contains a *perm* key, use that permission, otherwise
+     * apply @p defaultPermissions to the item.
+     */
+    static Item fromVariant( const QVariant& v, const CalamaresUtils::Permissions& defaultPermissions );
+};
+
+
+#endif

src/modules/preservefiles/PreserveFiles.cpp

@@ -7,46 +7,20 @@
 
 #include "PreserveFiles.h"
 
+#include "Item.h"
+
 #include "CalamaresVersion.h"
 #include "GlobalStorage.h"
 #include "JobQueue.h"
 #include "utils/CalamaresUtilsSystem.h"
 #include "utils/CommandList.h"
 #include "utils/Logger.h"
-#include "utils/Permissions.h"
 #include "utils/Units.h"
 
 #include <QFile>
 
 using namespace CalamaresUtils::Units;
 
-QString
-targetPrefix()
-{
-    if ( CalamaresUtils::System::instance()->doChroot() )
-    {
-        Calamares::GlobalStorage* gs = Calamares::JobQueue::instance()->globalStorage();
-        if ( gs && gs->contains( "rootMountPoint" ) )
-        {
-            QString r = gs->value( "rootMountPoint" ).toString();
-            if ( !r.isEmpty() )
-            {
-                return r;
-            }
-            else
-            {
-                cDebug() << "RootMountPoint is empty";
-            }
-        }
-        else
-        {
-            cDebug() << "No rootMountPoint defined, preserving files to '/'";
-        }
-    }
-
-    return QLatin1String( "/" );
-}
-
 QString
 atReplacements( QString s )
 {
@@ -79,95 +53,34 @@ PreserveFiles::prettyName() const
     return tr( "Saving files for later ..." );
 }
 
-static bool
-copy_file( const QString& source, const QString& dest )
-{
-    QFile sourcef( source );
-    if ( !sourcef.open( QFile::ReadOnly ) )
-    {
-        cWarning() << "Could not read" << source;
-        return false;
-    }
-
-    QFile destf( dest );
-    if ( !destf.open( QFile::WriteOnly ) )
-    {
-        sourcef.close();
-        cWarning() << "Could not open" << destf.fileName() << "for writing; could not copy" << source;
-        return false;
-    }
-
-    QByteArray b;
-    do
-    {
-        b = sourcef.read( 1_MiB );
-        destf.write( b );
-    } while ( b.count() > 0 );
-
-    sourcef.close();
-    destf.close();
-
-    return true;
-}
-
 Calamares::JobResult
 PreserveFiles::exec()
 {
-    if ( m_items.isEmpty() )
+    if ( m_items.empty() )
     {
         return Calamares::JobResult::error( tr( "No files configured to save for later." ) );
     }
 
-    QString prefix = targetPrefix();
-    if ( !prefix.endsWith( '/' ) )
-    {
-        prefix.append( '/' );
-    }
-
     int count = 0;
-    for ( const auto& it : m_items )
+    for ( const auto& it : qAsConst( m_items ) )
     {
-        QString source = it.source;
+        if ( !it )
-        QString bare_dest = atReplacements( it.dest );
-        QString dest = prefix + bare_dest;
-
-        if ( it.type == ItemType::Log )
         {
-            source = Logger::logFile();
+            // Invalid entries are nullptr, ignore them but count as a success
+            // because they shouldn't block the installation. There are
+            // warnings in the log showing what the configuration problem is.
+            ++count;
+            continue;
         }
-        if ( it.type == ItemType::Config )
+        // Try to preserve the file. If it's marked as optional, count it
+        // as a success regardless.
+        if ( it.exec( atReplacements ) || it.isOptional() )
         {
-            if ( !Calamares::JobQueue::instance()->globalStorage()->saveJson( dest ) )
+            ++count;
-            {
-                cWarning() << "Could not write a JSON dump of global storage to" << dest;
-            }
-            else
-            {
-                ++count;
-            }
-        }
-        else if ( source.isEmpty() )
-        {
-            cWarning() << "Skipping unnamed source file for" << dest;
-        }
-        else
-        {
-            if ( copy_file( source, dest ) )
-            {
-                if ( it.perm.isValid() )
-                {
-                    if ( !it.perm.apply( CalamaresUtils::System::instance()->targetPath( bare_dest ) ) )
-                    {
-                        cWarning() << "Could not set attributes of" << bare_dest;
-                    }
-                }
-
-                ++count;
-            }
         }
     }
 
-    return count == m_items.count()
+    return count == m_items.size()
         ? Calamares::JobResult::ok()
         : Calamares::JobResult::error( tr( "Not all of the configured files could be preserved." ) );
 }
@@ -193,53 +106,11 @@ PreserveFiles::setConfigurationMap( const QVariantMap& configurationMap )
     {
         defaultPermissions = QStringLiteral( "root:root:0400" );
     }
+    CalamaresUtils::Permissions perm( defaultPermissions );
 
-    QVariantList l = files.toList();
+    for ( const auto& li : files.toList() )
-    unsigned int c = 0;
-    for ( const auto& li : l )
     {
-        if ( li.type() == QVariant::String )
+        m_items.push_back( Item::fromVariant( li, perm ) );
-        {
-            QString filename = li.toString();
-            if ( !filename.isEmpty() )
-                m_items.append(
-                    Item { filename, filename, CalamaresUtils::Permissions( defaultPermissions ), ItemType::Path } );
-            else
-            {
-                cDebug() << "Empty filename for preservefiles, item" << c;
-            }
-        }
-        else if ( li.type() == QVariant::Map )
-        {
-            const auto map = li.toMap();
-            QString dest = map[ "dest" ].toString();
-            QString from = map[ "from" ].toString();
-            ItemType t = ( from == "log" ) ? ItemType::Log : ( from == "config" ) ? ItemType::Config : ItemType::None;
-            QString perm = map[ "perm" ].toString();
-            if ( perm.isEmpty() )
-            {
-                perm = defaultPermissions;
-            }
-
-            if ( dest.isEmpty() )
-            {
-                cDebug() << "Empty dest for preservefiles, item" << c;
-            }
-            else if ( t == ItemType::None )
-            {
-                cDebug() << "Invalid type for preservefiles, item" << c;
-            }
-            else
-            {
-                m_items.append( Item { QString(), dest, CalamaresUtils::Permissions( perm ), t } );
-            }
-        }
-        else
-        {
-            cDebug() << "Invalid type for preservefiles, item" << c;
-        }
-
-        ++c;
     }
 }
 

src/modules/preservefiles/PreserveFiles.h

@@ -10,33 +10,14 @@
 
 #include "CppJob.h"
 #include "DllMacro.h"
-#include "utils/Permissions.h"
 #include "utils/PluginFactory.h"
 
-#include <QList>
+class Item;
-#include <QObject>
-#include <QVariantMap>
 
 class PLUGINDLLEXPORT PreserveFiles : public Calamares::CppJob
 {
     Q_OBJECT
 
-    enum class ItemType
-    {
-        None,
-        Path,
-        Log,
-        Config
-    };
-
-    struct Item
-    {
-        QString source;
-        QString dest;
-        CalamaresUtils::Permissions perm;
-        ItemType type;
-    };
-
     using ItemList = QList< Item >;
 
 public:

src/modules/preservefiles/Tests.cpp

@@ -0,0 +1,93 @@
+/* === This file is part of Calamares - <https://calamares.io> ===
+ *
+ *   SPDX-FileCopyrightText: 2021 Adriaan de Groot <groot@kde.org>
+ *   SPDX-License-Identifier: GPL-3.0-or-later
+ *
+ *   Calamares is Free Software: see the License-Identifier above.
+ *
+ */
+
+#include "Item.h"
+
+#include "Settings.h"
+#include "utils/CalamaresUtilsSystem.h"
+#include "utils/Logger.h"
+#include "utils/NamedEnum.h"
+#include "utils/Yaml.h"
+
+#include <QtTest/QtTest>
+
+class PreserveFilesTests : public QObject
+{
+    Q_OBJECT
+public:
+    PreserveFilesTests();
+    ~PreserveFilesTests() override {}
+
+private Q_SLOTS:
+    void initTestCase();
+
+    void testItems_data();
+    void testItems();
+};
+
+PreserveFilesTests::PreserveFilesTests() {}
+
+void
+PreserveFilesTests::initTestCase()
+{
+    Logger::setupLogLevel( Logger::LOGDEBUG );
+    cDebug() << "PreserveFiles test started.";
+
+    // Ensure we have a system object, expect it to be a "bogus" one
+    CalamaresUtils::System* system = CalamaresUtils::System::instance();
+    QVERIFY( system );
+    cDebug() << Logger::SubEntry << "System @" << Logger::Pointer( system );
+
+    const auto* settings = Calamares::Settings::instance();
+    if ( !settings )
+    {
+        (void)new Calamares::Settings( true );
+    }
+}
+
+void
+PreserveFilesTests::testItems_data()
+{
+    QTest::addColumn< QString >( "filename" );
+    QTest::addColumn< bool >( "ok" );
+    QTest::addColumn< int >( "type_i" );
+
+    QTest::newRow( "log     " ) << QString( "1a-log.conf" ) << true << smash( ItemType::Log );
+    QTest::newRow( "config  " ) << QString( "1b-config.conf" ) << true << smash( ItemType::Config );
+    QTest::newRow( "src     " ) << QString( "1c-src.conf" ) << true << smash( ItemType::Path );
+    QTest::newRow( "filename" ) << QString( "1d-filename.conf" ) << true << smash( ItemType::Path );
+    QTest::newRow( "empty   " ) << QString( "1e-empty.conf" ) << false << smash( ItemType::None );
+    QTest::newRow( "bad     " ) << QString( "1f-bad.conf" ) << false << smash( ItemType::None );
+}
+
+void
+PreserveFilesTests::testItems()
+{
+    QFETCH( QString, filename );
+    QFETCH( bool, ok );
+    QFETCH( int, type_i );
+
+    QFile fi( QString( "%1/tests/%2" ).arg( BUILD_AS_TEST, filename ) );
+    QVERIFY( fi.exists() );
+
+    bool config_file_ok = false;
+    const auto map = CalamaresUtils::loadYaml( fi, &config_file_ok );
+    QVERIFY( config_file_ok );
+
+    CalamaresUtils::Permissions perm( QStringLiteral( "adridg:adridg:0750" ) );
+    auto i = Item::fromVariant( map[ "item" ], perm );
+    QCOMPARE( bool( i ), ok );
+    QCOMPARE( smash( i.type() ), type_i );
+}
+
+QTEST_GUILESS_MAIN( PreserveFilesTests )
+
+#include "utils/moc-warnings.h"
+
+#include "Tests.moc"

src/modules/preservefiles/preservefiles.conf

@@ -7,33 +7,48 @@
 # the list should have one of these forms:
 #
 # - an absolute path (probably within the host system). This will be preserved
-#   as the same path within the target system (chroot). If, globally, dontChroot
+#   as the same path within the target system (chroot). If, globally,
-#   is true, then these items are ignored (since the destination is the same
+#   *dontChroot* is true, then these items will be ignored (since the
-#   as the source).
+#   destination is the same as the source).
 # - a map with a *dest* key. The *dest* value is a path interpreted in the
-#   target system (if dontChroot is true, in the host system). Relative paths
+#   target system (if the global *dontChroot* is true, then the host is the
-#   are not recommended. There are three possible other keys in the map:
+#   target as well). Relative paths are not recommended. There are two
+#   ways to select the source data for the file:
 #   - *from*, which must have one of the values, below; it is used to
 #     preserve files whose pathname is known to Calamares internally.
 #   - *src*, to refer to a path interpreted in the host system. Relative
 #     paths are not recommended, and are interpreted relative to where
 #     Calamares is being run.
+#   Exactly one of the two source keys (either *from* or *src*) must be set.
+#
+#   Special values for the key *from* are:
+#     - *log*, for the complete log file (up to the moment the preservefiles
+#       module is run),
+#     - *config*, for a JSON dump of the contents of global storage.
+#       Note that this may contain sensitive information, and should be
+#       given restrictive permissions.
+#
+#  A map with a *dest* key can have these additional fields:
 #   - *perm*, is a colon-separated tuple of <user>:<group>:<mode>
 #     where <mode> is in octal (e.g. 4777 for wide-open, 0400 for read-only
 #     by owner). If set, the file's ownership and permissions are set to
 #     those values within the target system; if not set, no permissions
 #     are changed.
-#   Only one of the two source keys (either *from* or *src*) may be set.
+#   - *optional*, is a boolean; if this is set to `true` then failure to
+#     preserve the file will **not** be counted as a failure of the
+#     module, and installation will proceed. Set this for files that might
+#     not exist in the host system (e.g. nvidia configuration files that
+#     are created in some boot scenarios and not in others).
 #
-# The target filename is modified as follows:
+# The target path (*dest*) is modified as follows:
-#   - `@@ROOT@@` is replaced by the path to the target root (may be /)
+#   - `@@ROOT@@` is replaced by the path to the target root (may be /).
+#     There is never any reason to use this, since the *dest* is already
+#     interpreted in the target system.
 #   - `@@USER@@` is replaced by the username entered by on the user
 #       page (may be empty, for instance if no user page is enabled)
 #
-# Special values for the key *from* are:
+#
-#   - *log*, for the complete log file (up to the moment the preservefiles
+#
-#     module is run),
-#   - *config*, for a JSON dump of the contents of global storage
 files:
   - from: log
     dest: /var/log/Calamares.log
@@ -41,6 +56,9 @@ files:
   - from: config
     dest: /var/log/Calamares-install.json
     perm: root:wheel:600
+# - src: /var/log/nvidia.conf
+#   dest: /var/log/Calamares-nvidia.conf
+#   optional: true
 
 # The *perm* key contains a default value to apply to all files listed
 # above that do not have a *perm* key of their own. If not set,

src/modules/preservefiles/preservefiles.schema.yaml

@@ -0,0 +1,37 @@
+# SPDX-FileCopyrightText: 2020 Adriaan de Groot <groot@kde.org>
+# SPDX-License-Identifier: GPL-3.0-or-later
+---
+$schema: https://json-schema.org/schema#
+$id: https://calamares.io/schemas/preservefiles
+additionalProperties: false
+type: object
+properties:
+    # TODO: it's a particularly-formatted string
+    perm: { type: string }
+    files:
+        type: array
+        items:
+            # There are three entries here because: string, or an entry with
+            # a src (but no from) or an entry with from (but no src).
+            anyOf:
+              - type: string
+              - type: object
+                properties:
+                    dest: { type: string }
+                    src: { type: string }
+                    # TODO: it's a particularly-formatted string
+                    perm: { type: string }
+                    optional: { type: boolean }
+                required: [ dest ]
+                additionalProperties: false
+              - type: object
+                properties:
+                    dest: { type: string }
+                    from: { type: string, enum: [config, log] }
+                    # TODO: it's a particularly-formatted string
+                    perm: { type: string }
+                    optional: { type: boolean }
+                required: [ dest ]
+                additionalProperties: false
+
+required: [ files ]

src/modules/preservefiles/tests/1a-log.conf

@@ -0,0 +1,7 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+#
+item:
+    from: log
+    dest: /var/log/Calamares.log
+    perm: root:wheel:601

src/modules/preservefiles/tests/1b-config.conf

@@ -0,0 +1,6 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+item:
+    from: config
+    dest: /var/log/Calamares-install.json
+    perm: root:wheel:600

src/modules/preservefiles/tests/1c-src.conf

@@ -0,0 +1,6 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+item:
+    src: /root/.cache/calamares/session.log
+    dest: /var/log/Calamares.log
+    perm: root:wheel:600

src/modules/preservefiles/tests/1d-filename.conf

@@ -0,0 +1,6 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+item:
+    src: /root/.cache/calamares/session.log
+    dest: /var/log/Calamares.log
+    perm: root:wheel:600

src/modules/preservefiles/tests/1e-empty.conf

@@ -0,0 +1,3 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+item: []

src/modules/preservefiles/tests/1f-bad.conf

@@ -0,0 +1,4 @@
+# SPDX-FileCopyrightText: no
+# SPDX-License-Identifier: CC0-1.0
+item:
+    bop: 1