[partition] Log partition-model lookup failures

- An empty LV name isn't allowed, but the create-partition-dialog
  doesn't check for that.

CHANGES-3.2

@@ -7,6 +7,30 @@ contributors are listed. Note that Calamares does not have a historical
 changelog -- this log starts with version 3.2.0. The release notes on the
 website will have to do for older versions.
 
+# 3.2.50 (unreleased) #
+
+This release contains contributions from (alphabetically by first name):
+ - Erik Dubois
+
+## Core ##
+ - No core changes yet
+
+## Modules ##
+ - *networkcfg* could fail to update the NetworkManager configuration
+   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) #
+
+This is a hot-fix release, to fix a regression in the calculation of
+swap-size. Reported by EndeavourOS (Joe Kamprad) and Xero Linux.
+
+
 # 3.2.49 (2021-12-10) #
 
 This release contains contributions from (alphabetically by first name):

CMakeLists.txt

@@ -41,11 +41,11 @@
 # TODO:3.3: Require CMake 3.12
 cmake_minimum_required( VERSION 3.3 FATAL_ERROR )
 project( CALAMARES
-    VERSION 3.2.49
+    VERSION 3.2.50
     LANGUAGES C CXX
 )
 
-set( CALAMARES_VERSION_RC 0 )  # Set to 0 during release cycle, 1 during development
+set( CALAMARES_VERSION_RC 1 )  # Set to 0 during release cycle, 1 during development
 if( CALAMARES_VERSION_RC EQUAL 1 AND CMAKE_SOURCE_DIR STREQUAL CMAKE_BINARY_DIR )
     message( FATAL_ERROR "Do not build development versions in the source-directory." )
 endif()

src/modules/networkcfg/main.py

@@ -73,12 +73,12 @@ def replace_username(nm_config_filename, live_user, target_user):
     if not os.path.exists(nm_config_filename):
         return
 
-    with open(nm_config_filename, "r") as network_conf:
+    with open(nm_config_filename, "r", encoding="UTF-8") as network_conf:
         text = network_conf.readlines()
 
     live_permissions = 'permissions=user:{}:;'.format(live_user)
     target_permissions = 'permissions=user:{}:;\n'.format(target_user)
-    with open(nm_config_filename, "w") as network_conf:
+    with open(nm_config_filename, "w", encoding="UTF-8") as network_conf:
         for line in text:
             if live_permissions in line:
                 line = target_permissions

src/modules/partition/core/DeviceList.cpp

@@ -11,6 +11,7 @@
 #include "DeviceList.h"
 
 #include "partition/PartitionIterator.h"
+#include "utils/CalamaresUtilsSystem.h"
 #include "utils/Logger.h"
 
 #include <kpmcore/backend/corebackend.h>
@@ -43,11 +44,9 @@ hasRootPartition( Device* device )
 static bool
 blkIdCheckIso9660( const QString& path )
 {
-    QProcess blkid;
-    blkid.start( "blkid", { path } );
-    blkid.waitForFinished();
-    QString output = QString::fromLocal8Bit( blkid.readAllStandardOutput() );
-    return output.contains( "iso9660" );
+    // If blkid fails, there's no output, but we don't care
+    auto r = CalamaresUtils::System::runCommand( { "blkid", path }, std::chrono::seconds( 30 ) );
+    return r.getOutput().contains( "iso9660" );
 }
 
 static bool

src/modules/partition/core/KPMHelpers.cpp

@@ -15,8 +15,8 @@
 
 #include "partition/PartitionIterator.h"
 #include "utils/Logger.h"
+#include "utils/String.h"
 
-// KPMcore
 #include <kpmcore/backend/corebackendmanager.h>
 #include <kpmcore/core/device.h>
 #include <kpmcore/core/partition.h>
@@ -127,4 +127,23 @@ clonePartition( Device* device, Partition* partition )
                           partition->activeFlags() );
 }
 
+Calamares::JobResult
+execute( Operation& operation, const QString& failureMessage )
+{
+    operation.setStatus( Operation::StatusRunning );
+
+    Report report( nullptr );
+    if ( operation.execute( report ) )
+    {
+        return Calamares::JobResult::ok();
+    }
+
+    // Remove the === lines from the report by trimming them to empty
+    QStringList l = report.toText().split( '\n' );
+    std::for_each( l.begin(), l.end(), []( QString& s ) { CalamaresUtils::removeLeading( s, '=' ); } );
+
+    return Calamares::JobResult::error( failureMessage, l.join( '\n' ) );
+}
+
+
 }  // namespace KPMHelpers

src/modules/partition/core/KPMHelpers.h

@@ -11,12 +11,15 @@
 #ifndef KPMHELPERS_H
 #define KPMHELPERS_H
 
-// KPMcore
+#include "Job.h"
+
 #include <kpmcore/core/partitiontable.h>
 #include <kpmcore/fs/filesystem.h>
+#include <kpmcore/ops/operation.h>
+#include <kpmcore/util/report.h>
 
-// Qt
 #include <QList>
+#include <QVector>
 
 #include <functional>
 
@@ -35,6 +38,8 @@ class PartitionRole;
 #define KPM_PARTITION_FLAG_ESP PartitionTable::FlagEsp
 #endif
 
+using PartitionVector = QVector< const Partition* >;
+
 /**
  * Helper functions to manipulate partitions
  */
@@ -72,6 +77,24 @@ Partition* createNewEncryptedPartition( PartitionNode* parent,
 
 Partition* clonePartition( Device* device, Partition* partition );
 
+/** @brief Return a result for an @p operation
+ *
+ * Executes the operation, and if successful, returns a success result.
+ * Otherwise returns an error using @p failureMessage as the primary part
+ * of the error, and details obtained from the operation.
+ */
+Calamares::JobResult execute( Operation& operation, const QString& failureMessage );
+/** @brief Return a result for an @p operation
+ *
+ * It's acceptable to use an rvalue: the operation-running is the effect
+ * you're interested in, rather than keeping the temporary around.
+ */
+static inline Calamares::JobResult
+execute( Operation&& operation, const QString& failureMessage )
+{
+    return execute( operation, failureMessage );
+}
+
 }  // namespace KPMHelpers
 
 #endif /* KPMHELPERS_H */

src/modules/partition/core/PartitionActions.cpp

@@ -71,7 +71,7 @@ swapSuggestion( const qint64 availableSpaceB, Config::SwapChoice swap )
 
 
     // Allow for a fudge factor
-    suggestedSwapSizeB = qRound( suggestedSwapSizeB * overestimationFactor );
+    suggestedSwapSizeB = qRound64( suggestedSwapSizeB * overestimationFactor );
 
     // don't use more than 10% of available space
     if ( !ensureSuspendToDisk )

src/modules/partition/core/PartitionCoreModule.cpp

@@ -262,11 +262,9 @@ PartitionCoreModule::doInit()
             // Gives ownership of the Device* to the DeviceInfo object
             auto deviceInfo = new DeviceInfo( device );
             m_deviceInfos << deviceInfo;
-            cDebug() << Logger::SubEntry
-                << device->deviceNode()
-                << device->capacity()
-                << Logger::RedactedName( "DevName", device->name() )
-                << Logger::RedactedName( "DevNamePretty", device->prettyName() );
+            cDebug() << Logger::SubEntry << device->deviceNode() << device->capacity()
+                     << Logger::RedactedName( "DevName", device->name() )
+                     << Logger::RedactedName( "DevNamePretty", device->prettyName() );
         }
         else
         {
@@ -360,7 +358,12 @@ PartitionModel*
 PartitionCoreModule::partitionModelForDevice( const Device* device ) const
 {
     DeviceInfo* info = infoForDevice( device );
-    Q_ASSERT( info );
+    if ( !info )
+    {
+        cWarning() << "No DeviceInfo for" << Logger::Pointer( device )
+                   << ( device ? device->deviceNode() : QStringLiteral( "<null>" ) );
+        return nullptr;
+    }
     return info->partitionModel.data();
 }
 
@@ -411,15 +414,16 @@ PartitionCoreModule::createPartition( Device* device, Partition* partition, Part
 }
 
 void
-PartitionCoreModule::createVolumeGroup( QString& vgName, QVector< const Partition* > pvList, qint32 peSize )
+PartitionCoreModule::createVolumeGroup( const QString& vgName, const PartitionVector& pvList, qint32 peSize )
 {
+    QString actualName( vgName );
     // Appending '_' character in case of repeated VG name
-    while ( hasVGwithThisName( vgName ) )
+    while ( hasVGwithThisName( actualName ) )
     {
-        vgName.append( '_' );
+        actualName.append( '_' );
     }
 
-    LvmDevice* device = new LvmDevice( vgName );
+    LvmDevice* device = new LvmDevice( actualName );
     for ( const Partition* p : pvList )
     {
         device->physicalVolumes() << p;
@@ -430,12 +434,12 @@ PartitionCoreModule::createVolumeGroup( QString& vgName, QVector< const Partitio
     m_deviceModel->addDevice( device );
     m_deviceInfos << deviceInfo;
 
-    deviceInfo->makeJob< CreateVolumeGroupJob >( vgName, pvList, peSize );
+    deviceInfo->makeJob< CreateVolumeGroupJob >( actualName, pvList, peSize );
     refreshAfterModelChange();
 }
 
 void
-PartitionCoreModule::resizeVolumeGroup( LvmDevice* device, QVector< const Partition* >& pvList )
+PartitionCoreModule::resizeVolumeGroup( LvmDevice* device, const PartitionVector& pvList )
 {
     auto* deviceInfo = infoForDevice( device );
     Q_ASSERT( deviceInfo );
@@ -676,7 +680,7 @@ PartitionCoreModule::efiSystemPartitions() const
     return m_efiSystemPartitions;
 }
 
-QVector< const Partition* >
+PartitionVector
 PartitionCoreModule::lvmPVs() const
 {
     return m_lvmPVs;

src/modules/partition/core/PartitionCoreModule.h

@@ -136,25 +136,18 @@ public:
      */
     void
     createPartition( Device* device, Partition* partition, PartitionTable::Flags flags = KPM_PARTITION_FLAG( None ) );
+    void deletePartition( Device* device, Partition* partition );
+    void formatPartition( Device* device, Partition* partition );
+    void resizePartition( Device* device, Partition* partition, qint64 first, qint64 last );
+    void setPartitionFlags( Device* device, Partition* partition, PartitionTable::Flags flags );
 
-    void createVolumeGroup( QString& vgName, QVector< const Partition* > pvList, qint32 peSize );
-
-    void resizeVolumeGroup( LvmDevice* device, QVector< const Partition* >& pvList );
-
+    void createVolumeGroup( const QString& vgName, const PartitionVector& pvList, qint32 peSize );
+    void resizeVolumeGroup( LvmDevice* device, const PartitionVector& pvList );
     void deactivateVolumeGroup( LvmDevice* device );
-
     void removeVolumeGroup( LvmDevice* device );
 
-    void deletePartition( Device* device, Partition* partition );
-
-    void formatPartition( Device* device, Partition* partition );
-
     void setFilesystemLabel( Device* device, Partition* partition, const QString& newLabel );
 
-    void resizePartition( Device* device, Partition* partition, qint64 first, qint64 last );
-
-    void setPartitionFlags( Device* device, Partition* partition, PartitionTable::Flags flags );
-
     /// @brief Retrieve the path where the bootloader will be installed
     QString bootLoaderInstallPath() const { return m_bootLoaderInstallPath; }
     /// @brief Set the path where the bootloader will be installed
@@ -185,7 +178,7 @@ public:
 
     QList< Partition* > efiSystemPartitions() const;
 
-    QVector< const Partition* > lvmPVs() const;
+    PartitionVector lvmPVs() const;
 
     bool hasVGwithThisName( const QString& name ) const;
 
@@ -255,7 +248,7 @@ private:
 
     QList< DeviceInfo* > m_deviceInfos;
     QList< Partition* > m_efiSystemPartitions;
-    QVector< const Partition* > m_lvmPVs;
+    PartitionVector m_lvmPVs;
 
     DeviceModel* m_deviceModel;
     BootLoaderModel* m_bootLoaderModel;

src/modules/partition/gui/CreatePartitionDialog.cpp

@@ -66,19 +66,22 @@ CreatePartitionDialog::CreatePartitionDialog( Device* device,
     m_ui->encryptWidget->setText( tr( "En&crypt" ) );
     m_ui->encryptWidget->hide();
 
-    if ( m_device->type() != Device::Type::LVM_Device )
-    {
-        m_ui->lvNameLabel->hide();
-        m_ui->lvNameLineEdit->hide();
-    }
     if ( m_device->type() == Device::Type::LVM_Device )
     {
+        m_ui->lvNameLabel->show();
+        m_ui->lvNameLineEdit->show();
         /* LVM logical volume name can consist of: letters numbers _ . - +
          * It cannot start with underscore _ and must not be equal to . or .. or any entry in /dev/
          * QLineEdit accepts QValidator::Intermediate, so we just disable . at the beginning */
         QRegularExpression re( QStringLiteral( R"(^(?!_|\.)[\w\-.+]+)" ) );
         QRegularExpressionValidator* validator = new QRegularExpressionValidator( re, this );
         m_ui->lvNameLineEdit->setValidator( validator );
+        connect( m_ui->lvNameLineEdit, &QLineEdit::textChanged, this, &CreatePartitionDialog::updateOkButton );
+    }
+    else
+    {
+        m_ui->lvNameLabel->hide();
+        m_ui->lvNameLineEdit->hide();
     }
 
     if ( device->partitionTable()->type() == PartitionTable::msdos
@@ -347,3 +350,13 @@ CreatePartitionDialog::initPartResizerWidget( Partition* partition )
     m_partitionSizeController->setPartResizerWidget( m_ui->partResizerWidget );
     m_partitionSizeController->setSpinBox( m_ui->sizeSpinBox );
 }
+
+void
+CreatePartitionDialog::updateOkButton()
+{
+    if ( m_device->type() == Device::Type::LVM_Device )
+    {
+        QString lvName = m_ui->lvNameLineEdit->text();
+        cDebug() << "LVName" << lvName << m_ui->lvNameLineEdit->hasAcceptableInput();
+    }
+}

src/modules/partition/gui/CreatePartitionDialog.h

@@ -45,10 +45,12 @@ private:
                            QWidget* parentWidget );
 
 public:
+    /// @brief Tag-type for creating partition from free space
     struct FreeSpace
     {
         Partition* p;
     };
+    /// @brief Tag-type for editing (re-creating) a new partition
     struct FreshPartition
     {
         Partition* p;
@@ -81,6 +83,7 @@ public:
 private Q_SLOTS:
     void updateMountPointUi();
     void checkMountPointSelection();
+    void updateOkButton();  // Check if dialog can be accepted
 
 private:
     QScopedPointer< Ui_CreatePartitionDialog > m_ui;

src/modules/partition/gui/CreateVolumeGroupDialog.cpp

@@ -16,32 +16,13 @@
 #include <QLineEdit>
 #include <QSpinBox>
 
-CreateVolumeGroupDialog::CreateVolumeGroupDialog( QString& vgName,
-                                                  QVector< const Partition* >& selectedPVs,
-                                                  QVector< const Partition* > pvList,
-                                                  qint64& pSize,
+CreateVolumeGroupDialog::CreateVolumeGroupDialog( const PartitionVector& pvList,
+                                                  qint32 physicalExtentSize,
                                                   QWidget* parent )
-    : VolumeGroupBaseDialog( vgName, pvList, parent )
-    , m_selectedPVs( selectedPVs )
-    , m_peSize( pSize )
+    : VolumeGroupBaseDialog( parent, QString(), pvList )
 {
     setWindowTitle( tr( "Create Volume Group" ) );
 
-    peSize()->setValue( pSize );
-
-    vgType()->setEnabled( false );
-}
-
-void
-CreateVolumeGroupDialog::accept()
-{
-    QString& name = vgNameValue();
-    name = vgName()->text();
-
-    m_selectedPVs << checkedItems();
-
-    qint64& pe = m_peSize;
-    pe = peSize()->value();
-
-    QDialog::accept();
+    peSizeWidget()->setValue( physicalExtentSize );
+    vgTypeWidget()->setEnabled( false );
 }

src/modules/partition/gui/CreateVolumeGroupDialog.h

@@ -16,18 +16,7 @@ class CreateVolumeGroupDialog : public VolumeGroupBaseDialog
 {
     Q_OBJECT
 public:
-    CreateVolumeGroupDialog( QString& vgName,
-                             QVector< const Partition* >& selectedPVs,
-                             QVector< const Partition* > pvList,
-                             qint64& pSize,
-                             QWidget* parent );
-
-    void accept() override;
-
-private:
-    QVector< const Partition* >& m_selectedPVs;
-
-    qint64& m_peSize;
+    CreateVolumeGroupDialog( const PartitionVector& pvList, qint32 physicalExtentSize, QWidget* parent );
 };
 
 #endif  // CREATEVOLUMEGROUPDIALOG_H

src/modules/partition/gui/PartitionPage.cpp

@@ -278,26 +278,30 @@ PartitionPage::checkCanCreate( Device* device )
     }
 }
 
-void
-PartitionPage::onNewVolumeGroupClicked()
+static inline PartitionVector
+availablePVs( PartitionCoreModule* core )
 {
-    QString vgName;
-    QVector< const Partition* > selectedPVs;
-    qint64 peSize = 4;
+    PartitionVector availablePVs;
 
-    QVector< const Partition* > availablePVs;
-
-    for ( const Partition* p : m_core->lvmPVs() )
-        if ( !m_core->isInVG( p ) )
+    for ( const Partition* p : core->lvmPVs() )
+    {
+        if ( !core->isInVG( p ) )
         {
             availablePVs << p;
         }
+    }
+    return availablePVs;
+}
 
-    QPointer< CreateVolumeGroupDialog > dlg
-        = new CreateVolumeGroupDialog( vgName, selectedPVs, availablePVs, peSize, this );
+
+void
+PartitionPage::onNewVolumeGroupClicked()
+{
+    QPointer< CreateVolumeGroupDialog > dlg = new CreateVolumeGroupDialog( availablePVs( m_core ), 4, this );
 
     if ( dlg->exec() == QDialog::Accepted )
     {
+        const PartitionVector selectedPVs = dlg->selectedPVs();
         QModelIndex partitionIndex = m_ui->partitionTreeView->currentIndex();
 
         if ( partitionIndex.isValid() )
@@ -321,7 +325,7 @@ PartitionPage::onNewVolumeGroupClicked()
         QVariant previousIndexDeviceData = m_core->deviceModel()->data( deviceIndex, Qt::ToolTipRole );
 
         // Creating new VG
-        m_core->createVolumeGroup( vgName, selectedPVs, peSize );
+        m_core->createVolumeGroup( dlg->volumeGroupName(), selectedPVs, dlg->physicalExtentSize() );
 
         // As createVolumeGroup method call resets deviceModel,
         // is needed to set the current index in deviceComboBox as the previous one
@@ -342,20 +346,11 @@ PartitionPage::onResizeVolumeGroupClicked()
 
     Q_ASSERT( device && device->type() == Device::Type::LVM_Device );
 
-    QVector< const Partition* > availablePVs;
-    QVector< const Partition* > selectedPVs;
-
-    for ( const Partition* p : m_core->lvmPVs() )
-        if ( !m_core->isInVG( p ) )
-        {
-            availablePVs << p;
-        }
-
-    QPointer< ResizeVolumeGroupDialog > dlg = new ResizeVolumeGroupDialog( device, availablePVs, selectedPVs, this );
+    QPointer< ResizeVolumeGroupDialog > dlg = new ResizeVolumeGroupDialog( device, availablePVs( m_core ), this );
 
     if ( dlg->exec() == QDialog::Accepted )
     {
-        m_core->resizeVolumeGroup( device, selectedPVs );
+        m_core->resizeVolumeGroup( device, dlg->selectedPVs() );
     }
 
     delete dlg;

src/modules/partition/gui/ResizeVolumeGroupDialog.cpp

@@ -22,38 +22,28 @@
 
 ResizeVolumeGroupDialog::ResizeVolumeGroupDialog( LvmDevice* device,
                                                   const PartitionVector& availablePVs,
-                                                  PartitionVector& selectedPVs,
                                                   QWidget* parent )
-    : VolumeGroupBaseDialog( device->name(), device->physicalVolumes(), parent )
-    , m_selectedPVs( selectedPVs )
+    : VolumeGroupBaseDialog( parent, device->name(), device->physicalVolumes() )
 {
     setWindowTitle( tr( "Resize Volume Group" ) );
 
-    for ( int i = 0; i < pvList()->count(); i++ )
+    for ( int i = 0; i < pvListWidget()->count(); i++ )
     {
-        pvList()->item( i )->setCheckState( Qt::Checked );
+        pvListWidget()->item( i )->setCheckState( Qt::Checked );
     }
 
     for ( const Partition* p : availablePVs )
     {
-        pvList()->addItem( new ListPhysicalVolumeWidgetItem( p, false ) );
+        pvListWidget()->addItem( new ListPhysicalVolumeWidgetItem( p, false ) );
     }
 
-    peSize()->setValue(
+    peSizeWidget()->setValue(
         static_cast< int >( device->peSize() / Capacity::unitFactor( Capacity::Unit::Byte, Capacity::Unit::MiB ) ) );
 
-    vgName()->setEnabled( false );
-    peSize()->setEnabled( false );
-    vgType()->setEnabled( false );
+    vgNameWidget()->setEnabled( false );
+    peSizeWidget()->setEnabled( false );
+    vgTypeWidget()->setEnabled( false );
 
     setUsedSizeValue( device->allocatedPE() * device->peSize() );
     setLVQuantity( device->partitionTable()->children().count() );
 }
-
-void
-ResizeVolumeGroupDialog::accept()
-{
-    m_selectedPVs << checkedItems();
-
-    QDialog::accept();
-}

src/modules/partition/gui/ResizeVolumeGroupDialog.h

@@ -19,17 +19,7 @@ class ResizeVolumeGroupDialog : public VolumeGroupBaseDialog
 {
     Q_OBJECT
 public:
-    using PartitionVector = QVector< const Partition* >;
-
-    ResizeVolumeGroupDialog( LvmDevice* device,
-                             const PartitionVector& availablePVs,
-                             PartitionVector& selectedPVs,
-                             QWidget* parent );
-
-    void accept() override;
-
-private:
-    PartitionVector& m_selectedPVs;
+    ResizeVolumeGroupDialog( LvmDevice* device, const PartitionVector& availablePVs, QWidget* parent );
 };
 
 #endif  // RESIZEVOLUMEGROUPDIALOG_H

src/modules/partition/gui/VolumeGroupBaseDialog.cpp

@@ -10,6 +10,7 @@
 #include "VolumeGroupBaseDialog.h"
 #include "ui_VolumeGroupBaseDialog.h"
 
+#include "core/PartitionCoreModule.h"
 #include "core/SizeUtils.h"
 #include "gui/ListPhysicalVolumeWidgetItem.h"
 
@@ -20,10 +21,11 @@
 #include <QPushButton>
 #include <QSpinBox>
 
-VolumeGroupBaseDialog::VolumeGroupBaseDialog( QString& vgName, QVector< const Partition* > pvList, QWidget* parent )
+// Keeping the sources consistent
+VolumeGroupBaseDialog::VolumeGroupBaseDialog( QWidget* parent, const QString& vgName, PartitionVector pvList )
     : QDialog( parent )
     , ui( new Ui::VolumeGroupBaseDialog )
-    , m_vgNameValue( vgName )
+    , m_volumeGroupName( vgName )
     , m_totalSizeValue( 0 )
     , m_usedSizeValue( 0 )
 {
@@ -34,13 +36,12 @@ VolumeGroupBaseDialog::VolumeGroupBaseDialog( QString& vgName, QVector< const Pa
         ui->pvList->addItem( new ListPhysicalVolumeWidgetItem( p, false ) );
     }
 
-    ui->vgType->addItems( QStringList() << "LVM"
-                                        << "RAID" );
+    ui->vgType->addItems( { "LVM", "RAID" } );
     ui->vgType->setCurrentIndex( 0 );
 
     QRegularExpression re( R"(^(?!_|\.)[\w\-.+]+)" );
     ui->vgName->setValidator( new QRegularExpressionValidator( re, this ) );
-    ui->vgName->setText( m_vgNameValue );
+    ui->vgName->setText( vgName );
 
     updateOkButton();
     updateTotalSize();
@@ -55,7 +56,10 @@ VolumeGroupBaseDialog::VolumeGroupBaseDialog( QString& vgName, QVector< const Pa
         updateOkButton();
     } );
 
-    connect( ui->vgName, &QLineEdit::textChanged, this, [&]( const QString& ) { updateOkButton(); } );
+    connect( ui->vgName, &QLineEdit::textChanged, this, [&]( const QString& s ) {
+        m_volumeGroupName = s;
+        updateOkButton();
+    } );
 }
 
 VolumeGroupBaseDialog::~VolumeGroupBaseDialog()
@@ -63,10 +67,10 @@ VolumeGroupBaseDialog::~VolumeGroupBaseDialog()
     delete ui;
 }
 
-QVector< const Partition* >
-VolumeGroupBaseDialog::checkedItems() const
+PartitionVector
+VolumeGroupBaseDialog::selectedPVs() const
 {
-    QVector< const Partition* > items;
+    PartitionVector items;
 
     for ( int i = 0; i < ui->pvList->count(); i++ )
     {
@@ -90,8 +94,8 @@ VolumeGroupBaseDialog::isSizeValid() const
 void
 VolumeGroupBaseDialog::updateOkButton()
 {
-    okButton()->setEnabled( isSizeValid() && !checkedItems().empty() && !ui->vgName->text().isEmpty()
-                            && ui->peSize->value() > 0 );
+    okButtonWidget()->setEnabled( isSizeValid() && !selectedPVs().empty() && !ui->vgName->text().isEmpty()
+                                  && ui->peSize->value() > 0 );
 }
 
 void
@@ -111,13 +115,14 @@ VolumeGroupBaseDialog::setLVQuantity( qint32 lvQuantity )
 void
 VolumeGroupBaseDialog::updateTotalSize()
 {
+    m_physicalExtentSize = peSizeWidget()->value();
     m_totalSizeValue = 0;
 
-    for ( const Partition* p : checkedItems() )
+    for ( const Partition* p : selectedPVs() )
     {
         m_totalSizeValue += p->capacity()
             - p->capacity()
-                % ( ui->peSize->value() * Capacity::unitFactor( Capacity::Unit::Byte, Capacity::Unit::MiB ) );
+                % ( m_physicalExtentSize * Capacity::unitFactor( Capacity::Unit::Byte, Capacity::Unit::MiB ) );
     }
 
     ui->totalSize->setText( formatByteSize( m_totalSizeValue ) );
@@ -128,9 +133,10 @@ VolumeGroupBaseDialog::updateTotalSize()
 void
 VolumeGroupBaseDialog::updateTotalSectors()
 {
-    qint64 totalSectors = 0;
+    m_physicalExtentSize = peSizeWidget()->value();
 
-    qint64 extentSize = ui->peSize->value() * Capacity::unitFactor( Capacity::Unit::Byte, Capacity::Unit::MiB );
+    qint64 totalSectors = 0;
+    qint64 extentSize = m_physicalExtentSize * Capacity::unitFactor( Capacity::Unit::Byte, Capacity::Unit::MiB );
 
     if ( extentSize > 0 )
     {
@@ -140,38 +146,32 @@ VolumeGroupBaseDialog::updateTotalSectors()
     ui->totalSectors->setText( QString::number( totalSectors ) );
 }
 
-QString&
-VolumeGroupBaseDialog::vgNameValue() const
-{
-    return m_vgNameValue;
-}
-
 QLineEdit*
-VolumeGroupBaseDialog::vgName() const
+VolumeGroupBaseDialog::vgNameWidget() const
 {
     return ui->vgName;
 }
 
 QComboBox*
-VolumeGroupBaseDialog::vgType() const
+VolumeGroupBaseDialog::vgTypeWidget() const
 {
     return ui->vgType;
 }
 
 QSpinBox*
-VolumeGroupBaseDialog::peSize() const
+VolumeGroupBaseDialog::peSizeWidget() const
 {
     return ui->peSize;
 }
 
 QListWidget*
-VolumeGroupBaseDialog::pvList() const
+VolumeGroupBaseDialog::pvListWidget() const
 {
     return ui->pvList;
 }
 
 QPushButton*
-VolumeGroupBaseDialog::okButton() const
+VolumeGroupBaseDialog::okButtonWidget() const
 {
     return ui->buttonBox->button( QDialogButtonBox::StandardButton::Ok );
 }

src/modules/partition/gui/VolumeGroupBaseDialog.h

@@ -10,7 +10,7 @@
 #ifndef VOLUMEGROUPBASEDIALOG_H
 #define VOLUMEGROUPBASEDIALOG_H
 
-#include <kpmcore/core/partition.h>
+#include "core/KPMHelpers.h"
 
 #include <QDialog>
 
@@ -29,43 +29,46 @@ class VolumeGroupBaseDialog : public QDialog
     Q_OBJECT
 
 public:
-    explicit VolumeGroupBaseDialog( QString& vgName, QVector< const Partition* > pvList, QWidget* parent = nullptr );
+    explicit VolumeGroupBaseDialog( QWidget* parent, const QString& vgName, PartitionVector pvList );
     ~VolumeGroupBaseDialog() override;
 
+    qint32 physicalExtentSize() const { return m_physicalExtentSize; }
+    QString volumeGroupName() const { return m_volumeGroupName; }
+    /** @brief Which PVs (partitions) are selected for this VG
+     *
+     * The vector contains non-owned pointers.
+     */
+    PartitionVector selectedPVs() const;
+
 protected:
     virtual void updateOkButton();
 
     void setUsedSizeValue( qint64 usedSize );
-
     void setLVQuantity( qint32 lvQuantity );
-
-    void updateTotalSize();
-
-    void updateTotalSectors();
-
-    QVector< const Partition* > checkedItems() const;
-
     bool isSizeValid() const;
 
-    QString& vgNameValue() const;
+    void updateTotalSize();
+    void updateTotalSectors();
 
-    QLineEdit* vgName() const;
 
-    QComboBox* vgType() const;
-
-    QSpinBox* peSize() const;
-
-    QListWidget* pvList() const;
-
-    QPushButton* okButton() const;
+    /** @section UI-widget accessors
+     *
+     * These methods get UI internal widgets, so that subclasses
+     * can manipulate the values in those widgets.
+     */
+    QLineEdit* vgNameWidget() const;
+    QComboBox* vgTypeWidget() const;
+    QSpinBox* peSizeWidget() const;
+    QListWidget* pvListWidget() const;
+    QPushButton* okButtonWidget() const;
 
 private:
     Ui::VolumeGroupBaseDialog* ui;
 
-    QString& m_vgNameValue;
-
+    QString m_volumeGroupName;
     qint64 m_totalSizeValue;
     qint64 m_usedSizeValue;
+    qint32 m_physicalExtentSize;
 };
 
 #endif  // VOLUMEGROUPBASEDIALOG_H

src/modules/partition/jobs/CreatePartitionJob.cpp

@@ -11,7 +11,9 @@
 
 #include "CreatePartitionJob.h"
 
+#include "core/KPMHelpers.h"
 #include "core/PartitionInfo.h"
+
 #include "partition/FileSystem.h"
 #include "partition/PartitionQuery.h"
 #include "utils/CalamaresUtilsSystem.h"
@@ -273,17 +275,9 @@ CreatePartitionJob::exec()
         return createZfs( m_partition, m_device );
     }
 
-    Report report( nullptr );
-    NewOperation op( *m_device, m_partition );
-    op.setStatus( Operation::StatusRunning );
-
-    QString message = tr( "The installer failed to create partition on disk '%1'." ).arg( m_device->name() );
-    if ( op.execute( report ) )
-    {
-        return Calamares::JobResult::ok();
-    }
-
-    return Calamares::JobResult::error( message, report.toText() );
+    return KPMHelpers::execute(
+        NewOperation( *m_device, m_partition ),
+        tr( "The installer failed to create partition on disk '%1'." ).arg( m_device->name() ) );
 }
 
 void

src/modules/partition/jobs/CreatePartitionTableJob.cpp

@@ -12,9 +12,11 @@
 #include "CreatePartitionTableJob.h"
 
 #include "partition/PartitionIterator.h"
+#include "utils/CalamaresUtilsSystem.h"
 #include "utils/Logger.h"
 
-// KPMcore
+#include "core/KPMHelpers.h"
+
 #include <kpmcore/core/device.h>
 #include <kpmcore/core/partition.h>
 #include <kpmcore/core/partitiontable.h>
@@ -63,8 +65,6 @@ CreatePartitionTableJob::prettyStatusMessage() const
 Calamares::JobResult
 CreatePartitionTableJob::exec()
 {
-    Report report( nullptr );
-    QString message = tr( "The installer failed to create a partition table on %1." ).arg( m_device->name() );
 
     PartitionTable* table = m_device->partitionTable();
 
@@ -76,30 +76,16 @@ CreatePartitionTableJob::exec()
             cDebug() << Logger::SubEntry << ( ( *it ) ? ( *it )->deviceNode() : QString( "<null device>" ) );
         }
 
-        QProcess lsblk;
-        lsblk.setProgram( "lsblk" );
-        lsblk.setProcessChannelMode( QProcess::MergedChannels );
-        lsblk.start();
-        lsblk.waitForFinished();
-        cDebug() << Logger::SubEntry << "lsblk output:\n" << Logger::NoQuote << lsblk.readAllStandardOutput();
+        auto lsblkResult = CalamaresUtils::System::runCommand( { "lsblk" }, std::chrono::seconds( 30 ) );
+        cDebug() << Logger::SubEntry << "lsblk output:\n" << Logger::NoQuote << lsblkResult.getOutput();
 
-        QProcess mount;
-        mount.setProgram( "mount" );  // Debug output only, not mounting something
-        mount.setProcessChannelMode( QProcess::MergedChannels );
-        mount.start();
-        mount.waitForFinished();
-        cDebug() << Logger::SubEntry << "mount output:\n" << Logger::NoQuote << mount.readAllStandardOutput();
+        auto mountResult = CalamaresUtils::System::runCommand( { "mount" }, std::chrono::seconds( 30 ) );
+        cDebug() << Logger::SubEntry << "mount output:\n" << Logger::NoQuote << mountResult.getOutput();
     }
 
-    CreatePartitionTableOperation op( *m_device, table );
-    op.setStatus( Operation::StatusRunning );
-
-    if ( op.execute( report ) )
-    {
-        return Calamares::JobResult::ok();
-    }
-
-    return Calamares::JobResult::error( message, report.toText() );
+    return KPMHelpers::execute(
+        CreatePartitionTableOperation( *m_device, table ),
+        tr( "The installer failed to create a partition table on %1." ).arg( m_device->name() ) );
 }
 
 void

src/modules/partition/jobs/CreateVolumeGroupJob.cpp

@@ -9,19 +9,20 @@
 
 #include "CreateVolumeGroupJob.h"
 
-// KPMcore
+#include "core/KPMHelpers.h"
+
 #include <kpmcore/core/lvmdevice.h>
 #include <kpmcore/core/partition.h>
 #include <kpmcore/ops/createvolumegroupoperation.h>
 #include <kpmcore/util/report.h>
 
 CreateVolumeGroupJob::CreateVolumeGroupJob( Device*,
-                                            QString& vgName,
-                                            QVector< const Partition* > pvList,
+                                            const QString& vgName,
+                                            const PartitionVector& pvList,
                                             const qint32 peSize )
     : m_vgName( vgName )
     , m_pvList( pvList )
-    , m_peSize( peSize )
+    , m_physicalExtentSize( peSize )
 {
 }
 
@@ -46,19 +47,8 @@ CreateVolumeGroupJob::prettyStatusMessage() const
 Calamares::JobResult
 CreateVolumeGroupJob::exec()
 {
-    Report report( nullptr );
-
-    CreateVolumeGroupOperation op( m_vgName, m_pvList, m_peSize );
-
-    op.setStatus( Operation::StatusRunning );
-
-    QString message = tr( "The installer failed to create a volume group named '%1'." ).arg( m_vgName );
-    if ( op.execute( report ) )
-    {
-        return Calamares::JobResult::ok();
-    }
-
-    return Calamares::JobResult::error( message, report.toText() );
+    return KPMHelpers::execute( CreateVolumeGroupOperation( m_vgName, m_pvList, m_physicalExtentSize ),
+                                tr( "The installer failed to create a volume group named '%1'." ).arg( m_vgName ) );
 }
 
 void

src/modules/partition/jobs/CreateVolumeGroupJob.h

@@ -10,19 +10,23 @@
 #ifndef CREATEVOLUMEGROUPJOB_H
 #define CREATEVOLUMEGROUPJOB_H
 
+#include "core/KPMHelpers.h"
+
 #include "Job.h"
 #include "partition/KPMManager.h"
 
-#include <QVector>
-
-class Device;
-class Partition;
-
 class CreateVolumeGroupJob : public Calamares::Job
 {
     Q_OBJECT
 public:
-    CreateVolumeGroupJob( Device*, QString& vgName, QVector< const Partition* > pvList, const qint32 peSize );
+    /** @brief Make a job that will create a volume group
+     *
+     * The @p physicalExtentSize is given in MiB; typically this is 4 (MiB).
+     */
+    CreateVolumeGroupJob( Device*,
+                          const QString& vgName,
+                          const PartitionVector& pvList,
+                          const qint32 physicalExtentSize );
 
     QString prettyName() const override;
     QString prettyDescription() const override;
@@ -35,8 +39,8 @@ public:
 private:
     CalamaresUtils::Partition::KPMManager m_kpmcore;
     QString m_vgName;
-    QVector< const Partition* > m_pvList;
-    qint32 m_peSize;
+    PartitionVector m_pvList;
+    qint32 m_physicalExtentSize;
 };
 
 #endif  // CREATEVOLUMEGROUPJOB_H

src/modules/partition/jobs/DeactivateVolumeGroupJob.cpp

@@ -9,6 +9,8 @@
 
 #include "DeactivateVolumeGroupJob.h"
 
+#include "core/KPMHelpers.h"
+
 #include <kpmcore/core/lvmdevice.h>
 #include <kpmcore/ops/deactivatevolumegroupoperation.h>
 #include <kpmcore/util/report.h>
@@ -39,18 +41,12 @@ DeactivateVolumeGroupJob::prettyStatusMessage() const
 Calamares::JobResult
 DeactivateVolumeGroupJob::exec()
 {
-    Report report( nullptr );
-
     DeactivateVolumeGroupOperation op( *m_device );
-
-    op.setStatus( Operation::OperationStatus::StatusRunning );
-
-    QString message = tr( "The installer failed to deactivate a volume group named %1." ).arg( m_device->name() );
-    if ( op.execute( report ) )
+    auto r = KPMHelpers::execute(
+        op, tr( "The installer failed to deactivate a volume group named %1." ).arg( m_device->name() ) );
+    if ( r )
     {
         op.preview();
-        return Calamares::JobResult::ok();
     }
-
-    return Calamares::JobResult::error( message, report.toText() );
+    return r;
 }

src/modules/partition/jobs/DeletePartitionJob.cpp

@@ -10,9 +10,11 @@
  */
 
 #include "DeletePartitionJob.h"
+
+#include "core/KPMHelpers.h"
+
 #include "utils/CalamaresUtilsSystem.h"
 
-// KPMcore
 #include <kpmcore/core/device.h>
 #include <kpmcore/core/partition.h>
 #include <kpmcore/core/partitiontable.h>
@@ -45,7 +47,7 @@ removePartition( Partition* partition )
     auto r = CalamaresUtils::System::instance()->runCommand(
         { "sfdisk", "--delete", "--force", partition->devicePath(), QString::number( partition->number() ) },
         std::chrono::seconds( 5 ) );
-    if ( r.getExitCode() !=0 || r.getOutput().contains("failed") )
+    if ( r.getExitCode() != 0 || r.getOutput().contains( "failed" ) )
     {
         return Calamares::JobResult::error(
             QCoreApplication::translate( DeletePartitionJob::staticMetaObject.className(), "Deletion Failed" ),
@@ -96,17 +98,8 @@ DeletePartitionJob::exec()
         return removePartition( m_partition );
     }
 
-    Report report( nullptr );
-    DeleteOperation op( *m_device, m_partition );
-    op.setStatus( Operation::StatusRunning );
-
-    QString message = tr( "The installer failed to delete partition %1." ).arg( m_partition->devicePath() );
-    if ( op.execute( report ) )
-    {
-        return Calamares::JobResult::ok();
-    }
-
-    return Calamares::JobResult::error( message, report.toText() );
+    return KPMHelpers::execute( DeleteOperation( *m_device, m_partition ),
+                                tr( "The installer failed to delete partition %1." ).arg( m_partition->devicePath() ) );
 }
 
 void

src/modules/partition/jobs/FormatPartitionJob.cpp

@@ -11,6 +11,8 @@
 
 #include "FormatPartitionJob.h"
 
+#include "core/KPMHelpers.h"
+
 #include "partition/FileSystem.h"
 #include "utils/Logger.h"
 
@@ -65,17 +67,7 @@ FormatPartitionJob::prettyStatusMessage() const
 Calamares::JobResult
 FormatPartitionJob::exec()
 {
-    Report report( nullptr );  // Root of the report tree, no parent
-    CreateFileSystemOperation op( *m_device, *m_partition, m_partition->fileSystem().type() );
-    op.setStatus( Operation::StatusRunning );
-
-    QString message = tr( "The installer failed to format partition %1 on disk '%2'." )
-                          .arg( m_partition->partitionPath(), m_device->name() );
-
-    if ( op.execute( report ) )
-    {
-        return Calamares::JobResult::ok();
-    }
-
-    return Calamares::JobResult::error( message, report.toText() );
+    return KPMHelpers::execute( CreateFileSystemOperation( *m_device, *m_partition, m_partition->fileSystem().type() ),
+                                tr( "The installer failed to format partition %1 on disk '%2'." )
+                                    .arg( m_partition->partitionPath(), m_device->name() ) );
 }

src/modules/partition/jobs/RemoveVolumeGroupJob.cpp

@@ -9,6 +9,8 @@
 
 #include "RemoveVolumeGroupJob.h"
 
+#include "core/KPMHelpers.h"
+
 #include <kpmcore/core/lvmdevice.h>
 #include <kpmcore/ops/removevolumegroupoperation.h>
 #include <kpmcore/util/report.h>
@@ -39,17 +41,7 @@ RemoveVolumeGroupJob::prettyStatusMessage() const
 Calamares::JobResult
 RemoveVolumeGroupJob::exec()
 {
-    Report report( nullptr );
-
-    RemoveVolumeGroupOperation op( *m_device );
-
-    op.setStatus( Operation::OperationStatus::StatusRunning );
-
-    QString message = tr( "The installer failed to remove a volume group named '%1'." ).arg( m_device->name() );
-    if ( op.execute( report ) )
-    {
-        return Calamares::JobResult::ok();
-    }
-
-    return Calamares::JobResult::error( message, report.toText() );
+    return KPMHelpers::execute(
+        RemoveVolumeGroupOperation( *m_device ),
+        tr( "The installer failed to remove a volume group named '%1'." ).arg( m_device->name() ) );
 }

src/modules/partition/jobs/ResizePartitionJob.cpp

@@ -11,9 +11,10 @@
 
 #include "ResizePartitionJob.h"
 
+#include "core/KPMHelpers.h"
+
 #include "utils/Units.h"
 
-// KPMcore
 #include <kpmcore/core/device.h>
 #include <kpmcore/ops/resizeoperation.h>
 #include <kpmcore/util/report.h>
@@ -66,23 +67,16 @@ ResizePartitionJob::prettyStatusMessage() const
 Calamares::JobResult
 ResizePartitionJob::exec()
 {
-    Report report( nullptr );
     // Restore partition sectors that were modified for preview
     m_partition->setFirstSector( m_oldFirstSector );
     m_partition->setLastSector( m_oldLastSector );
+
     ResizeOperation op( *m_device, *m_partition, m_newFirstSector, m_newLastSector );
-    op.setStatus( Operation::StatusRunning );
     connect( &op, &Operation::progress, this, &ResizePartitionJob::iprogress );
-
-    QString errorMessage = tr( "The installer failed to resize partition %1 on disk '%2'." )
-                               .arg( m_partition->partitionPath() )
-                               .arg( m_device->name() );
-    if ( op.execute( report ) )
-    {
-        return Calamares::JobResult::ok();
-    }
-
-    return Calamares::JobResult::error( errorMessage, report.toText() );
+    return KPMHelpers::execute( op,
+                                tr( "The installer failed to resize partition %1 on disk '%2'." )
+                                    .arg( m_partition->partitionPath() )
+                                    .arg( m_device->name() ) );
 }
 
 void

src/modules/partition/jobs/ResizeVolumeGroupJob.cpp

@@ -9,13 +9,12 @@
 
 #include "ResizeVolumeGroupJob.h"
 
-// KPMcore
 #include <kpmcore/core/lvmdevice.h>
 #include <kpmcore/core/partition.h>
 #include <kpmcore/ops/resizevolumegroupoperation.h>
 #include <kpmcore/util/report.h>
 
-ResizeVolumeGroupJob::ResizeVolumeGroupJob( Device*, LvmDevice* device, QVector< const Partition* >& partitionList )
+ResizeVolumeGroupJob::ResizeVolumeGroupJob( Device*, LvmDevice* device, const PartitionVector& partitionList )
     : m_device( device )
     , m_partitionList( partitionList )
 {
@@ -51,19 +50,9 @@ ResizeVolumeGroupJob::prettyStatusMessage() const
 Calamares::JobResult
 ResizeVolumeGroupJob::exec()
 {
-    Report report( nullptr );
-
-    ResizeVolumeGroupOperation op( *m_device, m_partitionList );
-
-    op.setStatus( Operation::OperationStatus::StatusRunning );
-
-    QString message = tr( "The installer failed to resize a volume group named '%1'." ).arg( m_device->name() );
-    if ( op.execute( report ) )
-    {
-        return Calamares::JobResult::ok();
-    }
-
-    return Calamares::JobResult::error( message, report.toText() );
+    return KPMHelpers::execute(
+        ResizeVolumeGroupOperation( *m_device, m_partitionList ),
+        tr( "The installer failed to resize a volume group named '%1'." ).arg( m_device->name() ) );
 }
 
 QString

src/modules/partition/jobs/ResizeVolumeGroupJob.h

@@ -10,20 +10,19 @@
 #ifndef RESIZEVOLUMEGROUPJOB_H
 #define RESIZEVOLUMEGROUPJOB_H
 
+#include "core/KPMHelpers.h"
+
 #include "Job.h"
 #include "partition/KPMManager.h"
 
-#include <QVector>
-
 class Device;
 class LvmDevice;
-class Partition;
 
 class ResizeVolumeGroupJob : public Calamares::Job
 {
     Q_OBJECT
 public:
-    ResizeVolumeGroupJob( Device*, LvmDevice* device, QVector< const Partition* >& partitionList );
+    ResizeVolumeGroupJob( Device*, LvmDevice* device, const PartitionVector& partitionList );
 
     QString prettyName() const override;
     QString prettyDescription() const override;
@@ -37,7 +36,7 @@ private:
 private:
     CalamaresUtils::Partition::KPMManager m_kpmcore;
     LvmDevice* m_device;
-    QVector< const Partition* > m_partitionList;
+    PartitionVector m_partitionList;
 };
 
 #endif  // RESIZEVOLUMEGROUPJOB_H

src/modules/partition/jobs/SetPartitionFlagsJob.cpp

@@ -13,6 +13,8 @@
 
 #include "SetPartitionFlagsJob.h"
 
+#include "core/KPMHelpers.h"
+
 #include "partition/FileSystem.h"
 #include "utils/Logger.h"
 #include "utils/Units.h"
@@ -148,17 +150,8 @@ SetPartFlagsJob::exec()
     cDebug() << "Setting flags on" << m_device->deviceNode() << "partition" << partition()->deviceNode()
              << Logger::DebugList( flagsList );
 
-    Report report( nullptr );
     SetPartFlagsOperation op( *m_device, *partition(), m_flags );
-    op.setStatus( Operation::StatusRunning );
     connect( &op, &Operation::progress, this, &SetPartFlagsJob::iprogress );
-
-    QString errorMessage
-        = tr( "The installer failed to set flags on partition %1." ).arg( m_partition->partitionPath() );
-    if ( op.execute( report ) )
-    {
-        return Calamares::JobResult::ok();
-    }
-
-    return Calamares::JobResult::error( errorMessage, report.toText() );
+    return KPMHelpers::execute(
+        op, tr( "The installer failed to set flags on partition %1." ).arg( m_partition->partitionPath() ) );
 }

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;
-        QString bare_dest = atReplacements( it.dest );
-        QString dest = prefix + bare_dest;
-
-        if ( it.type == ItemType::Log )
+        if ( !it )
         {
-            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 ) )
-            {
-                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;
-            }
+            ++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();
-    unsigned int c = 0;
-    for ( const auto& li : l )
+    for ( const auto& li : files.toList() )
     {
-        if ( li.type() == QVariant::String )
-        {
-            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;
+        m_items.push_back( Item::fromVariant( li, perm ) );
     }
 }
 

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>
-#include <QObject>
-#include <QVariantMap>
+class Item;
 
 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,42 +7,58 @@
 # 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
-#   is true, then these items are ignored (since the destination is the same
-#   as the source).
+#   as the same path within the target system (chroot). If, globally,
+#   *dontChroot* is true, then these items will be ignored (since the
+#   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
-#   are not recommended. There are three possible other keys in the map:
+#   target system (if the global *dontChroot* is true, then the host is the
+#   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:
-#   - `@@ROOT@@` is replaced by the path to the target root (may be /)
+# The target path (*dest*) is modified as follows:
+#   - `@@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:
-  - /etc/oem-information
   - from: log
-    dest: /root/install.log
-    perm: root:wheel:644
+    dest: /var/log/Calamares.log
+    perm: root:wheel:600
   - from: config
-    dest: /root/install.json
-    perm: root:wheel:400
+    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