Merge branch 'issue-1564' into calamares

FIXES #1564
FIXES #1817

Tested by doing an LVM installation from KDE Neon, as described
in #1817. Installation was successful, and machine booted
successfully afterwards.

src/modules/partition/core/PartitionCoreModule.cpp

@@ -580,6 +580,42 @@ PartitionCoreModule::setPartitionFlags( Device* device, Partition* partition, Pa
     PartitionInfo::setFlags( partition, flags );
 }
 
+STATICTEST QStringList
+findEssentialLVs( const QList< PartitionCoreModule::DeviceInfo* >& infos )
+{
+    QStringList doNotClose;
+    cDebug() << "Checking LVM use on" << infos.count() << "devices";
+    for ( const auto* info : infos )
+    {
+        if ( info->device->type() != Device::Type::LVM_Device )
+        {
+            continue;
+        }
+
+        for ( const auto& j : qAsConst( info->jobs() ) )
+        {
+            FormatPartitionJob* format = dynamic_cast< FormatPartitionJob* >( j.data() );
+            if ( format )
+            {
+                // device->deviceNode() is /dev/<vg name>
+                // partition()->partitionPath() is /dev/<vg name>/<lv>
+                const auto* partition = format->partition();
+                const QString partPath = partition->partitionPath();
+                const QString devicePath = info->device->deviceNode() + '/';
+                const bool isLvm = partition->roles().has( PartitionRole::Lvm_Lv );
+                if ( isLvm && partPath.startsWith( devicePath ) )
+                {
+                    cDebug() << Logger::SubEntry << partPath
+                             << "is an essential LV filesystem=" << partition->fileSystem().type();
+                    QString lvName = partPath.right( partPath.length() - devicePath.length() );
+                    doNotClose.append( info->device->name() + '-' + lvName );
+                }
+            }
+        }
+    }
+    return doNotClose;
+}
+
 Calamares::JobList
 PartitionCoreModule::jobs( const Config* config ) const
 {
@@ -604,15 +640,19 @@ PartitionCoreModule::jobs( const Config* config ) const
     lst << automountControl;
     lst << Calamares::job_ptr( new ClearTempMountsJob() );
 
-    for ( auto info : m_deviceInfos )
+    const QStringList doNotClose = findEssentialLVs( m_deviceInfos );
+
+    for ( const auto* info : m_deviceInfos )
     {
         if ( info->isDirty() )
         {
-            lst << Calamares::job_ptr( new ClearMountsJob( info->device.data() ) );
+            auto* job = new ClearMountsJob( info->device.data() );
+            job->setMapperExceptions( doNotClose );
+            lst << Calamares::job_ptr( job );
         }
     }
 
-    for ( auto info : m_deviceInfos )
+    for ( const auto* info : m_deviceInfos )
     {
         lst << info->jobs();
         devices << info->device.data();

src/modules/partition/core/PartitionCoreModule.h

@@ -84,6 +84,8 @@ public:
         PartitionModel* partitionModelAfter;
     };
 
+    struct DeviceInfo;
+
     PartitionCoreModule( QObject* parent = nullptr );
     ~PartitionCoreModule() override;
 
@@ -239,7 +241,6 @@ Q_SIGNALS:
     void deviceReverted( Device* device );
 
 private:
-    struct DeviceInfo;
     void refreshAfterModelChange();
 
     void doInit();

src/modules/partition/jobs/ClearMountsJob.cpp

@@ -23,34 +23,26 @@
 #include <kpmcore/core/partition.h>
 #include <kpmcore/util/report.h>
 
+#include <QCoreApplication>
 #include <QDir>
 #include <QProcess>
 #include <QStringList>
 
 using CalamaresUtils::Partition::PartitionIterator;
 
-ClearMountsJob::ClearMountsJob( Device* device )
-    : Calamares::Job()
-    , m_device( device )
-{
-}
 
-
+/** @brief Returns list of partitions on a given @p deviceName
-QString
+ *
-ClearMountsJob::prettyName() const
+ * The @p deviceName is a (whole-block) device, like "sda", and the partitions
-{
+ * returned are then "sdaX". The whole-block device itself is ignored, if
-    return tr( "Clear mounts for partitioning operations on %1" ).arg( m_device->deviceNode() );
+ * present. Partitions are returned with their full /dev/ path (e.g. /dev/sda1).
-}
+ *
-
+ * The format for /etc/partitions is, e.g.
-
+ *      major minor #blocks name
-QString
+ *      8     0     33554422 sda
-ClearMountsJob::prettyStatusMessage() const
+ *      8     1     33554400 sda1
-{
+ */
-    return tr( "Clearing mounts for partitioning operations on %1." ).arg( m_device->deviceNode() );
+STATICTEST QStringList
-}
-
-
-QStringList
 getPartitionsForDevice( const QString& deviceName )
 {
     QStringList partitions;
@@ -58,7 +50,7 @@ getPartitionsForDevice( const QString& deviceName )
     QFile dev_partitions( "/proc/partitions" );
     if ( dev_partitions.open( QFile::ReadOnly ) )
     {
-        cDebug() << "Reading from" << dev_partitions.fileName();
+        cDebug() << "Reading from" << dev_partitions.fileName() << "looking for" << deviceName;
         QTextStream in( &dev_partitions );
         (void)in.readLine();  // That's the header line, skip it
         while ( !in.atEnd() )
@@ -69,7 +61,7 @@ getPartitionsForDevice( const QString& deviceName )
             if ( ( columns.count() >= 4 ) && ( columns[ 3 ].startsWith( deviceName ) )
                  && ( columns[ 3 ] != deviceName ) )
             {
-                partitions.append( columns[ 3 ] );
+                partitions.append( QStringLiteral( "/dev/" ) + columns[ 3 ] );
             }
         }
     }
@@ -81,23 +73,16 @@ getPartitionsForDevice( const QString& deviceName )
     return partitions;
 }
 
-Calamares::JobResult
+STATICTEST QStringList
-ClearMountsJob::exec()
+getSwapsForDevice( const QString& deviceName )
 {
-    CalamaresUtils::Partition::Syncer s;
-
-    QString deviceName = m_device->deviceNode().split( '/' ).last();
-
-    QStringList goodNews;
     QProcess process;
 
-    QStringList partitionsList = getPartitionsForDevice( deviceName );
-
     // Build a list of partitions of type 82 (Linux swap / Solaris).
     // We then need to clear them just in case they contain something resumable from a
     // previous suspend-to-disk.
     QStringList swapPartitions;
-    process.start( "sfdisk", { "-d", m_device->deviceNode() } );
+    process.start( "sfdisk", { "-d", deviceName } );
     process.waitForFinished();
     // Sample output:
     //    % sudo sfdisk -d /dev/sda
@@ -116,40 +101,77 @@ ClearMountsJob::exec()
         *it = ( *it ).simplified().split( ' ' ).first();
     }
 
-    const QStringList cryptoDevices = getCryptoDevices();
+    return swapPartitions;
-    for ( const QString& mapperPath : cryptoDevices )
+}
+
+static inline bool
+isControl( const QString& baseName )
+{
+    return baseName == "control";
+}
+
+static inline bool
+isFedoraSpecial( const QString& baseName )
+{
+    // Fedora live images use /dev/mapper/live-* internally. We must not
+    // unmount those devices, because they are used by the live image and
+    // because we need /dev/mapper/live-base in the unpackfs module.
+    return baseName.startsWith( "live-" );
+}
+
+/** @brief Returns a list of unneeded crypto devices
+ *
+ * These are the crypto devices to unmount and close; some are "needed"
+ * for system operation: on Fedora, the live- mappers are special.
+ * Some other devices are special, too, so those do not end up in
+ * the list.
+ */
+STATICTEST QStringList
+getCryptoDevices( const QStringList& mapperExceptions )
+{
+    QDir mapperDir( "/dev/mapper" );
+    const QFileInfoList fiList = mapperDir.entryInfoList( QDir::Files );
+    QStringList list;
+    for ( const QFileInfo& fi : fiList )
     {
-        tryUmount( mapperPath );
+        QString baseName = fi.baseName();
-        QString news = tryCryptoClose( mapperPath );
+        if ( isControl( baseName ) || isFedoraSpecial( baseName ) || mapperExceptions.contains( baseName ) )
-        if ( !news.isEmpty() )
         {
-            goodNews.append( news );
+            continue;
         }
+        list.append( fi.absoluteFilePath() );
     }
+    return list;
+}
+
+STATICTEST QStringList
+getLVMVolumes()
+{
+    QProcess process;
 
     // First we umount all LVM logical volumes we can find
     process.start( "lvscan", { "-a" } );
     process.waitForFinished();
     if ( process.exitCode() == 0 )  //means LVM2 tools are installed
     {
-        const QStringList lvscanLines = QString::fromLocal8Bit( process.readAllStandardOutput() ).split( '\n' );
+        QStringList lvscanLines = QString::fromLocal8Bit( process.readAllStandardOutput() ).split( '\n' );
-        for ( const QString& lvscanLine : lvscanLines )
+        // Get the second column (`value(1)`) sinec that is the device name,
-        {
+        // remove quoting.
-            QString lvPath = lvscanLine.simplified().split( ' ' ).value( 1 );  //second column
+        std::transform( lvscanLines.begin(), lvscanLines.end(), lvscanLines.begin(), []( const QString& lvscanLine ) {
-            lvPath = lvPath.replace( '\'', "" );
+            return lvscanLine.simplified().split( ' ' ).value( 1 ).replace( '\'', "" );
-
+        } );
-            QString news = tryUmount( lvPath );
+        return lvscanLines;
-            if ( !news.isEmpty() )
-            {
-                goodNews.append( news );
-            }
-        }
     }
     else
     {
         cWarning() << "this system does not seem to have LVM2 tools.";
     }
-
+    return QStringList();
+}
+STATICTEST QStringList
+getPVGroups( const QString& deviceName )
+{
+    QProcess process;
     // Then we go looking for volume groups that use this device for physical volumes
     process.start( "pvdisplay", { "-C", "--noheadings" } );
     process.waitForFinished();
@@ -172,88 +194,84 @@ ClearMountsJob::exec()
 
                 vgSet.insert( vgName );
             }
-
+            return QStringList { vgSet.cbegin(), vgSet.cend() };
-            foreach ( const QString& vgName, vgSet )
-            {
-                process.start( "vgchange", { "-an", vgName } );
-                process.waitForFinished();
-                if ( process.exitCode() == 0 )
-                {
-                    goodNews.append( QString( "Successfully disabled volume group %1." ).arg( vgName ) );
-                }
-            }
         }
     }
     else
     {
         cWarning() << "this system does not seem to have LVM2 tools.";
     }
+    return QStringList();
+}
 
-    const QStringList cryptoDevices2 = getCryptoDevices();
+/*
-    for ( const QString& mapperPath : cryptoDevices2 )
+ * The tryX() free functions, below, return an empty QString on
+ * failure, or a non-empty QString on success. The string is
+ * meant **only** for debugging and is not displayed to the user,
+ * which is why no translation is applied.
+ *
+ */
+
+class MessageAndPath
+{
+public:
+    ///@brief An unsuccessful attempt at something
+    MessageAndPath() {}
+    ///@brief A success at doing @p thing to @p path
+    MessageAndPath( const char* thing, const QString& path )
+        : m_message( thing )
+        , m_path( path )
     {
-        tryUmount( mapperPath );
-        QString news = tryCryptoClose( mapperPath );
-        if ( !news.isEmpty() )
-        {
-            goodNews.append( news );
-        }
     }
 
-    for ( const QString& p : partitionsList )
+    bool isEmpty() const { return !m_message; }
-    {
-        QString partPath = QString( "/dev/%1" ).arg( p );
 
-        QString news = tryUmount( partPath );
+    explicit operator QString() const
-        if ( !news.isEmpty() )
+    {
-        {
+        return isEmpty() ? QString() : QCoreApplication::translate( "ClearMountsJob", m_message ).arg( m_path );
-            goodNews.append( news );
-        }
     }
 
-    foreach ( QString p, swapPartitions )
+private:
+    const char* const m_message = nullptr;
+    QString const m_path;
+};
+
+STATICTEST inline QDebug&
+operator<<( QDebug& s, const MessageAndPath& m )
+{
+    if ( m.isEmpty() )
     {
-        QString news = tryClearSwap( p );
+        return s;
-        if ( !news.isEmpty() )
-        {
-            goodNews.append( news );
-        }
     }
-
+    return s << QString( m );
-    Calamares::JobResult ok = Calamares::JobResult::ok();
-    ok.setMessage( tr( "Cleared all mounts for %1" ).arg( m_device->deviceNode() ) );
-    ok.setDetails( goodNews.join( "\n" ) );
-
-    cDebug() << "ClearMountsJob finished. Here's what was done:\n" << goodNews.join( "\n" );
-
-    return ok;
 }
 
 
-QString
+///@brief Returns a debug-string if @p partPath could be unmounted
-ClearMountsJob::tryUmount( const QString& partPath )
+STATICTEST MessageAndPath
+tryUmount( const QString& partPath )
 {
     QProcess process;
     process.start( "umount", { partPath } );
     process.waitForFinished();
     if ( process.exitCode() == 0 )
     {
-        return QString( "Successfully unmounted %1." ).arg( partPath );
+        return { QT_TRANSLATE_NOOP( "ClearMountsJob", "Successfully unmounted %1." ), partPath };
     }
 
     process.start( "swapoff", { partPath } );
     process.waitForFinished();
     if ( process.exitCode() == 0 )
     {
-        return QString( "Successfully disabled swap %1." ).arg( partPath );
+        return { QT_TRANSLATE_NOOP( "ClearMountsJob", "Successfully disabled swap %1." ), partPath };
     }
 
-    return QString();
+    return {};
 }
 
-
+///@brief Returns a debug-string if @p partPath was swap and could be cleared
-QString
+STATICTEST MessageAndPath
-ClearMountsJob::tryClearSwap( const QString& partPath )
+tryClearSwap( const QString& partPath )
 {
     QProcess process;
     process.start( "blkid", { "-s", "UUID", "-o", "value", partPath } );
@@ -261,53 +279,110 @@ ClearMountsJob::tryClearSwap( const QString& partPath )
     QString swapPartUuid = QString::fromLocal8Bit( process.readAllStandardOutput() ).simplified();
     if ( process.exitCode() != 0 || swapPartUuid.isEmpty() )
     {
-        return QString();
+        return {};
     }
 
     process.start( "mkswap", { "-U", swapPartUuid, partPath } );
     process.waitForFinished();
     if ( process.exitCode() != 0 )
     {
-        return QString();
+        return {};
     }
 
-    return QString( "Successfully cleared swap %1." ).arg( partPath );
+    return { QT_TRANSLATE_NOOP( "ClearMountsJob", "Successfully cleared swap %1." ), partPath };
 }
 
-
+///@brief Returns a debug-string if @p mapperPath could be closed
-QString
+STATICTEST MessageAndPath
-ClearMountsJob::tryCryptoClose( const QString& mapperPath )
+tryCryptoClose( const QString& mapperPath )
 {
+    /* ignored */ tryUmount( mapperPath );
+
     QProcess process;
     process.start( "cryptsetup", { "close", mapperPath } );
     process.waitForFinished();
     if ( process.exitCode() == 0 )
     {
-        return QString( "Successfully closed mapper device %1." ).arg( mapperPath );
+        return { QT_TRANSLATE_NOOP( "ClearMountsJob", "Successfully closed mapper device %1." ), mapperPath };
     }
 
-    return QString();
+    return {};
 }
 
-
+STATICTEST MessageAndPath
-QStringList
+tryVGDisable( const QString& vgName )
-ClearMountsJob::getCryptoDevices() const
 {
-    QDir mapperDir( "/dev/mapper" );
+    QProcess vgProcess;
-    const QFileInfoList fiList = mapperDir.entryInfoList( QDir::Files );
+    vgProcess.start( "vgchange", { "-an", vgName } );
-    QStringList list;
+    vgProcess.waitForFinished();
-    QProcess process;
+    return ( vgProcess.exitCode() == 0 )
-    for ( const QFileInfo& fi : fiList )
+        ? MessageAndPath { QT_TRANSLATE_NOOP( "ClearMountsJob", "Successfully disabled volume group %1." ), vgName }
-    {
+        : MessageAndPath {};
-        QString baseName = fi.baseName();
+}
-        // Fedora live images use /dev/mapper/live-* internally. We must not
+
-        // unmount those devices, because they are used by the live image and
+///@brief Apply @p f to all the @p paths, appending successes to @p news
-        // because we need /dev/mapper/live-base in the unpackfs module.
+template < typename F >
-        if ( baseName == "control" || baseName.startsWith( "live-" ) )
+void
-        {
+apply( const QStringList& paths, F f, QList< MessageAndPath >& news )
-            continue;
+{
-        }
+    for ( const QString& p : qAsConst( paths ) )
-        list.append( fi.absoluteFilePath() );
+    {
-    }
+        auto n = f( p );
-    return list;
+        if ( !n.isEmpty() )
+        {
+            news.append( n );
+        }
+    }
+}
+
+STATICTEST QStringList
+stringify( const QList< MessageAndPath >& news )
+{
+    QStringList l;
+    for ( const auto& m : qAsConst( news ) )
+    {
+        l << QString( m );
+    }
+    return l;
+}
+
+ClearMountsJob::ClearMountsJob( Device* device )
+    : Calamares::Job()
+    , m_deviceNode( device->deviceNode() )
+{
+}
+
+QString
+ClearMountsJob::prettyName() const
+{
+    return tr( "Clear mounts for partitioning operations on %1" ).arg( m_deviceNode );
+}
+
+QString
+ClearMountsJob::prettyStatusMessage() const
+{
+    return tr( "Clearing mounts for partitioning operations on %1." ).arg( m_deviceNode );
+}
+
+Calamares::JobResult
+ClearMountsJob::exec()
+{
+    const QString deviceName = m_deviceNode.split( '/' ).last();
+    CalamaresUtils::Partition::Syncer s;
+    QList< MessageAndPath > goodNews;
+
+    apply( getCryptoDevices( m_mapperExceptions ), tryCryptoClose, goodNews );
+    apply( getLVMVolumes(), tryUmount, goodNews );
+    apply( getPVGroups( deviceName ), tryVGDisable, goodNews );
+
+    apply( getCryptoDevices( m_mapperExceptions ), tryCryptoClose, goodNews );
+    apply( getPartitionsForDevice( deviceName ), tryUmount, goodNews );
+    apply( getSwapsForDevice( m_deviceNode ), tryClearSwap, goodNews );
+
+    Calamares::JobResult ok = Calamares::JobResult::ok();
+    ok.setMessage( tr( "Cleared all mounts for %1" ).arg( m_deviceNode ) );
+    ok.setDetails( stringify( goodNews ).join( "\n" ) );
+    cDebug() << "ClearMountsJob finished. Here's what was done:" << Logger::DebugListT< MessageAndPath >( goodNews );
+
+    return ok;
 }

src/modules/partition/jobs/ClearMountsJob.h

@@ -17,22 +17,43 @@ class Device;
 /**
  * This job tries to free all mounts for the given device, so partitioning
  * operations can proceed.
+ *
+ * - partitions on the device are unmounted
+ * - swap on the device is disabled and cleared
+ * - physical volumes for LVM on the device are disabled
+ *
+ * In addition, regardless of device:
+ * - almost all(*) /dev/mapper entries (crypto / LUKS, also LVM) are closed
+ * - all logical volumes for LVM are unmounted
+ * Exceptions to "all /dev/mapper" may be configured through
+ * the setMapperExceptions() method. Pass in names of mapper
+ * files that should not be closed (e.g. "myvg-mylv").
+ *
+ * (*) Some exceptions always exist: /dev/mapper/control is never
+ *     closed. /dev/mapper/live-* is never closed.
+ *
  */
 class ClearMountsJob : public Calamares::Job
 {
     Q_OBJECT
 public:
+    /** @brief Creates a job freeing mounts on @p device
+     *
+     * No ownership is transferred; the @p device is used only to access
+     * the device node (name).
+     */
     explicit ClearMountsJob( Device* device );
+
     QString prettyName() const override;
     QString prettyStatusMessage() const override;
     Calamares::JobResult exec() override;
 
+    ///@brief Sets the list of exceptions (names) when closing /dev/mapper
+    void setMapperExceptions( const QStringList& names ) { m_mapperExceptions = names; }
+
 private:
-    QString tryUmount( const QString& partPath );
+    const QString m_deviceNode;
-    QString tryClearSwap( const QString& partPath );
+    QStringList m_mapperExceptions;
-    QString tryCryptoClose( const QString& mapperPath );
-    QStringList getCryptoDevices() const;
-    Device* m_device;
 };
 
 #endif  // CLEARMOUNTSJOB_H