[zfs][mount] Refactor zfs dataset mounting logic

src/modules/mount/main.py

@@ -72,20 +72,6 @@ def get_btrfs_subvolumes(partitions):
     return btrfs_subvolumes
 
 
-def bool_to_zfs_command(gs_value):
-    """ Something in the chain is converting on and off to true and false.  This converts it back.
-
-    :param gs_value: The value from global storage which needs to be fixed
-    :return:
-    """
-    if gs_value is True:
-        return "on"
-    elif gs_value is False:
-        return "off"
-    else:
-        return gs_value
-
-
 def mount_zfs(root_mount_point, partition):
     """ Mounts a zfs partition at @p root_mount_point
 
@@ -107,7 +93,7 @@ def mount_zfs(root_mount_point, partition):
 
     # import the zpool
     try:
-        libcalamares.utils.host_env_process_output(["zpool", "import", "-R", root_mount_point, pool_name], None)
+        libcalamares.utils.host_env_process_output(["zpool", "import", "-N", "-R", root_mount_point, pool_name], None)
     except subprocess.CalledProcessError:
         raise ZfsException(_("Failed to import zpool"))
 
@@ -123,8 +109,7 @@ def mount_zfs(root_mount_point, partition):
     if encrypt is True:
         # The zpool is encrypted, we need to unlock it
         try:
-            libcalamares.utils.host_env_process_output(["sh", "-c",
+            libcalamares.utils.host_env_process_output(["zfs", "load-key", pool_name], None, passphrase)
-                                                        "echo \"" + passphrase + "\" | zfs load-key " + pool_name], None)
         except subprocess.CalledProcessError:
             raise ZfsException(_("Failed to unlock zpool"))
 
@@ -136,35 +121,17 @@ def mount_zfs(root_mount_point, partition):
             libcalamares.utils.warning("Failed to locate zfs dataset list")
             raise ZfsException(_("Internal error mounting zfs datasets"))
 
-        # first we handle the / dataset if there is one
+        zfs.sort(key=lambda x: x["mountpoint"])
         for dataset in zfs:
-            if dataset['mountpoint'] == '/':
-                # Properly set the canmount field from global storage
-                can_mount = bool_to_zfs_command(dataset['canMount'])
             try:
-                    libcalamares.utils.host_env_process_output(["zfs", "set", "canmount=" + can_mount,
+                if dataset["canMount"] == "noauto" or dataset["canMount"] is True:
-                                                                dataset["zpool"] + "/" + dataset["dsName"]], None)
+                    libcalamares.utils.host_env_process_output(["zfs", "mount",
-                except subprocess.CalledProcessError:
+                                                                dataset["zpool"] + '/' + dataset["dsName"]])
-                    raise ZfsException(_("Failed to set zfs mountpoint"))
-                if dataset["canMount"] == "noauto":
-                    mount_result = subprocess.run(["zfs", "mount", dataset["zpool"] + '/' + dataset["dsName"]])
-                    if mount_result.returncode != 0:
-                        raise ZfsException(_("Failed to mount root dataset"))
-
-        # Set the canmount property for each dataset.  This will effectively mount the dataset
-        for dataset in zfs:
-            # We already handled the / mountpoint above
-            if dataset["mountpoint"] != '/':
-                can_mount = bool_to_zfs_command(dataset["canMount"])
-
-                try:
-                    libcalamares.utils.host_env_process_output(["zfs", "set", "canmount=" + can_mount,
-                                                                dataset["zpool"] + "/" + dataset["dsName"]], None)
             except subprocess.CalledProcessError:
                 raise ZfsException(_("Failed to set zfs mountpoint"))
     else:
         try:
-            libcalamares.utils.host_env_process_output(["zfs", "set", "canmount=on", pool_name + "/" + ds_name], None)
+            libcalamares.utils.host_env_process_output(["zfs", "mount", pool_name + '/' + ds_name])
         except subprocess.CalledProcessError:
             raise ZfsException(_("Failed to set zfs mountpoint"))
 

src/modules/zfs/ZfsJob.cpp

@@ -39,7 +39,7 @@ alphaNumeric( QString input )
  * 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
+ * The function returns a fully qualified path to the device or an empty string if no device
  * is found
  *
  * @p pMap is the partition map from global storage
@@ -64,6 +64,30 @@ findBestZfsDevice( QVariantMap pMap )
     }
 }
 
+/** @brief Converts the value in a QVariant to a string which is a valid option for canmount
+ *
+ * Storing "on" and "off" in QVariant results in a conversion to boolean.  This function takes
+ * the Qvariant in @p canMount and converts it to a QString holding "on", "off" or the string
+ * value in the QVariant.
+ *
+ */
+static QString
+convertCanMount( QVariant canMount )
+{
+    if ( canMount == true )
+    {
+        return "on";
+    }
+    else if ( canMount == false )
+    {
+        return "off";
+    }
+    else
+    {
+        return canMount.toString();
+    }
+}
+
 ZfsJob::ZfsJob( QObject* parent )
     : Calamares::CppJob( parent )
 {
@@ -263,12 +287,12 @@ ZfsJob::exec()
                     continue;
                 }
 
-                // Create the dataset.  We set canmount=no regardless of the setting for now.
+                QString canMount = convertCanMount( datasetMap[ "canMount" ].toString() );
-                // It is modified to the correct value in the mount module to ensure mount order is maintained
+
+                // Create the dataset
                 auto r = system->runCommand( { QStringList() << "zfs"
                                                              << "create" << m_datasetOptions.split( ' ' ) << "-o"
-                                                             << "canmount=off"
+                                                             << "canmount=" + canMount << "-o"
-                                                             << "-o"
                                                              << "mountpoint=" + datasetMap[ "mountpoint" ].toString()
                                                              << poolName + "/" + datasetMap[ "dsName" ].toString() },
                                              std::chrono::seconds( 10 ) );
@@ -292,13 +316,11 @@ ZfsJob::exec()
         }
         else
         {
-            // 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 );
             auto r = system->runCommand( { QStringList() << "zfs"
                                                          << "create" << m_datasetOptions.split( ' ' ) << "-o"
-                                                         << "canmount=off"
+                                                         << "canmount=on"
                                                          << "-o"
                                                          << "mountpoint=" + mountpoint << poolName + "/" + dsName },
                                          std::chrono::seconds( 10 ) );