Implemented more thiserror for libcontainer (Part 2) (#1881)

Implemented more thiserror for the libcontainer. This PR covers most of the low hanging fruits with smaller crates. Note, I decided to add a new syscall error wrapper that wraps the nix::Error, std::io::Error, and our own syscall crate. This aims to simplify where we call syscalls from 3 different sources and can simplify/unify many errors. --------- Signed-off-by: yihuaf <[email protected]>
youki-dev · May 8, 2023 · 5c31fae · 5c31fae
1 parent a387071
commit 5c31fae
Show file tree

Hide file tree

Showing 12 changed files with 330 additions and 266 deletions.
diff --git a/crates/libcontainer/src/apparmor.rs b/crates/libcontainer/src/apparmor.rs
@@ -1,17 +1,36 @@
-use anyhow::{Context, Result};
+use crate::utils;
 use std::{
     fs::{self},
     path::Path,
 };
 
-use crate::utils;
+#[derive(Debug, thiserror::Error)]
+pub enum AppArmorError {
+    #[error("failed to apply AppArmor profile")]
+    ApplyProfile {
+        path: std::path::PathBuf,
+        profile: String,
+        // TODO: fix this after `utils` crate is migrated to `thiserror`
+        source: anyhow::Error,
+    },
+    #[error("failed to read AppArmor profile: {source} {path}")]
+    ReadProfile {
+        path: String,
+        source: std::io::Error,
+    },
+}
+
+type Result<T> = std::result::Result<T, AppArmorError>;
 
 const ENABLED_PARAMETER_PATH: &str = "/sys/module/apparmor/parameters/enabled";
 
 /// Checks if AppArmor has been enabled on the system.
 pub fn is_enabled() -> Result<bool> {
-    let aa_enabled = fs::read_to_string(ENABLED_PARAMETER_PATH)
-        .with_context(|| format!("could not read {ENABLED_PARAMETER_PATH}"))?;
+    let aa_enabled =
+        fs::read_to_string(ENABLED_PARAMETER_PATH).map_err(|e| AppArmorError::ReadProfile {
+            path: ENABLED_PARAMETER_PATH.to_string(),
+            source: e,
+        })?;
     Ok(aa_enabled.starts_with('Y'))
 }
 
@@ -32,6 +51,14 @@ pub fn apply_profile(profile: &str) -> Result<()> {
 }
 
 fn activate_profile(path: &Path, profile: &str) -> Result<()> {
-    utils::ensure_procfs(path)?;
-    utils::write_file(path, format!("exec {profile}"))
+    utils::ensure_procfs(path).map_err(|err| AppArmorError::ApplyProfile {
+        path: path.to_owned(),
+        profile: profile.to_owned(),
+        source: err,
+    })?;
+    utils::write_file(path, format!("exec {profile}")).map_err(|err| AppArmorError::ApplyProfile {
+        path: path.to_owned(),
+        profile: profile.to_owned(),
+        source: err,
+    })
 }
diff --git a/crates/libcontainer/src/config.rs b/crates/libcontainer/src/config.rs
@@ -1,15 +1,39 @@
+use crate::utils;
+use oci_spec::runtime::{Hooks, Spec};
+use serde::{Deserialize, Serialize};
 use std::{
     fs,
     io::{BufReader, BufWriter, Write},
     path::{Path, PathBuf},
 };
 
-use anyhow::{Context, Result};
-use serde::{Deserialize, Serialize};
-
-use oci_spec::runtime::{Hooks, Spec};
+#[derive(Debug, thiserror::Error)]
+pub enum ConfigError {
+    #[error("failed to save config")]
+    SaveIO {
+        source: std::io::Error,
+        path: PathBuf,
+    },
+    #[error("failed to save config")]
+    SaveEncode {
+        source: serde_json::Error,
+        path: PathBuf,
+    },
+    #[error("failed to parse config")]
+    LoadIO {
+        source: std::io::Error,
+        path: PathBuf,
+    },
+    #[error("failed to parse config")]
+    LoadParse {
+        source: serde_json::Error,
+        path: PathBuf,
+    },
+    #[error("missing linux in spec")]
+    MissingLinux,
+}
 
-use crate::utils;
+type Result<T> = std::result::Result<T, ConfigError>;
 
 const YOUKI_CONFIG_NAME: &str = "youki_config.json";
 
@@ -29,7 +53,7 @@ impl<'a> YoukiConfig {
             cgroup_path: utils::get_cgroup_path(
                 spec.linux()
                     .as_ref()
-                    .context("no linux in spec")?
+                    .ok_or(ConfigError::MissingLinux)?
                     .cgroups_path(),
                 container_id,
                 rootless,
@@ -38,19 +62,37 @@ impl<'a> YoukiConfig {
     }
 
     pub fn save<P: AsRef<Path>>(&self, path: P) -> Result<()> {
-        let file = fs::File::create(path.as_ref().join(YOUKI_CONFIG_NAME))?;
+        let file = fs::File::create(path.as_ref().join(YOUKI_CONFIG_NAME)).map_err(|err| {
+            ConfigError::SaveIO {
+                source: err,
+                path: path.as_ref().to_owned(),
+            }
+        })?;
         let mut writer = BufWriter::new(file);
-        serde_json::to_writer(&mut writer, self)?;
-        writer.flush()?;
+        serde_json::to_writer(&mut writer, self).map_err(|err| ConfigError::SaveEncode {
+            source: err,
+            path: path.as_ref().to_owned(),
+        })?;
+        writer.flush().map_err(|err| ConfigError::SaveIO {
+            source: err,
+            path: path.as_ref().to_owned(),
+        })?;
+
         Ok(())
     }
 
     pub fn load<P: AsRef<Path>>(path: P) -> Result<Self> {
         let path = path.as_ref();
-        let file = fs::File::open(path.join(YOUKI_CONFIG_NAME))?;
+        let file =
+            fs::File::open(path.join(YOUKI_CONFIG_NAME)).map_err(|err| ConfigError::LoadIO {
+                source: err,
+                path: path.to_owned(),
+            })?;
         let reader = BufReader::new(file);
-        let config = serde_json::from_reader(reader)
-            .with_context(|| format!("failed to load config from {path:?}"))?;
+        let config = serde_json::from_reader(reader).map_err(|err| ConfigError::LoadParse {
+            source: err,
+            path: path.to_owned(),
+        })?;
         Ok(config)
     }
 }

diff --git a/crates/libcontainer/src/error.rs b/crates/libcontainer/src/error.rs
@@ -0,0 +1,12 @@
+/// UnifiedSyscallError aims to simplify error handling of syscalls in
+/// libcontainer. In many occasions, we mix nix::Error, std::io::Error and our
+/// own syscall wrappers, which makes error handling complicated.
+#[derive(Debug, thiserror::Error)]
+pub enum UnifiedSyscallError {
+    #[error(transparent)]
+    Io(#[from] std::io::Error),
+    #[error(transparent)]
+    Nix(#[from] nix::Error),
+    #[error(transparent)]
+    Syscall(#[from] crate::syscall::SyscallError),
+}
diff --git a/crates/libcontainer/src/hooks.rs b/crates/libcontainer/src/hooks.rs
@@ -1,29 +1,38 @@
-use anyhow::{bail, Context, Result};
 use nix::{sys::signal, unistd::Pid};
 use oci_spec::runtime::Hook;
 use std::{
-    collections::HashMap, fmt, io::ErrorKind, io::Write, os::unix::prelude::CommandExt, process,
+    collections::HashMap,
+    io::ErrorKind,
+    io::Write,
+    os::unix::prelude::CommandExt,
+    process::{self},
     thread, time,
 };
 
 use crate::{container::Container, utils};
-// A special error used to signal a timeout. We want to differentiate between a
-// timeout vs. other error.
-#[derive(Debug)]
-pub struct HookTimeoutError;
-impl std::error::Error for HookTimeoutError {}
-impl fmt::Display for HookTimeoutError {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        "hook command timeout".fmt(f)
-    }
+
+#[derive(Debug, thiserror::Error)]
+pub enum HookError {
+    #[error("failed to execute hook command")]
+    CommandExecute(#[source] std::io::Error),
+    #[error("failed to encode container state")]
+    EncodeContainerState(#[source] serde_json::Error),
+    #[error("hook command exited with non-zero exit code: {0}")]
+    NonZeroExitCode(i32),
+    #[error("hook command was killed by a signal")]
+    Killed,
+    #[error("failed to execute hook command due to a timeout")]
+    Timeout,
+    #[error("container state is required to run hook")]
+    MissingContainerState,
+    #[error("failed to write container state to stdin")]
+    WriteContainerState(#[source] std::io::Error),
 }
 
-pub fn run_hooks(hooks: Option<&Vec<Hook>>, container: Option<&Container>) -> Result<()> {
-    if container.is_none() {
-        bail!("container state is required to run hook");
-    }
+type Result<T> = std::result::Result<T, HookError>;
 
-    let state = &container.unwrap().state;
+pub fn run_hooks(hooks: Option<&Vec<Hook>>, container: Option<&Container>) -> Result<()> {
+    let state = &(container.ok_or(HookError::MissingContainerState)?.state);
 
     if let Some(hooks) = hooks {
         for hook in hooks {
@@ -53,7 +62,7 @@ pub fn run_hooks(hooks: Option<&Vec<Hook>>, container: Option<&Container>) -> Re
                 .envs(envs)
                 .stdin(process::Stdio::piped())
                 .spawn()
-                .with_context(|| "Failed to execute hook")?;
+                .map_err(HookError::CommandExecute)?;
             let hook_process_pid = Pid::from_raw(hook_process.id() as i32);
             // Based on the OCI spec, we need to pipe the container state into
             // the hook command through stdin.
@@ -67,13 +76,13 @@ pub fn run_hooks(hooks: Option<&Vec<Hook>>, container: Option<&Container>) -> Re
                 // error, in the case that the hook command is waiting for us to
                 // write to stdin.
                 let encoded_state =
-                    serde_json::to_string(state).context("failed to encode container state")?;
+                    serde_json::to_string(state).map_err(HookError::EncodeContainerState)?;
                 if let Err(e) = stdin.write_all(encoded_state.as_bytes()) {
                     if e.kind() != ErrorKind::BrokenPipe {
                         // Not a broken pipe. The hook command may be waiting
                         // for us.
                         let _ = signal::kill(hook_process_pid, signal::Signal::SIGKILL);
-                        bail!("failed to write container state to stdin: {:?}", e);
+                        return Err(HookError::WriteContainerState(e));
                     }
                 }
             }
@@ -100,7 +109,7 @@ pub fn run_hooks(hooks: Option<&Vec<Hook>>, container: Option<&Container>) -> Re
                         // Kill the process. There is no need to further clean
                         // up because we will be error out.
                         let _ = signal::kill(hook_process_pid, signal::Signal::SIGKILL);
-                        return Err(HookTimeoutError.into());
+                        return Err(HookError::Timeout);
                     }
                     Err(_) => {
                         unreachable!();
@@ -112,21 +121,12 @@ pub fn run_hooks(hooks: Option<&Vec<Hook>>, container: Option<&Container>) -> Re
 
             match res {
                 Ok(exit_status) => match exit_status.code() {
-                    Some(0) => {}
-                    Some(exit_code) => {
-                        bail!(
-                            "Failed to execute hook command. Non-zero return code. {:?}",
-                            exit_code
-                        );
-                    }
-                    None => {
-                        bail!("Process is killed by signal");
-                    }
+                    Some(0) => Ok(()),
+                    Some(exit_code) => Err(HookError::NonZeroExitCode(exit_code)),
+                    None => Err(HookError::Killed),
                 },
-                Err(e) => {
-                    bail!("Failed to execute hook command: {:?}", e);
-                }
-            }
+                Err(e) => Err(HookError::CommandExecute(e)),
+            }?;
         }
     }
 
@@ -136,7 +136,7 @@ pub fn run_hooks(hooks: Option<&Vec<Hook>>, container: Option<&Container>) -> Re
 #[cfg(test)]
 mod test {
     use super::*;
-    use anyhow::{bail, Result};
+    use anyhow::{bail, Context, Result};
     use oci_spec::runtime::HookBuilder;
     use serial_test::serial;
     use std::{env, fs};
@@ -221,14 +221,14 @@ mod test {
             Ok(_) => {
                 bail!("The test expects the hook to error out with timeout. Should not execute cleanly");
             }
+            Err(HookError::Timeout) => {}
             Err(err) => {
-                // We want to make sure the error returned is indeed timeout
-                // error. All other errors are considered failure.
-                if !err.is::<HookTimeoutError>() {
-                    bail!("Failed to execute hook: {:?}", err);
-                }
+                bail!(
+                    "The test expects the hook to error out with timeout. Got error: {}",
+                    err
+                );
             }
-        }
+        };
 
         Ok(())
     }

diff --git a/crates/libcontainer/src/lib.rs b/crates/libcontainer/src/lib.rs
@@ -2,6 +2,7 @@ pub mod apparmor;
 pub mod capabilities;
 pub mod config;
 pub mod container;
+pub mod error;
 pub mod hooks;
 pub mod namespaces;
 pub mod notify_socket;

diff --git a/crates/libcontainer/src/namespaces.rs b/crates/libcontainer/src/namespaces.rs
@@ -7,7 +7,8 @@
 //! UTS (hostname and domain information, processes will think they're running on servers with different names),
 //! Cgroup (Resource limits, execution priority etc.)
 
-use crate::syscall::{syscall::create_syscall, Syscall, SyscallError};
+use crate::error::UnifiedSyscallError;
+use crate::syscall::{syscall::create_syscall, Syscall};
 use nix::{fcntl, sched::CloneFlags, sys::stat, unistd};
 use oci_spec::runtime::{LinuxNamespace, LinuxNamespaceType};
 use std::collections;
@@ -20,13 +21,7 @@ pub enum NamespaceError {
     ApplyNamespaceSyscallFailed {
         namespace: Box<LinuxNamespace>,
         #[source]
-        err: SyscallError,
-    },
-    #[error("failed to set namespace")]
-    ApplyNamespaceUnixSyscallFailed {
-        namespace: Box<LinuxNamespace>,
-        #[source]
-        err: nix::Error,
+        err: UnifiedSyscallError,
     },
 }
 
@@ -93,30 +88,28 @@ impl Namespaces {
         match namespace.path() {
             Some(path) => {
                 let fd = fcntl::open(path, fcntl::OFlag::empty(), stat::Mode::empty()).map_err(
-                    |err| NamespaceError::ApplyNamespaceUnixSyscallFailed {
+                    |err| NamespaceError::ApplyNamespaceSyscallFailed {
                         namespace: Box::new(namespace.to_owned()),
-                        err,
+                        err: err.into(),
                     },
                 )?;
                 self.command
                     .set_ns(fd, get_clone_flag(namespace.typ()))
                     .map_err(|err| NamespaceError::ApplyNamespaceSyscallFailed {
                         namespace: Box::new(namespace.to_owned()),
-                        err,
+                        err: err.into(),
                     })?;
-                unistd::close(fd).map_err(|err| {
-                    NamespaceError::ApplyNamespaceUnixSyscallFailed {
-                        namespace: Box::new(namespace.to_owned()),
-                        err,
-                    }
+                unistd::close(fd).map_err(|err| NamespaceError::ApplyNamespaceSyscallFailed {
+                    namespace: Box::new(namespace.to_owned()),
+                    err: err.into(),
                 })?;
             }
             None => {
                 self.command
                     .unshare(get_clone_flag(namespace.typ()))
                     .map_err(|err| NamespaceError::ApplyNamespaceSyscallFailed {
                         namespace: Box::new(namespace.to_owned()),
-                        err,
+                        err: err.into(),
                     })?;
             }
         }