Various logic fixups in preparation for phased uninstall (#1277)

* EncryptApfsVolume: fix encryption detection, enable curing

* fixup: `launchctl print` does not take a `-plist` flag

* fixup: we always thought the service was started

The string "not running" contains "running", so we'd always set
`service_started = true;` here. That's wrong, so instead check for "does
not contain 'not running'".

This does open us up to thinking that anything that is not "not running"
is running, but we can address that later, if it becomes an issue.

Also I'm pretty sure we don't need this at all, since it's always "not
running" after it mounts the volume... But I'll leave that for later.

* BootstrapLaunchctlService: always run

* KickstartLaunchctlService: use execute_command helper

* fixup: get_uuid_for_label -> get_disk_info_for_label

For a follow-up commit.

* If a Nix Store volume already exists and is encrypted with FileVault, force `encrypt` to true

* fixup: check_loaded should not print to stdout/stderr, should use DARWIN_LAUNCHD_DOMAIN const

* fixup: cargo clippy

* fixup: check_loaded_output -> check_loaded

Even though the exit code is an output of running the program, it's not
textual output on stdout / stderr.

* fixup: make comment more readable

src/action/common/configure_init_service.rs

@@ -270,7 +270,7 @@ impl Action for ConfigureInitService {
                         Command::new("launchctl")
                             .process_group(0)
                             .arg("enable")
-                            .arg(&format!("{domain}/{service}"))
+                            .arg(format!("{domain}/{service}"))
                             .stdin(std::process::Stdio::null()),
                     )
                     .await
@@ -283,7 +283,7 @@ impl Action for ConfigureInitService {
                             .process_group(0)
                             .arg("kickstart")
                             .arg("-k")
-                            .arg(&format!("{domain}/{service}"))
+                            .arg(format!("{domain}/{service}"))
                             .stdin(std::process::Stdio::null()),
                     )
                     .await

src/action/macos/bootstrap_launchctl_service.rs

@@ -33,7 +33,6 @@ impl BootstrapLaunchctlService {
             command.process_group(0);
             command.arg("print");
             command.arg(format!("{DARWIN_LAUNCHD_DOMAIN}/{service}"));
-            command.arg("-plist");
             command.stdin(std::process::Stdio::null());
             command.stdout(std::process::Stdio::piped());
             command.stderr(std::process::Stdio::piped());
@@ -49,15 +48,6 @@ impl BootstrapLaunchctlService {
             .await
             .map_err(Self::error)?;
 
-        if is_present && !is_disabled {
-            return Ok(StatefulAction::completed(Self {
-                service,
-                path,
-                is_present,
-                is_disabled,
-            }));
-        }
-
         Ok(StatefulAction::uncompleted(Self {
             service,
             path,
@@ -110,19 +100,23 @@ impl Action for BootstrapLaunchctlService {
                 Command::new("launchctl")
                     .process_group(0)
                     .arg("enable")
-                    .arg(&format!("{DARWIN_LAUNCHD_DOMAIN}/{service}"))
+                    .arg(format!("{DARWIN_LAUNCHD_DOMAIN}/{service}"))
                     .stdin(std::process::Stdio::null()),
             )
             .await
             .map_err(Self::error)?;
         }
 
-        if !*is_present {
-            crate::action::macos::retry_bootstrap(DARWIN_LAUNCHD_DOMAIN, service, path)
+        if *is_present {
+            crate::action::macos::retry_bootout(DARWIN_LAUNCHD_DOMAIN, service)
                 .await
                 .map_err(Self::error)?;
         }
 
+        crate::action::macos::retry_bootstrap(DARWIN_LAUNCHD_DOMAIN, service, path)
+            .await
+            .map_err(Self::error)?;
+
         Ok(())
     }
 

src/action/macos/create_determinate_volume_service.rs

@@ -1,15 +1,19 @@
 use serde::{Deserialize, Serialize};
 use tracing::{span, Span};
 
-use std::path::{Path, PathBuf};
+use std::{
+    path::{Path, PathBuf},
+    process::Stdio,
+};
 use tokio::{
     fs::{remove_file, OpenOptions},
     io::AsyncWriteExt,
     process::Command,
 };
 
-use crate::action::{
-    Action, ActionDescription, ActionError, ActionErrorKind, ActionTag, StatefulAction,
+use crate::{
+    action::{Action, ActionDescription, ActionError, ActionErrorKind, ActionTag, StatefulAction},
+    execute_command,
 };
 
 use super::DARWIN_LAUNCHD_DOMAIN;
@@ -43,27 +47,27 @@ impl CreateDeterminateVolumeService {
 
         // If the service is currently loaded or running, we need to unload it during execute (since we will then recreate it and reload it)
         // This `launchctl` command may fail if the service isn't loaded
-        let mut check_loaded_command = Command::new("launchctl");
-        check_loaded_command.arg("print");
-        check_loaded_command.arg(format!("system/{}", this.mount_service_label));
-        tracing::trace!(
-            command = format!("{:?}", check_loaded_command.as_std()),
-            "Executing"
-        );
-        let check_loaded_output = check_loaded_command
-            .status()
-            .await
-            .map_err(|e| ActionErrorKind::command(&check_loaded_command, e))
-            .map_err(Self::error)?;
-
-        this.needs_bootout = check_loaded_output.success();
+        let check_loaded = execute_command(
+            Command::new("launchctl")
+                .arg("print")
+                .arg(format!(
+                    "{DARWIN_LAUNCHD_DOMAIN}/{}",
+                    this.mount_service_label
+                ))
+                .stdin(Stdio::null())
+                .stdout(Stdio::null())
+                .stderr(Stdio::null()),
+        )
+        .await
+        .ok();
 
-        if this.needs_bootout {
+        if check_loaded.is_some() {
             tracing::debug!(
                 "Detected loaded service `{}` which needs unload before replacing `{}`",
                 this.mount_service_label,
                 this.path.display(),
             );
+            this.needs_bootout = true;
         }
 
         if this.path.exists() {

src/action/macos/create_fstab_entry.rs

@@ -1,6 +1,6 @@
 use uuid::Uuid;
 
-use super::{get_uuid_for_label, CreateApfsVolume};
+use super::{get_disk_info_for_label, CreateApfsVolume};
 use crate::action::{
     Action, ActionDescription, ActionError, ActionErrorKind, ActionState, ActionTag, StatefulAction,
 };
@@ -123,11 +123,11 @@ impl Action for CreateFstabEntry {
             existing_entry,
         } = self;
         let fstab_path = Path::new(FSTAB_PATH);
-        let uuid = match get_uuid_for_label(apfs_volume_label)
+        let uuid = match get_disk_info_for_label(apfs_volume_label)
             .await
             .map_err(Self::error)?
         {
-            Some(uuid) => uuid,
+            Some(diskutil_info) => diskutil_info.volume_uuid,
             None => {
                 return Err(Self::error(CreateFstabEntryError::CannotDetermineUuid(
                     apfs_volume_label.clone(),
@@ -237,11 +237,11 @@ impl Action for CreateFstabEntry {
     async fn revert(&mut self) -> Result<(), ActionError> {
         let fstab_path = Path::new(FSTAB_PATH);
 
-        if let Some(uuid) = get_uuid_for_label(&self.apfs_volume_label)
+        if let Some(diskutil_info) = get_disk_info_for_label(&self.apfs_volume_label)
             .await
             .map_err(Self::error)?
         {
-            let fstab_entry = fstab_lines(&uuid, &self.apfs_volume_label);
+            let fstab_entry = fstab_lines(&diskutil_info.volume_uuid, &self.apfs_volume_label);
 
             let mut file = OpenOptions::new()
                 .create(false)

src/action/macos/create_nix_hook_service.rs

@@ -1,15 +1,16 @@
 use serde::{Deserialize, Serialize};
 use tracing::{span, Span};
 
-use std::path::PathBuf;
+use std::{path::PathBuf, process::Stdio};
 use tokio::{
     fs::{remove_file, OpenOptions},
     io::AsyncWriteExt,
     process::Command,
 };
 
-use crate::action::{
-    Action, ActionDescription, ActionError, ActionErrorKind, ActionTag, StatefulAction,
+use crate::{
+    action::{Action, ActionDescription, ActionError, ActionErrorKind, ActionTag, StatefulAction},
+    execute_command,
 };
 
 use super::DARWIN_LAUNCHD_DOMAIN;
@@ -37,26 +38,24 @@ impl CreateNixHookService {
 
         // If the service is currently loaded or running, we need to unload it during execute (since we will then recreate it and reload it)
         // This `launchctl` command may fail if the service isn't loaded
-        let mut check_loaded_command = Command::new("launchctl");
-        check_loaded_command.process_group(0);
-        check_loaded_command.arg("print");
-        check_loaded_command.arg(format!("system/{}", this.service_label));
-        tracing::trace!(
-            command = format!("{:?}", check_loaded_command.as_std()),
-            "Executing"
-        );
-        let check_loaded_output = check_loaded_command
-            .output()
-            .await
-            .map_err(|e| ActionErrorKind::command(&check_loaded_command, e))
-            .map_err(Self::error)?;
-        this.needs_bootout = check_loaded_output.status.success();
-        if this.needs_bootout {
+        let check_loaded = execute_command(
+            Command::new("launchctl")
+                .arg("print")
+                .arg(format!("{DARWIN_LAUNCHD_DOMAIN}/{}", this.service_label))
+                .stdin(Stdio::null())
+                .stdout(Stdio::null())
+                .stderr(Stdio::null()),
+        )
+        .await
+        .ok();
+
+        if check_loaded.is_some() {
             tracing::debug!(
                 "Detected loaded service `{}` which needs unload before replacing `{}`",
                 this.service_label,
                 this.path.display(),
             );
+            this.needs_bootout = true;
         }
 
         if this.path.exists() {

src/action/macos/create_volume_service.rs

@@ -1,19 +1,25 @@
 use serde::{Deserialize, Serialize};
 use tracing::{span, Span};
 
-use std::path::{Path, PathBuf};
+use std::{
+    path::{Path, PathBuf},
+    process::Stdio,
+};
 use tokio::{
     fs::{remove_file, OpenOptions},
     io::AsyncWriteExt,
     process::Command,
 };
 
-use crate::action::{
-    macos::DARWIN_LAUNCHD_DOMAIN, Action, ActionDescription, ActionError, ActionErrorKind,
-    ActionTag, StatefulAction,
+use crate::{
+    action::{
+        macos::DARWIN_LAUNCHD_DOMAIN, Action, ActionDescription, ActionError, ActionErrorKind,
+        ActionTag, StatefulAction,
+    },
+    execute_command,
 };
 
-use super::get_uuid_for_label;
+use super::get_disk_info_for_label;
 
 /** Create a plist for a `launchctl` service to mount the given `apfs_volume_label` on the given `mount_point`.
  */
@@ -51,39 +57,41 @@ impl CreateVolumeService {
 
         // If the service is currently loaded or running, we need to unload it during execute (since we will then recreate it and reload it)
         // This `launchctl` command may fail if the service isn't loaded
-        let mut check_loaded_command = Command::new("launchctl");
-        check_loaded_command.arg("print");
-        check_loaded_command.arg(format!("system/{}", this.mount_service_label));
-        tracing::trace!(
-            command = format!("{:?}", check_loaded_command.as_std()),
-            "Executing"
-        );
-        let check_loaded_output = check_loaded_command
-            .output()
-            .await
-            .map_err(|e| ActionErrorKind::command(&check_loaded_command, e))
-            .map_err(Self::error)?;
-        this.needs_bootout = check_loaded_output.status.success();
-        if this.needs_bootout {
+        let check_loaded = execute_command(
+            Command::new("launchctl")
+                .arg("print")
+                .arg(format!(
+                    "{DARWIN_LAUNCHD_DOMAIN}/{}",
+                    this.mount_service_label
+                ))
+                .stdin(Stdio::null())
+                .stdout(Stdio::null())
+                .stderr(Stdio::null()),
+        )
+        .await
+        .ok();
+
+        if check_loaded.is_some() {
             tracing::debug!(
                 "Detected loaded service `{}` which needs unload before replacing `{}`",
                 this.mount_service_label,
                 this.path.display(),
             );
+            this.needs_bootout = true;
         }
 
         if this.path.exists() {
             let discovered_plist: LaunchctlMountPlist =
                 plist::from_file(&this.path).map_err(Self::error)?;
-            match get_uuid_for_label(&this.apfs_volume_label)
+            match get_disk_info_for_label(&this.apfs_volume_label)
                 .await
                 .map_err(Self::error)?
             {
-                Some(uuid) => {
+                Some(disk_info) => {
                     let expected_plist = generate_mount_plist(
                         &this.mount_service_label,
                         &this.apfs_volume_label,
-                        uuid,
+                        disk_info.volume_uuid,
                         &this.mount_point,
                         encrypt,
                     )
@@ -191,7 +199,7 @@ impl Action for CreateVolumeService {
                 .map_err(Self::error)?;
         }
 
-        let uuid = match get_uuid_for_label(apfs_volume_label)
+        let disk_info = match get_disk_info_for_label(apfs_volume_label)
             .await
             .map_err(Self::error)?
         {
@@ -205,7 +213,7 @@ impl Action for CreateVolumeService {
         let generated_plist = generate_mount_plist(
             mount_service_label,
             apfs_volume_label,
-            uuid,
+            disk_info.volume_uuid,
             mount_point,
             *encrypt,
         )