Don't unmount the Nix store when doing a curing reinstall, plus other…

… nits and cleanup (#1299)

* Don't fail to delete a macos user that doesn't exist

* Deduplicate the diskutil info collection

* Don't unmount the Nix volume if it is already the correct volume, mounted at the correct place

* Fixup clippy

* Don't panic if the nix store doesn't exist yet, duh

* Only skip the unmount step if the create volume was already completed

* Centralize the 'skip-if-mounted' logic and apply it to create_nix_volume too

* Don't have the /nix path configurable at the moment

* Use the skipped helper

src/action/base/create_directory.rs

@@ -279,7 +279,7 @@ impl Action for CreateDirectory {
                 tracing::debug!("Not cleaning mountpoint `{}`", path.display());
             },
             (false, true, _) | (false, false, true) => {
-                crate::util::remove_dir_all(&path, OnMissing::Error)
+                crate::util::remove_dir_all(path, OnMissing::Error)
                     .await
                     .map_err(|e| ActionErrorKind::Remove(path.clone(), e))
                     .map_err(Self::error)?

src/action/base/create_file.rs

@@ -260,7 +260,7 @@ impl Action for CreateFile {
             force: _,
         } = self;
 
-        crate::util::remove_file(&path, OnMissing::Ignore)
+        crate::util::remove_file(path, OnMissing::Ignore)
             .await
             .map_err(|e| ActionErrorKind::Remove(path.to_owned(), e))
             .map_err(Self::error)?;

src/action/base/create_user.rs

@@ -334,6 +334,10 @@ pub async fn delete_user_macos(name: &str) -> Result<(), ActionErrorKind> {
             // These Macs cannot always delete users, as sometimes there is no graphical login
             tracing::warn!("Encountered an exit code 40 with -14120 error while removing user, this is likely because the initial executing user did not have a secure token, or that there was no graphical login session. To delete the user, log in graphically, then run `/usr/bin/dscl . -delete /Users/{}`", name);
         },
+        Some(185) if stderr.contains("-14009 (eDSUnknownNodeName)") => {
+            // The user has already been deleted
+            tracing::debug!("User already deleted: /Users/{}`", name);
+        },
         _ => {
             // Something went wrong
             return Err(ActionErrorKind::command_output(&command, output));

src/action/linux/provision_selinux.rs

@@ -125,7 +125,7 @@ impl Action for ProvisionSelinux {
 async fn remove_existing_policy(policy_path: &Path) -> Result<(), ActionErrorKind> {
     execute_command(Command::new("semodule").arg("--remove").arg("nix")).await?;
 
-    crate::util::remove_file(&policy_path, OnMissing::Ignore)
+    crate::util::remove_file(policy_path, OnMissing::Ignore)
         .await
         .map_err(|e| ActionErrorKind::Remove(policy_path.into(), e))?;
 

src/action/macos/create_apfs_volume.rs

@@ -1,4 +1,3 @@
-use std::io::Cursor;
 use std::path::{Path, PathBuf};
 
 use tokio::process::Command;
@@ -125,19 +124,9 @@ impl Action for CreateApfsVolume {
     #[tracing::instrument(level = "debug", skip_all)]
     async fn revert(&mut self) -> Result<(), ActionError> {
         let currently_mounted = {
-            let buf = execute_command(
-                Command::new("/usr/sbin/diskutil")
-                    .process_group(0)
-                    .args(["info", "-plist"])
-                    .arg(&self.name)
-                    .stdin(std::process::Stdio::null()),
-            )
-            .await
-            .map_err(Self::error)?
-            .stdout;
-            let the_plist: DiskUtilInfoOutput =
-                plist::from_reader(Cursor::new(buf)).map_err(Self::error)?;
-
+            let the_plist = DiskUtilInfoOutput::for_volume_name(&self.name)
+                .await
+                .map_err(Self::error)?;
             the_plist.is_mounted()
         };
 

src/action/macos/create_determinate_nix_volume.rs

@@ -72,14 +72,20 @@ impl CreateDeterminateNixVolume {
 
         let create_synthetic_objects = CreateSyntheticObjects::plan().await.map_err(Self::error)?;
 
-        let unmount_volume = UnmountApfsVolume::plan(disk, name.clone())
-            .await
-            .map_err(Self::error)?;
-
         let create_volume = CreateApfsVolume::plan(disk, name.clone(), case_sensitive)
             .await
             .map_err(Self::error)?;
 
+        let unmount_volume = if create_volume.state == crate::action::ActionState::Completed {
+            UnmountApfsVolume::plan_skip_if_already_mounted_to_nix(disk, name.clone())
+                .await
+                .map_err(Self::error)?
+        } else {
+            UnmountApfsVolume::plan(disk, name.clone())
+                .await
+                .map_err(Self::error)?
+        };
+
         let create_fstab_entry = CreateFstabEntry::plan(name.clone(), &create_volume)
             .await
             .map_err(Self::error)?;

src/action/macos/create_nix_volume.rs

@@ -62,14 +62,20 @@ impl CreateNixVolume {
 
         let create_synthetic_objects = CreateSyntheticObjects::plan().await.map_err(Self::error)?;
 
-        let unmount_volume = UnmountApfsVolume::plan(disk, name.clone())
-            .await
-            .map_err(Self::error)?;
-
         let create_volume = CreateApfsVolume::plan(disk, name.clone(), case_sensitive)
             .await
             .map_err(Self::error)?;
 
+        let unmount_volume = if create_volume.state == crate::action::ActionState::Completed {
+            UnmountApfsVolume::plan_skip_if_already_mounted_to_nix(disk, name.clone())
+                .await
+                .map_err(Self::error)?
+        } else {
+            UnmountApfsVolume::plan(disk, name.clone())
+                .await
+                .map_err(Self::error)?
+        };
+
         let create_fstab_entry = CreateFstabEntry::plan(name.clone(), &create_volume)
             .await
             .map_err(Self::error)?;

src/action/macos/enable_ownership.rs

@@ -1,4 +1,3 @@
-use std::io::Cursor;
 use std::path::{Path, PathBuf};
 
 use tokio::process::Command;
@@ -54,18 +53,9 @@ impl Action for EnableOwnership {
     #[tracing::instrument(level = "debug", skip_all)]
     async fn execute(&mut self) -> Result<(), ActionError> {
         let should_enable_ownership = {
-            let buf = execute_command(
-                Command::new("/usr/sbin/diskutil")
-                    .process_group(0)
-                    .args(["info", "-plist"])
-                    .arg(&self.path)
-                    .stdin(std::process::Stdio::null()),
-            )
-            .await
-            .map_err(Self::error)?
-            .stdout;
-            let the_plist: DiskUtilInfoOutput =
-                plist::from_reader(Cursor::new(buf)).map_err(Self::error)?;
+            let the_plist = DiskUtilInfoOutput::for_volume_path(&self.path)
+                .await
+                .map_err(Self::error)?;
 
             !the_plist.global_permissions_enabled
         };

src/action/macos/unmount_apfs_volume.rs

@@ -1,4 +1,3 @@
-use std::io::Cursor;
 use std::path::{Path, PathBuf};
 
 use tokio::process::Command;
@@ -29,6 +28,29 @@ impl UnmountApfsVolume {
         let disk = disk.as_ref().to_owned();
         Ok(Self { disk, name }.into())
     }
+
+    #[tracing::instrument(level = "debug", skip_all)]
+    pub async fn plan_skip_if_already_mounted_to_nix(
+        disk: impl AsRef<Path>,
+        name: String,
+    ) -> Result<StatefulAction<Self>, ActionError> {
+        let diskinfo = DiskUtilInfoOutput::for_volume_name(&name).await;
+
+        let task = Self {
+            disk: disk.as_ref().to_owned(),
+            name,
+        };
+
+        if let Ok(diskinfo) = diskinfo {
+            if Path::new(&diskinfo.parent_whole_disk) == disk.as_ref()
+                && diskinfo.mount_point.as_deref() == Some("/nix".as_ref())
+            {
+                return Ok(StatefulAction::skipped(task));
+            }
+        }
+
+        Ok(task.into())
+    }
 }
 
 #[async_trait::async_trait]
@@ -57,18 +79,9 @@ impl Action for UnmountApfsVolume {
     #[tracing::instrument(level = "debug", skip_all)]
     async fn execute(&mut self) -> Result<(), ActionError> {
         let currently_mounted = {
-            let buf = execute_command(
-                Command::new("/usr/sbin/diskutil")
-                    .process_group(0)
-                    .args(["info", "-plist"])
-                    .arg(&self.name)
-                    .stdin(std::process::Stdio::null()),
-            )
-            .await
-            .map_err(Self::error)?
-            .stdout;
-            let the_plist: DiskUtilInfoOutput =
-                plist::from_reader(Cursor::new(buf)).map_err(Self::error)?;
+            let the_plist = DiskUtilInfoOutput::for_volume_name(&self.name)
+                .await
+                .map_err(Self::error)?;
 
             the_plist.is_mounted()
         };
@@ -97,18 +110,9 @@ impl Action for UnmountApfsVolume {
     #[tracing::instrument(level = "debug", skip_all)]
     async fn revert(&mut self) -> Result<(), ActionError> {
         let currently_mounted = {
-            let buf = execute_command(
-                Command::new("/usr/sbin/diskutil")
-                    .process_group(0)
-                    .args(["info", "-plist"])
-                    .arg(&self.name)
-                    .stdin(std::process::Stdio::null()),
-            )
-            .await
-            .map_err(Self::error)?
-            .stdout;
-            let the_plist: DiskUtilInfoOutput =
-                plist::from_reader(Cursor::new(buf)).map_err(Self::error)?;
+            let the_plist = DiskUtilInfoOutput::for_volume_name(&self.name)
+                .await
+                .map_err(Self::error)?;
 
             the_plist.is_mounted()
         };

src/os/darwin/diskutil.rs

@@ -10,6 +10,28 @@ pub struct DiskUtilInfoOutput {
 }
 
 impl DiskUtilInfoOutput {
+    pub async fn for_volume_name(
+        volume_name: &str,
+    ) -> Result<Self, crate::action::ActionErrorKind> {
+        Self::for_volume_path(std::path::Path::new(volume_name)).await
+    }
+
+    pub async fn for_volume_path(
+        volume_path: &std::path::Path,
+    ) -> Result<Self, crate::action::ActionErrorKind> {
+        let buf = crate::execute_command(
+            tokio::process::Command::new("/usr/sbin/diskutil")
+                .process_group(0)
+                .args(["info", "-plist"])
+                .arg(volume_path)
+                .stdin(std::process::Stdio::null()),
+        )
+        .await?
+        .stdout;
+
+        Ok(plist::from_reader(std::io::Cursor::new(buf))?)
+    }
+
     pub fn is_mounted(&self) -> bool {
         match self.mount_point {
             None => false,