Rework error handling and merge the two state files.

src/activate.rs

@@ -2,9 +2,65 @@ mod etc_files;
 mod services;
 
 use anyhow::Result;
-use std::process;
+use serde::{Deserialize, Serialize};
+use std::fs::DirBuilder;
+use std::path::{Path, PathBuf};
+use std::{fs, io, process};
+use thiserror::Error;
 
-use crate::StorePath;
+use crate::activate::etc_files::EtcTree;
+use crate::{StorePath, STATE_FILE_NAME, SYSTEM_MANAGER_STATE_DIR};
+
+#[derive(Error, Debug)]
+pub enum ActivationError<R> {
+    #[error("")]
+    WithPartialResult { result: R, source: anyhow::Error },
+}
+
+impl<R> ActivationError<R> {
+    fn with_partial_result<E>(result: R, source: E) -> Self
+    where
+        E: Into<anyhow::Error>,
+    {
+        Self::WithPartialResult {
+            result,
+            source: source.into(),
+        }
+    }
+}
+
+pub type ActivationResult<R> = Result<R, ActivationError<R>>;
+
+#[derive(Debug, Clone, Default, Serialize, Deserialize)]
+#[serde(rename_all = "camelCase")]
+pub struct State {
+    etc_tree: EtcTree,
+    services: services::Services,
+}
+
+impl State {
+    pub fn from_file(state_file: &Path) -> Result<Self> {
+        if state_file.is_file() {
+            log::info!("Reading state info from {}", state_file.display());
+            let reader = io::BufReader::new(fs::File::open(state_file)?);
+            serde_json::from_reader(reader).or_else(|e| {
+                log::error!("Error reading the state file, ignoring.");
+                log::error!("{e:?}");
+                Ok(Self::default())
+            })
+        } else {
+            Ok(Self::default())
+        }
+    }
+
+    pub fn write_to_file(&self, state_file: &Path) -> Result<()> {
+        log::info!("Writing state info into file: {}", state_file.display());
+        let writer = io::BufWriter::new(fs::File::create(state_file)?);
+
+        serde_json::to_writer(writer, self)?;
+        Ok(())
+    }
+}
 
 pub fn activate(store_path: &StorePath, ephemeral: bool) -> Result<()> {
     log::info!("Activating system-manager profile: {store_path}");
@@ -17,20 +73,67 @@ pub fn activate(store_path: &StorePath, ephemeral: bool) -> Result<()> {
         anyhow::bail!("Failure in pre-activation assertions.");
     }
 
-    log::info!("Activating etc files...");
-    etc_files::activate(store_path, ephemeral)?;
+    let state_file = &get_state_file()?;
+    let old_state = State::from_file(state_file)?;
 
-    log::info!("Activating systemd services...");
-    services::activate(store_path, ephemeral)?;
+    log::info!("Activating etc files...");
+
+    match etc_files::activate(store_path, old_state.etc_tree, ephemeral) {
+        Ok(etc_tree) => {
+            log::info!("Activating systemd services...");
+            match services::activate(store_path, old_state.services, ephemeral) {
+                Ok(services) => State { etc_tree, services },
+                Err(ActivationError::WithPartialResult { result, source }) => {
+                    log::error!("Error during activation: {source:?}");
+                    State {
+                        etc_tree,
+                        services: result,
+                    }
+                }
+            }
+        }
+        Err(ActivationError::WithPartialResult { result, source }) => {
+            log::error!("Error during activation: {source:?}");
+            State {
+                etc_tree: result,
+                ..old_state
+            }
+        }
+    }
+    .write_to_file(state_file)?;
 
     Ok(())
 }
 
-// TODO should we also remove the GC root for the profile if it exists?
 pub fn deactivate() -> Result<()> {
     log::info!("Deactivating system-manager");
-    etc_files::deactivate()?;
-    services::deactivate()?;
+    let state_file = &get_state_file()?;
+    let old_state = State::from_file(state_file)?;
+    log::debug!("{old_state:?}");
+
+    match etc_files::deactivate(old_state.etc_tree) {
+        Ok(etc_tree) => {
+            log::info!("Deactivating systemd services...");
+            match services::deactivate(old_state.services) {
+                Ok(services) => State { etc_tree, services },
+                Err(ActivationError::WithPartialResult { result, source }) => {
+                    log::error!("Error during deactivation: {source:?}");
+                    State {
+                        etc_tree,
+                        services: result,
+                    }
+                }
+            }
+        }
+        Err(ActivationError::WithPartialResult { result, source }) => {
+            log::error!("Error during deactivation: {source:?}");
+            State {
+                etc_tree: result,
+                ..old_state
+            }
+        }
+    }
+    .write_to_file(state_file)?;
     Ok(())
 }
 
@@ -46,3 +149,11 @@ fn run_preactivation_assertions(store_path: &StorePath) -> Result<process::ExitS
     .status()?;
     Ok(status)
 }
+
+fn get_state_file() -> Result<PathBuf> {
+    let state_file = Path::new(SYSTEM_MANAGER_STATE_DIR).join(STATE_FILE_NAME);
+    DirBuilder::new()
+        .recursive(true)
+        .create(SYSTEM_MANAGER_STATE_DIR)?;
+    Ok(state_file)
+}

src/activate/etc_files.rs

@@ -1,6 +1,5 @@
 mod etc_tree;
 
-use anyhow::{anyhow, Result};
 use im::HashMap;
 use itertools::Itertools;
 use serde::{Deserialize, Serialize};
@@ -10,13 +9,17 @@ use std::path;
 use std::path::{Path, PathBuf};
 use std::{fs, io};
 
+use self::etc_tree::EtcFileStatus;
+use super::ActivationResult;
+use crate::activate::ActivationError;
 use crate::{
     create_link, create_store_link, etc_dir, remove_dir, remove_file, remove_link, StorePath,
-    ETC_STATE_FILE_NAME, SYSTEM_MANAGER_STATE_DIR, SYSTEM_MANAGER_STATIC_NAME,
+    SYSTEM_MANAGER_STATIC_NAME,
 };
-use etc_tree::EtcTree;
 
-use self::etc_tree::EtcFileStatus;
+pub use etc_tree::EtcTree;
+
+type EtcActivationResult = ActivationResult<EtcTree>;
 
 #[derive(Debug, Clone, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
@@ -63,7 +66,7 @@ struct CreatedEtcFile {
     path: PathBuf,
 }
 
-pub fn activate(store_path: &StorePath, ephemeral: bool) -> Result<()> {
+fn read_config(store_path: &StorePath) -> anyhow::Result<EtcFilesConfig> {
     log::info!("Reading etc file definitions...");
     let file = fs::File::open(
         Path::new(&store_path.store_path)
@@ -73,48 +76,47 @@ pub fn activate(store_path: &StorePath, ephemeral: bool) -> Result<()> {
     let reader = io::BufReader::new(file);
     let config: EtcFilesConfig = serde_json::from_reader(reader)?;
     log::debug!("{config}");
+    Ok(config)
+}
+
+pub fn activate(
+    store_path: &StorePath,
+    old_state: EtcTree,
+    ephemeral: bool,
+) -> EtcActivationResult {
+    let config = read_config(store_path)
+        .map_err(|e| ActivationError::with_partial_result(old_state.clone(), e))?;
 
     let etc_dir = etc_dir(ephemeral);
     log::info!("Creating /etc entries in {}", etc_dir.display());
 
-    DirBuilder::new().recursive(true).create(&etc_dir)?;
-
-    let old_state = EtcTree::from_file(&get_state_file()?)?;
     let initial_state = EtcTree::root_node();
 
-    let (state, status) = create_etc_static_link(
+    let state = create_etc_static_link(
         SYSTEM_MANAGER_STATIC_NAME,
         &config.static_env,
         &etc_dir,
         initial_state,
-    );
-    status?;
+    )?;
 
-    // Create the rest of the links and serialise the resulting state
-    create_etc_links(config.entries.values(), &etc_dir, state, &old_state)
+    // Create the rest of the links
+    let final_state = create_etc_links(config.entries.values(), &etc_dir, state, &old_state)
         .update_state(old_state, &try_delete_path)
-        .unwrap_or_default()
-        .write_to_file(&get_state_file()?)?;
+        .unwrap_or_default();
 
     log::info!("Done");
-    Ok(())
+    Ok(final_state)
 }
 
-pub fn deactivate() -> Result<()> {
-    let state = EtcTree::from_file(&get_state_file()?)?;
-    log::debug!("{:?}", state);
-
-    state
-        .deactivate(&try_delete_path)
-        .unwrap_or_default()
-        .write_to_file(&get_state_file()?)?;
+pub fn deactivate(old_state: EtcTree) -> EtcActivationResult {
+    let final_state = old_state.deactivate(&try_delete_path).unwrap_or_default();
 
     log::info!("Done");
-    Ok(())
+    Ok(final_state)
 }
 
 fn try_delete_path(path: &Path, status: &EtcFileStatus) -> bool {
-    fn do_try_delete(path: &Path, status: &EtcFileStatus) -> Result<()> {
+    fn do_try_delete(path: &Path, status: &EtcFileStatus) -> anyhow::Result<()> {
         // exists() returns false for broken symlinks
         if path.exists() || path.is_symlink() {
             if path.is_symlink() {
@@ -158,12 +160,15 @@ where
     E: Iterator<Item = &'a EtcFile>,
 {
     entries.fold(state, |state, entry| {
-        let (new_state, status) = create_etc_entry(entry, etc_dir, state, old_state);
-        match status {
-            Ok(_) => new_state,
-            Err(e) => {
-                log::error!("Error while creating file in {}: {e}", etc_dir.display());
-                new_state
+        let new_state = create_etc_entry(entry, etc_dir, state, old_state);
+        match new_state {
+            Ok(new_state) => new_state,
+            Err(ActivationError::WithPartialResult { result, source }) => {
+                log::error!(
+                    "Error while creating file in {}: {source:?}",
+                    etc_dir.display()
+                );
+                result
             }
         }
     })
@@ -174,13 +179,15 @@ fn create_etc_static_link(
     store_path: &StorePath,
     etc_dir: &Path,
     state: EtcTree,
-) -> (EtcTree, Result<()>) {
+) -> EtcActivationResult {
     let static_path = etc_dir.join(static_dir_name);
-    let (new_state, status) = create_dir_recursively(static_path.parent().unwrap(), state);
-    match status.and_then(|_| create_store_link(store_path, &static_path)) {
-        Ok(_) => (new_state.register_managed_entry(&static_path), Ok(())),
-        e => (new_state, e),
-    }
+    let new_state = create_dir_recursively(static_path.parent().unwrap(), state);
+    new_state.and_then(|new_state| {
+        create_store_link(store_path, &static_path).map_or_else(
+            |e| Err(ActivationError::with_partial_result(new_state.clone(), e)),
+            |_| Ok(new_state.clone().register_managed_entry(&static_path)),
+        )
+    })
 }
 
 fn create_etc_link<P>(
@@ -188,7 +195,7 @@ fn create_etc_link<P>(
     etc_dir: &Path,
     state: EtcTree,
     old_state: &EtcTree,
-) -> (EtcTree, Result<()>)
+) -> EtcActivationResult
 where
     P: AsRef<Path>,
 {
@@ -199,46 +206,44 @@ where
         state: EtcTree,
         old_state: &EtcTree,
         upwards_path: &Path,
-    ) -> (EtcTree, Result<()>) {
+    ) -> EtcActivationResult {
         let link_path = etc_dir.join(link_target);
         if link_path.is_dir() && absolute_target.is_dir() {
             log::info!("Entering into directory...");
-            (
-                absolute_target
-                    .read_dir()
-                    .expect("Error reading the directory.")
-                    .fold(state, |state, entry| {
-                        let (new_state, status) = go(
-                            &link_target.join(
-                                entry
-                                    .expect("Error reading the directory entry.")
-                                    .file_name(),
-                            ),
-                            etc_dir,
-                            state,
-                            old_state,
-                            &upwards_path.join(".."),
-                        );
-                        if let Err(e) = status {
+            Ok(absolute_target
+                .read_dir()
+                .expect("Error reading the directory.")
+                .fold(state, |state, entry| {
+                    let new_state = go(
+                        &link_target.join(
+                            entry
+                                .expect("Error reading the directory entry.")
+                                .file_name(),
+                        ),
+                        etc_dir,
+                        state,
+                        old_state,
+                        &upwards_path.join(".."),
+                    );
+                    match new_state {
+                        Ok(new_state) => new_state,
+                        Err(ActivationError::WithPartialResult { result, source }) => {
                             log::error!(
-                                "Error while trying to link directory {}: {:?}",
-                                absolute_target.display(),
-                                e
+                                "Error while trying to link directory {}: {source:?}",
+                                absolute_target.display()
                             );
+                            result
                         }
-                        new_state
-                    }),
-                // TODO better error handling
-                Ok(()),
-            )
+                    }
+                }))
         } else {
-            (
+            Err(ActivationError::with_partial_result(
                 state,
-                Err(anyhow!(
+                anyhow::anyhow!(
                     "Unmanaged file or directory {} already exists, ignoring...",
                     link_path.display()
-                )),
-            )
+                ),
+            ))
         }
     }
 
@@ -248,46 +253,42 @@ where
         state: EtcTree,
         old_state: &EtcTree,
         upwards_path: &Path,
-    ) -> (EtcTree, Result<()>) {
+    ) -> EtcActivationResult {
         let link_path = etc_dir.join(link_target);
-        match create_dir_recursively(link_path.parent().unwrap(), state) {
-            (dir_state, Ok(_)) => {
-                let target = upwards_path
-                    .join(SYSTEM_MANAGER_STATIC_NAME)
-                    .join(link_target);
-                let absolute_target = etc_dir.join(SYSTEM_MANAGER_STATIC_NAME).join(link_target);
-                if link_path.exists() && !old_state.is_managed(&link_path) {
-                    link_dir_contents(
-                        link_target,
-                        &absolute_target,
-                        etc_dir,
-                        dir_state,
-                        old_state,
-                        upwards_path,
-                    )
-                } else if link_path.is_symlink()
-                    && link_path.read_link().expect("Error reading link.") == target
-                {
-                    (dir_state.register_managed_entry(&link_path), Ok(()))
-                } else {
-                    let result = if link_path.exists() {
-                        assert!(old_state.is_managed(&link_path));
-                        fs::remove_file(&link_path).map_err(anyhow::Error::from)
-                    } else {
-                        Ok(())
-                    };
+        let dir_state = create_dir_recursively(link_path.parent().unwrap(), state)?;
+        let target = upwards_path
+            .join(SYSTEM_MANAGER_STATIC_NAME)
+            .join(link_target);
+        let absolute_target = etc_dir.join(SYSTEM_MANAGER_STATIC_NAME).join(link_target);
+        if link_path.exists() && !old_state.is_managed(&link_path) {
+            link_dir_contents(
+                link_target,
+                &absolute_target,
+                etc_dir,
+                dir_state,
+                old_state,
+                upwards_path,
+            )
+        } else if link_path.is_symlink()
+            && link_path.read_link().expect("Error reading link.") == target
+        {
+            Ok(dir_state.register_managed_entry(&link_path))
+        } else {
+            let result = if link_path.exists() {
+                assert!(old_state.is_managed(&link_path));
+                fs::remove_file(&link_path)
+                    .map_err(|e| ActivationError::with_partial_result(dir_state.clone(), e))
+            } else {
+                Ok(())
+            };
 
-                    match result.and_then(|_| create_link(&target, &link_path)) {
-                        Ok(_) => (dir_state.register_managed_entry(&link_path), Ok(())),
-                        Err(e) => (
-                            dir_state,
-                            Err(anyhow!(e)
-                                .context(format!("Error creating link: {}", link_path.display()))),
-                        ),
-                    }
-                }
+            match result.and_then(|_| {
+                create_link(&target, &link_path)
+                    .map_err(|e| ActivationError::with_partial_result(dir_state.clone(), e))
+            }) {
+                Ok(_) => Ok(dir_state.register_managed_entry(&link_path)),
+                Err(e) => Err(e),
             }
-            (new_state, e) => (new_state, e),
         }
     }
 
@@ -305,74 +306,83 @@ fn create_etc_entry(
     etc_dir: &Path,
     state: EtcTree,
     old_state: &EtcTree,
-) -> (EtcTree, Result<()>) {
+) -> EtcActivationResult {
     if entry.mode == "symlink" {
         if let Some(path::Component::Normal(link_target)) = entry.target.components().next() {
             create_etc_link(&link_target, etc_dir, state, old_state)
         } else {
-            (
+            Err(ActivationError::with_partial_result(
                 state,
-                Err(anyhow!("Cannot create link: {}", entry.target.display(),)),
-            )
+                anyhow::anyhow!("Cannot create link: {}", entry.target.display()),
+            ))
         }
     } else {
         let target_path = etc_dir.join(&entry.target);
-        let (new_state, status) = create_dir_recursively(target_path.parent().unwrap(), state);
-        match status.and_then(|_| {
-            copy_file(
-                &entry.source.store_path.join(&entry.target),
-                &target_path,
-                &entry.mode,
-                old_state,
-            )
-        }) {
-            Ok(_) => (new_state.register_managed_entry(&target_path), Ok(())),
-            e => (new_state, e),
+        let new_state = create_dir_recursively(target_path.parent().unwrap(), state)?;
+        match copy_file(
+            &entry.source.store_path.join(&entry.target),
+            &target_path,
+            &entry.mode,
+            old_state,
+        ) {
+            Ok(_) => Ok(new_state.register_managed_entry(&target_path)),
+            Err(e) => Err(ActivationError::with_partial_result(new_state, e)),
         }
     }
 }
 
-fn create_dir_recursively(dir: &Path, state: EtcTree) -> (EtcTree, Result<()>) {
+fn create_dir_recursively(dir: &Path, state: EtcTree) -> EtcActivationResult {
     use itertools::FoldWhile::{Continue, Done};
     use path::Component;
 
     let dirbuilder = DirBuilder::new();
-    let (new_state, _, status) = dir
+    let (new_state, _) = dir
         .components()
         .fold_while(
-            (state, PathBuf::from(path::MAIN_SEPARATOR_STR), Ok(())),
-            |(state, path, _), component| match component {
-                Component::RootDir => Continue((state, path, Ok(()))),
-                Component::Normal(dir) => {
+            (Ok(state), PathBuf::from(path::MAIN_SEPARATOR_STR)),
+            |(state, path), component| match (state, component) {
+                (Ok(state), Component::RootDir) => Continue((Ok(state), path)),
+                (Ok(state), Component::Normal(dir)) => {
                     let new_path = path.join(dir);
                     if !new_path.exists() {
                         log::debug!("Creating path: {}", new_path.display());
                         match dirbuilder.create(&new_path) {
                             Ok(_) => {
                                 let new_state = state.register_managed_entry(&new_path);
-                                Continue((new_state, new_path, Ok(())))
+                                Continue((Ok(new_state), new_path))
                             }
-                            Err(e) => Done((state, path, Err(anyhow!(e)))),
+                            Err(e) => Done((
+                                Err(ActivationError::with_partial_result(
+                                    state,
+                                    anyhow::anyhow!(e).context(format!(
+                                        "Error creating directory {}",
+                                        new_path.display()
+                                    )),
+                                )),
+                                path,
+                            )),
                         }
                     } else {
-                        Continue((state, new_path, Ok(())))
+                        Continue((Ok(state), new_path))
                     }
                 }
-                otherwise => Done((
-                    state,
-                    path,
-                    Err(anyhow!(
-                        "Unexpected path component encountered: {:?}",
-                        otherwise
+                (Ok(state), otherwise) => Done((
+                    Err(ActivationError::with_partial_result(
+                        state,
+                        anyhow::anyhow!("Unexpected path component encountered: {:?}", otherwise),
                     )),
+                    path,
                 )),
+                (Err(e), _) => {
+                    panic!("Something went horribly wrong! We should not get here: {e:?}.")
+                }
             },
         )
         .into_inner();
-    (new_state, status)
+    new_state
 }
 
-fn copy_file(source: &Path, target: &Path, mode: &str, old_state: &EtcTree) -> Result<()> {
+fn copy_file(source: &Path, target: &Path, mode: &str, old_state: &EtcTree) -> anyhow::Result<()> {
     let exists = target.try_exists()?;
     if !exists || old_state.is_managed(target) {
         log::debug!(
@@ -388,11 +398,3 @@ fn copy_file(source: &Path, target: &Path, mode: &str, old_state: &EtcTree) -> R
         anyhow::bail!("File {} already exists, ignoring.", target.display());
     }
 }
-
-fn get_state_file() -> Result<PathBuf> {
-    let state_file = Path::new(SYSTEM_MANAGER_STATE_DIR).join(ETC_STATE_FILE_NAME);
-    DirBuilder::new()
-        .recursive(true)
-        .create(SYSTEM_MANAGER_STATE_DIR)?;
-    Ok(state_file)
-}

src/activate/etc_files/etc_tree.rs

@@ -1,10 +1,9 @@
-use anyhow::Result;
 use im::HashMap;
 use serde::{Deserialize, Serialize};
 use std::cmp::Eq;
 use std::iter::Peekable;
+use std::path;
 use std::path::{Path, PathBuf};
-use std::{fs, io, path};
 
 #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
@@ -230,29 +229,6 @@ impl EtcTree {
 
         Some(merged)
     }
-
-    pub fn write_to_file(&self, state_file: &Path) -> Result<()> {
-        log::info!("Writing state info into file: {}", state_file.display());
-        let writer = io::BufWriter::new(fs::File::create(state_file)?);
-
-        serde_json::to_writer(writer, self)?;
-        Ok(())
-    }
-
-    pub fn from_file(state_file: &Path) -> Result<Self> {
-        if state_file.is_file() {
-            log::info!("Reading state info from {}", state_file.display());
-            let reader = io::BufReader::new(fs::File::open(state_file)?);
-            match serde_json::from_reader(reader) {
-                Ok(created_files) => return Ok(created_files),
-                Err(e) => {
-                    log::error!("Error reading the state file, ignoring.");
-                    log::error!("{:?}", e);
-                }
-            }
-        }
-        Ok(Self::default())
-    }
 }
 
 #[cfg(test)]

src/activate/services.rs

@@ -1,24 +1,25 @@
-use anyhow::{Context, Result};
+use anyhow::Context;
 use im::{HashMap, HashSet};
 use serde::{Deserialize, Serialize};
-use std::fs::DirBuilder;
 use std::path::{self, Path, PathBuf};
 use std::time::Duration;
 use std::{fs, io, str};
 
-use crate::{
-    create_link, etc_dir, systemd, StorePath, SERVICES_STATE_FILE_NAME, SYSTEM_MANAGER_STATE_DIR,
-};
+use super::ActivationResult;
+use crate::activate::ActivationError;
+use crate::{create_link, etc_dir, systemd, StorePath};
+
+type ServiceActivationResult = ActivationResult<Services>;
 
 #[derive(Debug, Clone, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
-struct ServiceConfig {
+pub struct ServiceConfig {
     store_path: StorePath,
 }
 
-type Services = HashMap<String, ServiceConfig>;
+pub type Services = HashMap<String, ServiceConfig>;
 
-fn print_services(services: &Services) -> Result<String> {
+fn print_services(services: &Services) -> String {
     let out = itertools::intersperse(
         services
             .iter()
@@ -26,30 +27,39 @@ fn print_services(services: &Services) -> Result<String> {
         "\n".to_owned(),
     )
     .collect();
-    Ok(out)
+    out
 }
 
-pub fn activate(store_path: &StorePath, ephemeral: bool) -> Result<()> {
-    verify_systemd_dir(ephemeral)?;
-
-    let old_services = read_saved_services()?;
+pub fn activate(
+    store_path: &StorePath,
+    old_services: Services,
+    ephemeral: bool,
+) -> ServiceActivationResult {
+    verify_systemd_dir(ephemeral)
+        .map_err(|e| ActivationError::with_partial_result(old_services.clone(), e))?;
 
     log::info!("Reading new service definitions...");
     let file = fs::File::open(
         Path::new(&store_path.store_path)
             .join("services")
             .join("services.json"),
-    )?;
+    )
+    .map_err(|e| ActivationError::with_partial_result(old_services.clone(), e))?;
     let reader = io::BufReader::new(file);
-    let services: Services = serde_json::from_reader(reader)?;
-    log::debug!("{}", print_services(&services)?);
+    let services: Services = serde_json::from_reader(reader)
+        .map_err(|e| ActivationError::with_partial_result(old_services.clone(), e))?;
+    log::debug!("{}", print_services(&services));
 
-    serialise_saved_services(&services)?;
+    //serialise_saved_services(&services)?;
 
     let services_to_stop = old_services.clone().relative_complement(services.clone());
+    let services_to_reload = get_services_to_reload(services.clone(), old_services.clone());
 
-    let service_manager = systemd::ServiceManager::new_session()?;
-    let job_monitor = service_manager.monitor_jobs_init()?;
+    let service_manager = systemd::ServiceManager::new_session()
+        .map_err(|e| ActivationError::with_partial_result(old_services.clone(), e))?;
+    let job_monitor = service_manager
+        .monitor_jobs_init()
+        .map_err(|e| ActivationError::with_partial_result(old_services.clone(), e))?;
     let timeout = Some(Duration::from_secs(30));
 
     // We need to do this before we reload the systemd daemon, so that the daemon
@@ -58,33 +68,37 @@ pub fn activate(store_path: &StorePath, ephemeral: bool) -> Result<()> {
     wait_for_jobs(
         &service_manager,
         &job_monitor,
-        stop_services(&service_manager, &services_to_stop)?,
+        stop_services(&service_manager, &services_to_stop),
         &timeout,
-    )?;
+    )
+    .map_err(|e| ActivationError::with_partial_result(services.clone(), e))?;
 
     // We added all new services and removed old ones, so let's reload the units
     // to tell systemd about them.
     log::info!("Reloading the systemd daemon...");
-    service_manager.daemon_reload()?;
+    service_manager
+        .daemon_reload()
+        .map_err(|e| ActivationError::with_partial_result(services.clone(), e))?;
 
-    let active_targets = get_active_targets(&service_manager);
-    let services_to_reload = get_services_to_reload(services, old_services);
+    let active_targets = get_active_targets(&service_manager)
+        .map_err(|e| ActivationError::with_partial_result(services.clone(), e))?;
 
     wait_for_jobs(
         &service_manager,
         &job_monitor,
-        reload_services(&service_manager, &services_to_reload)?
-            + start_units(&service_manager, &active_targets?)?,
+        reload_services(&service_manager, &services_to_reload)
+            + start_units(&service_manager, &active_targets),
         &timeout,
-    )?;
+    )
+    .map_err(|e| ActivationError::with_partial_result(services.clone(), e))?;
 
     log::info!("Done");
-    Ok(())
+    Ok(services)
 }
 
 fn get_active_targets(
     service_manager: &systemd::ServiceManager,
-) -> Result<Vec<systemd::UnitStatus>> {
+) -> anyhow::Result<Vec<systemd::UnitStatus>> {
     // We exclude some targets that we do not want to start
     let excluded_targets: HashSet<String> =
         ["suspend.target", "hibernate.target", "hybrid-sleep.target"]
@@ -135,7 +149,7 @@ fn systemd_system_dir(ephemeral: bool) -> PathBuf {
     }
 }
 
-fn verify_systemd_dir(ephemeral: bool) -> Result<()> {
+fn verify_systemd_dir(ephemeral: bool) -> anyhow::Result<()> {
     if ephemeral {
         let system_dir = systemd_system_dir(ephemeral);
         if system_dir.exists()
@@ -177,15 +191,18 @@ fn verify_systemd_dir(ephemeral: bool) -> Result<()> {
     Ok(())
 }
 
-pub fn deactivate() -> Result<()> {
-    restore_ephemeral_system_dir()?;
-
-    let old_services = read_saved_services()?;
+pub fn deactivate(old_services: Services) -> ServiceActivationResult {
     log::debug!("{:?}", old_services);
 
-    let service_manager = systemd::ServiceManager::new_session()?;
+    restore_ephemeral_system_dir()
+        .map_err(|e| ActivationError::with_partial_result(old_services.clone(), e))?;
+
+    let service_manager = systemd::ServiceManager::new_session()
+        .map_err(|e| ActivationError::with_partial_result(old_services.clone(), e))?;
     if !old_services.is_empty() {
-        let job_monitor = service_manager.monitor_jobs_init()?;
+        let job_monitor = service_manager
+            .monitor_jobs_init()
+            .map_err(|e| ActivationError::with_partial_result(old_services.clone(), e))?;
         let timeout = Some(Duration::from_secs(30));
 
         // We need to do this before we reload the systemd daemon, so that the daemon
@@ -193,19 +210,21 @@ pub fn deactivate() -> Result<()> {
         wait_for_jobs(
             &service_manager,
             &job_monitor,
-            stop_services(&service_manager, &old_services)?,
+            stop_services(&service_manager, &old_services),
             &timeout,
-        )?;
+        )
+        // We consider all jobs stopped now..
+        .map_err(|e| ActivationError::with_partial_result(im::HashMap::new(), e))?;
     } else {
         log::info!("No services to deactivate.");
     }
     log::info!("Reloading the systemd daemon...");
-    service_manager.daemon_reload()?;
-
-    serialise_saved_services(&HashMap::new())?;
+    service_manager
+        .daemon_reload()
+        .map_err(|e| ActivationError::with_partial_result(im::HashMap::new(), e))?;
 
     log::info!("Done");
-    Ok(())
+    Ok(im::HashMap::new())
 }
 
 // If we turned the ephemeral systemd system dir under /run into a symlink,
@@ -213,7 +232,7 @@ pub fn deactivate() -> Result<()> {
 // To avoid this, we always check whether this directory exists and is correct,
 // and we recreate it if needed.
 // NOTE: We rely on the fact that the etc files get cleaned up first, before this runs!
-fn restore_ephemeral_system_dir() -> Result<()> {
+fn restore_ephemeral_system_dir() -> anyhow::Result<()> {
     let ephemeral_systemd_system_dir = systemd_system_dir(true);
     if !ephemeral_systemd_system_dir.exists() {
         if ephemeral_systemd_system_dir.is_symlink() {
@@ -224,43 +243,7 @@ fn restore_ephemeral_system_dir() -> Result<()> {
     Ok(())
 }
 
-// TODO: we should probably lock this file to avoid concurrent writes
-fn serialise_saved_services(services: &Services) -> Result<()> {
-    let state_file = Path::new(SYSTEM_MANAGER_STATE_DIR).join(SERVICES_STATE_FILE_NAME);
-    DirBuilder::new()
-        .recursive(true)
-        .create(SYSTEM_MANAGER_STATE_DIR)?;
-
-    log::info!("Writing state info into file: {}", state_file.display());
-    let writer = io::BufWriter::new(fs::File::create(state_file)?);
-    serde_json::to_writer(writer, services)?;
-    Ok(())
-}
-
-fn read_saved_services() -> Result<Services> {
-    let state_file = Path::new(SYSTEM_MANAGER_STATE_DIR).join(SERVICES_STATE_FILE_NAME);
-    DirBuilder::new()
-        .recursive(true)
-        .create(SYSTEM_MANAGER_STATE_DIR)?;
-
-    if Path::new(&state_file).is_file() {
-        log::info!("Reading state info from {}", state_file.display());
-        let reader = io::BufReader::new(fs::File::open(state_file)?);
-        match serde_json::from_reader(reader) {
-            Ok(linked_services) => return Ok(linked_services),
-            Err(e) => {
-                log::error!("Error reading the state file, ignoring.");
-                log::error!("{:?}", e);
-            }
-        }
-    }
-    Ok(HashMap::default())
-}
-
-fn stop_services(
-    service_manager: &systemd::ServiceManager,
-    services: &Services,
-) -> Result<HashSet<JobId>> {
+fn stop_services(service_manager: &systemd::ServiceManager, services: &Services) -> HashSet<JobId> {
     for_each_unit(
         |s| service_manager.stop_unit(s),
         convert_services(services),
@@ -271,7 +254,7 @@ fn stop_services(
 fn reload_services(
     service_manager: &systemd::ServiceManager,
     services: &Services,
-) -> Result<HashSet<JobId>> {
+) -> HashSet<JobId> {
     for_each_unit(
         |s| service_manager.reload_unit(s),
         convert_services(services),
@@ -282,7 +265,7 @@ fn reload_services(
 fn start_units(
     service_manager: &systemd::ServiceManager,
     units: &[systemd::UnitStatus],
-) -> Result<HashSet<JobId>> {
+) -> HashSet<JobId> {
     for_each_unit(
         |unit| service_manager.start_unit(unit),
         convert_units(units),
@@ -301,35 +284,32 @@ fn convert_units(units: &[systemd::UnitStatus]) -> Vec<&str> {
         .collect::<Vec<&str>>()
 }
 
-fn for_each_unit<'a, F, R, S>(action: F, units: S, log_action: &str) -> Result<HashSet<JobId>>
+fn for_each_unit<'a, F, R, S>(action: F, units: S, log_action: &str) -> HashSet<JobId>
 where
-    F: Fn(&str) -> Result<R>,
+    F: Fn(&str) -> anyhow::Result<R>,
     S: AsRef<[&'a str]>,
 {
-    let successful_services: HashSet<JobId> =
-        units
-            .as_ref()
-            .iter()
-            .fold(HashSet::new(), |mut set, unit| match action(unit) {
-                Ok(_) => {
-                    log::info!("Unit {}: {}...", unit, log_action);
-                    set.insert(JobId {
-                        id: (*unit).to_owned(),
-                    });
-                    set
-                }
-                Err(e) => {
-                    log::error!(
-                        "Service {}: error {log_action}, please consult the logs",
-                        unit
-                    );
-                    log::error!("{e}");
-                    set
-                }
-            });
-
     // TODO: do we want to propagate unit failures here in some way?
-    Ok(successful_services)
+    units
+        .as_ref()
+        .iter()
+        .fold(HashSet::new(), |mut set, unit| match action(unit) {
+            Ok(_) => {
+                log::info!("Unit {}: {}...", unit, log_action);
+                set.insert(JobId {
+                    id: (*unit).to_owned(),
+                });
+                set
+            }
+            Err(e) => {
+                log::error!(
+                    "Service {}: error {log_action}, please consult the logs",
+                    unit
+                );
+                log::error!("{e}");
+                set
+            }
+        })
 }
 
 fn wait_for_jobs(
@@ -337,7 +317,7 @@ fn wait_for_jobs(
     job_monitor: &systemd::JobMonitor,
     jobs: HashSet<JobId>,
     timeout: &Option<Duration>,
-) -> Result<()> {
+) -> anyhow::Result<()> {
     if !service_manager.monitor_jobs_finish(job_monitor, timeout, jobs)? {
         anyhow::bail!("Timeout waiting for systemd jobs");
     }

src/lib.rs

@@ -13,8 +13,7 @@ pub const PROFILE_DIR: &str = "/nix/var/nix/profiles/system-manager-profiles";
 pub const PROFILE_NAME: &str = "system-manager";
 pub const GCROOT_PATH: &str = "/nix/var/nix/gcroots/system-manager-current";
 pub const SYSTEM_MANAGER_STATE_DIR: &str = "/var/lib/system-manager/state";
-pub const SERVICES_STATE_FILE_NAME: &str = "services.json";
-pub const ETC_STATE_FILE_NAME: &str = "etc-files.json";
+pub const STATE_FILE_NAME: &str = "system-manager-state.json";
 pub const SYSTEM_MANAGER_STATIC_NAME: &str = ".system-manager-static";
 
 #[derive(PartialEq, Debug, Clone, Serialize, Deserialize)]

src/main.rs

@@ -294,8 +294,8 @@ fn check_root() -> Result<()> {
 }
 
 fn handle_toplevel_error<T>(r: Result<T>) -> ExitCode {
-    if r.is_err() {
-        log::error!("{:?}", r);
+    if let Err(e) = r {
+        log::error!("{:?}", e);
         return ExitCode::FAILURE;
     }
     ExitCode::SUCCESS