[zfs] Cleanup code based on review feedback

src/modules/zfs/ZfsJob.cpp

@@ -19,6 +19,51 @@
 
 #include <QProcess>
 
+#include <unistd.h>
+
+/** @brief Returns the alphanumeric portion of a string
+ *
+ * @p input is the input string
+ *
+ */
+static QString
+alphaNumeric( QString input )
+{
+    return input.remove( QRegExp( "[^a-zA-Z\\d\\s]" ) );
+}
+
+/** @brief Returns the best available device for zpool creation
+ *
+ * zfs partitions generally don't have UUID until the zpool is created. Generally,
+ * they are formed using either the id or the partuuid.  The id isn't stored by kpmcore
+ * so this function checks to see if we have a partuuid.  If so, it forms a device path
+ * for it.  As a backup, it uses the device name i.e. /dev/sdax.
+ *
+ * The function returns a fullt qualified path to the device or an empty string if no device
+ * is found
+ *
+ * @p pMap is the partition map from global storage
+ *
+ */
+static QString
+findBestZfsDevice( QVariantMap pMap )
+{
+    // Find the best device identifier, if one isn't available, skip this partition
+    QString deviceName;
+    if ( pMap[ "partuuid" ].toString() != "" )
+    {
+        return "/dev/disk/by-partuuid/" + pMap[ "partuuid" ].toString().toLower();
+    }
+    else if ( pMap[ "device" ].toString() != "" )
+    {
+        return pMap[ "device" ].toString().toLower();
+    }
+    else
+    {
+        return QString();
+    }
+}
+
 ZfsJob::ZfsJob( QObject* parent )
     : Calamares::CppJob( parent )
 {
@@ -32,14 +77,8 @@ ZfsJob::prettyName() const
     return tr( "Create ZFS pools and datasets" );
 }
 
-QString
-ZfsJob::AlphaNumeric( QString input ) const
-{
-    return input.remove( QRegExp( "[^a-zA-Z\\d\\s]" ) );
-}
-
 void
-ZfsJob::CollectMountpoints( const QVariantList& partitions )
+ZfsJob::collectMountpoints( const QVariantList& partitions )
 {
     m_mountpoints.empty();
     for ( const QVariant& partition : partitions )
@@ -56,7 +95,7 @@ ZfsJob::CollectMountpoints( const QVariantList& partitions )
 }
 
 bool
-ZfsJob::IsMountpointOverlapping( const QString& targetMountpoint ) const
+ZfsJob::isMountpointOverlapping( const QString& targetMountpoint ) const
 {
     for ( const QString& mountpoint : m_mountpoints )
     {
@@ -70,49 +109,32 @@ ZfsJob::IsMountpointOverlapping( const QString& targetMountpoint ) const
 
 
 ZfsResult
-ZfsJob::CreateZpool( QString deviceName, QString poolName, QString poolOptions, bool encrypt, QString passphrase ) const
+ZfsJob::createZpool( QString deviceName, QString poolName, QString poolOptions, bool encrypt, QString passphrase ) const
 {
     // zfs doesn't wait for the devices so pause for 2 seconds to ensure we give time for the device files to be created
-    QString command;
+    sleep( 2 );
+
+    QStringList command;
     if ( encrypt )
     {
-        command = "sleep 2 ; echo \"" + passphrase + "\" | zpool create " + poolOptions
+        command = QStringList() << "zpool"
-            + " -O encryption=aes-256-gcm -O keyformat=passphrase " + poolName + " " + deviceName;
+                                << "create" << poolOptions.split( ' ' ) << "-O"
+                                << "encryption=aes-256-gcm"
+                                << "-O"
+                                << "keyformat=passphrase" << poolName << deviceName;
     }
     else
     {
-        command = "sleep 2 ; zpool create " + poolOptions + " " + poolName + " " + deviceName;
+        command = QStringList() << "zpool"
+                                << "create" << poolOptions.split( ' ' ) << poolName << deviceName;
     }
 
-    // We use a qProcess instead of runCommand so the password will not end up in the logs
+    auto r = CalamaresUtils::System::instance()->runCommand(
-    QProcess process;
+        CalamaresUtils::System::RunLocation::RunInHost, command, QString(), passphrase, std::chrono::seconds( 10 ) );
 
-    process.setProcessChannelMode( QProcess::MergedChannels );
+    if ( r.getExitCode() != 0 )
-    cDebug() << Logger::SubEntry << "Running zpool create";
-
-    process.start( "sh", QStringList() << "-c" << command );
-
-    if ( !process.waitForStarted() )
     {
-        return { false, tr( "zpool create process failed to start" ) };
+        cWarning() << "Failed to run zpool create.  The output was: " + r.getOutput();
-    }
-
-    if ( !process.waitForFinished( 5000 ) )
-    {
-        return { false, tr( "Process for zpool create timed out" ) };
-    }
-
-    QString output = QString::fromLocal8Bit( process.readAllStandardOutput() ).trimmed();
-
-    if ( process.exitStatus() == QProcess::CrashExit )
-    {
-        return { false, tr( "The output from the crash was: " ) + output };
-    }
-
-    auto exitcode = process.exitCode();
-    if ( exitcode != 0 )
-    {
-        cWarning() << "Failed to run zpool create.  The output was: " + output;
         return { false, tr( "Failed to create zpool on " ) + deviceName };
     }
 
@@ -140,6 +162,13 @@ ZfsJob::exec()
 
     QVariantList poolNames;
 
+    // Check to ensure the list of zfs info from the partition module is available and convert it to a list
+    if ( !gs->contains( "zfsInfo" ) && gs->value( "zfsInfo" ).canConvert( QVariant::List ) )
+    {
+        return Calamares::JobResult::error( tr( "Internal data missing" ), tr( "Failed to create zpool" ) );
+    }
+    QVariantList zfsInfoList = gs->value( "zfsInfo" ).toList();
+
     for ( auto& partition : qAsConst( partitions ) )
     {
         QVariantMap pMap;
@@ -155,16 +184,8 @@ ZfsJob::exec()
         }
 
         // Find the best device identifier, if one isn't available, skip this partition
-        QString deviceName;
+        QString deviceName = findBestZfsDevice( pMap );
-        if ( pMap[ "partuuid" ].toString() != "" )
+        if ( deviceName.isEmpty() )
-        {
-            deviceName = "/dev/disk/by-partuuid/" + pMap[ "partuuid" ].toString().toLower();
-        }
-        else if ( pMap[ "device" ].toString() != "" )
-        {
-            deviceName = pMap[ "device" ].toString().toLower();
-        }
-        else
         {
             continue;
         }
@@ -180,16 +201,9 @@ ZfsJob::exec()
         QString poolName = m_poolName;
         if ( mountpoint != '/' )
         {
-            poolName += AlphaNumeric( mountpoint );
+            poolName += alphaNumeric( mountpoint );
         }
 
-        // Check to ensure the list of zfs info from the partition module is available and convert it to a list
-        if ( !gs->contains( "zfsInfo" ) && gs->value( "zfsInfo" ).canConvert( QVariant::List ) )
-        {
-            return Calamares::JobResult::error( tr( "Internal data missing" ), tr( "Failed to create zpool" ) );
-        }
-        QVariantList zfsInfoList = gs->value( "zfsInfo" ).toList();
-
         // Look in the zfs info list to see if this partition should be encrypted
         bool encrypt = false;
         QString passphrase;
@@ -207,11 +221,11 @@ ZfsJob::exec()
         ZfsResult zfsResult;
         if ( encrypt )
         {
-            zfsResult = CreateZpool( deviceName, poolName, m_poolOptions, true, passphrase );
+            zfsResult = createZpool( deviceName, poolName, m_poolOptions, true, passphrase );
         }
         else
         {
-            zfsResult = CreateZpool( deviceName, poolName, m_poolOptions, false );
+            zfsResult = createZpool( deviceName, poolName, m_poolOptions, false );
         }
 
         if ( !zfsResult.success )
@@ -229,7 +243,7 @@ ZfsJob::exec()
         // If the mountpoint is /, create datasets per the config file. If not, create a single dataset mounted at the partitions mountpoint
         if ( mountpoint == '/' )
         {
-            CollectMountpoints( partitions );
+            collectMountpoints( partitions );
             QVariantList datasetList;
             for ( const auto& dataset : qAsConst( m_datasets ) )
             {
@@ -244,18 +258,19 @@ ZfsJob::exec()
                 }
 
                 // We should skip this dataset if it conflicts with a permanent mountpoint
-                if ( IsMountpointOverlapping( datasetMap[ "mountpoint" ].toString() ) )
+                if ( isMountpointOverlapping( datasetMap[ "mountpoint" ].toString() ) )
                 {
                     continue;
                 }
 
                 // Create the dataset.  We set canmount=no regardless of the setting for now.
                 // It is modified to the correct value in the mount module to ensure mount order is maintained
-                auto r = system->runCommand( { "sh",
+                auto r = system->runCommand( { QStringList() << "zfs"
-                                               "-c",
+                                                             << "create" << m_datasetOptions.split( ' ' ) << "-o"
-                                               "zfs create " + m_datasetOptions + " -o canmount=off -o mountpoint="
+                                                             << "canmount=off"
-                                                   + datasetMap[ "mountpoint" ].toString() + " " + poolName + "/"
+                                                             << "-o"
-                                                   + datasetMap[ "dsName" ].toString() },
+                                                             << "mountpoint=" + datasetMap[ "mountpoint" ].toString()
+                                                             << poolName + "/" + datasetMap[ "dsName" ].toString() },
                                              std::chrono::seconds( 10 ) );
                 if ( r.getExitCode() != 0 )
                 {
@@ -280,15 +295,17 @@ ZfsJob::exec()
             // This is a zpool with a single dataset We again set canmount=no regardless of the desired setting.
             // It is modified to the correct value in the mount module to ensure mount order is maintained
             QString dsName = mountpoint;
-            dsName = AlphaNumeric( mountpoint );
+            dsName = alphaNumeric( mountpoint );
-            auto r = system->runCommand( { "sh",
+            auto r = system->runCommand( { QStringList() << "zfs"
-                                           "-c",
+                                                         << "create" << m_datasetOptions.split( ' ' ) << "-o"
-                                           "zfs create " + m_datasetOptions + " -o canmount=off -o mountpoint="
+                                                         << "canmount=off"
-                                               + mountpoint + " " + poolName + "/" + dsName },
+                                                         << "-o"
+                                                         << "mountpoint=" + mountpoint << poolName + "/" + dsName },
                                          std::chrono::seconds( 10 ) );
             if ( r.getExitCode() != 0 )
             {
-                cWarning() << "Failed to create dataset" << dsName;
+                return Calamares::JobResult::error( tr( "Failed to create dataset" ),
+                                                    tr( "The output was: " ) + r.getOutput() );
             }
             poolNameEntry[ "dsName" ] = dsName;
         }

src/modules/zfs/ZfsJob.h

@@ -23,7 +23,7 @@
 struct ZfsResult
 {
     bool success;
-    QString failureMessage;
+    QString failureMessage;  // This message is displayed to the user and should be translated at the time of population
 };
 
 /** @brief Create zpools and zfs datasets
@@ -60,26 +60,19 @@ private:
      * @p passphrase is a string continaing the passphrase
      *
      */
-    ZfsResult CreateZpool( QString deviceName,
+    ZfsResult createZpool( QString deviceName,
                            QString poolName,
                            QString poolOptions,
                            bool encrypt,
                            QString passphrase = QString() ) const;
 
-    /** @brief Returns the alphanumeric portion of a string
-     *
-     * @p input is the input string
-     *
-     */
-    QString AlphaNumeric( QString input ) const;
-
     /** @brief Collects all the mountpoints from the partitions
      *
      * Iterates over @p partitions to gather each mountpoint present
      * in the list of maps and populates m_mountpoints
      *
      */
-    void CollectMountpoints( const QVariantList& partitions );
+    void collectMountpoints( const QVariantList& partitions );
 
     /** @brief Check to see if a given mountpoint overlaps with one of the defined moutnpoints
      *
@@ -88,7 +81,7 @@ private:
      * since all the mountpoints would begin with /
      *
      */
-    bool IsMountpointOverlapping( const QString& targetMountpoint ) const;
+    bool isMountpointOverlapping( const QString& targetMountpoint ) const;
 };
 
 CALAMARES_PLUGIN_FACTORY_DECLARATION( ZfsJobFactory )