Add some clean up that improves coverage

crates/libcontainer/Cargo.toml

@@ -18,7 +18,7 @@ systemd = ["libcgroups/systemd", "v2"]
 v2 = ["libcgroups/v2"]
 v1 = ["libcgroups/v1"]
 cgroupsv2_devices = ["libcgroups/cgroupsv2_devices"]
-test_utils = ["rand"]
+test_utils = ["dep:rand"]
 
 [dependencies]
 bitflags = "2.3.1"

crates/libcontainer/src/utils.rs

@@ -1,16 +1,14 @@
 //! Utility functionality
 
 use std::collections::HashMap;
-use std::ffi::CString;
 use std::fs::{self, DirBuilder, File};
 use std::os::linux::fs::MetadataExt;
 use std::os::unix::fs::DirBuilderExt;
-use std::os::unix::prelude::{AsRawFd, OsStrExt};
+use std::os::unix::prelude::AsRawFd;
 use std::path::{Component, Path, PathBuf};
 
 use nix::sys::stat::Mode;
 use nix::sys::statfs;
-use nix::unistd;
 use nix::unistd::{Uid, User};
 
 #[derive(Debug, thiserror::Error)]
@@ -145,33 +143,6 @@ pub fn get_user_home(uid: u32) -> Option<PathBuf> {
     }
 }
 
-#[derive(Debug, thiserror::Error)]
-pub enum DoExecError {
-    #[error("failed to convert path to cstring")]
-    PathToCString {
-        source: std::ffi::NulError,
-        path: PathBuf,
-    },
-    #[error("failed to execvp")]
-    Execvp { source: nix::Error },
-}
-
-pub fn do_exec(path: impl AsRef<Path>, args: &[String]) -> Result<(), DoExecError> {
-    let p = CString::new(path.as_ref().as_os_str().as_bytes()).map_err(|e| {
-        DoExecError::PathToCString {
-            source: e,
-            path: path.as_ref().to_path_buf(),
-        }
-    })?;
-    let c_args: Vec<CString> = args
-        .iter()
-        .map(|s| CString::new(s.as_bytes()).unwrap_or_default())
-        .collect();
-    unistd::execvp(&p, &c_args).map_err(|err| DoExecError::Execvp { source: err })?;
-
-    Ok(())
-}
-
 /// If None, it will generate a default path for cgroups.
 pub fn get_cgroup_path(
     cgroups_path: &Option<PathBuf>,
@@ -187,49 +158,24 @@ pub fn get_cgroup_path(
     }
 }
 
-#[derive(Debug, thiserror::Error)]
-pub enum WrappedIOError {
-    #[error("failed to read from {path:?}")]
-    ReadFile {
-        source: std::io::Error,
-        path: PathBuf,
-    },
-    #[error("failed to write to {path:?}")]
-    WriteFile {
-        source: std::io::Error,
-        path: PathBuf,
-    },
-    #[error("failed to open {path:?}")]
-    Open {
-        source: std::io::Error,
-        path: PathBuf,
-    },
-    #[error("failed to create directory {path:?}")]
-    CreateDirAll {
-        source: std::io::Error,
-        path: PathBuf,
-    },
-    #[error("failed to get metadata")]
-    GetMetadata { source: std::io::Error },
-    #[error("metada doesn't match the expected attributes")]
-    MetadataMismatch,
-}
-
 pub fn write_file<P: AsRef<Path>, C: AsRef<[u8]>>(
     path: P,
     contents: C,
-) -> Result<(), WrappedIOError> {
-    fs::write(path.as_ref(), contents).map_err(|err| WrappedIOError::WriteFile {
-        source: err,
-        path: path.as_ref().to_path_buf(),
-    })
+) -> Result<(), std::io::Error> {
+    fs::write(path.as_ref(), contents).map_err(|err| {
+        tracing::error!(path = ?path.as_ref(), ?err, "failed to write file");
+        err
+    })?;
+
+    Ok(())
 }
 
-pub fn create_dir_all<P: AsRef<Path>>(path: P) -> Result<(), WrappedIOError> {
-    fs::create_dir_all(path.as_ref()).map_err(|err| WrappedIOError::CreateDirAll {
-        source: err,
-        path: path.as_ref().to_path_buf(),
-    })
+pub fn create_dir_all<P: AsRef<Path>>(path: P) -> Result<(), std::io::Error> {
+    fs::create_dir_all(path.as_ref()).map_err(|err| {
+        tracing::error!(path = ?path.as_ref(), ?err, "failed to create directory");
+        err
+    })?;
+    Ok(())
 }
 
 pub fn open<P: AsRef<Path>>(path: P) -> Result<File, std::io::Error> {
@@ -239,6 +185,14 @@ pub fn open<P: AsRef<Path>>(path: P) -> Result<File, std::io::Error> {
     })
 }
 
+#[derive(Debug, thiserror::Error)]
+pub enum MkdirWithModeError {
+    #[error("IO error")]
+    Io(#[from] std::io::Error),
+    #[error("metada doesn't match the expected attributes")]
+    MetadataMismatch,
+}
+
 /// Creates the specified directory and all parent directories with the specified mode. Ensures
 /// that the directory has been created with the correct mode and that the owner of the directory
 /// is the owner that has been specified
@@ -256,30 +210,23 @@ pub fn create_dir_all_with_mode<P: AsRef<Path>>(
     path: P,
     owner: u32,
     mode: Mode,
-) -> Result<(), WrappedIOError> {
+) -> Result<(), MkdirWithModeError> {
     let path = path.as_ref();
     if !path.exists() {
         DirBuilder::new()
             .recursive(true)
             .mode(mode.bits())
-            .create(path)
-            .map_err(|err| WrappedIOError::CreateDirAll {
-                source: err,
-                path: path.to_path_buf(),
-            })?;
+            .create(path)?;
     }
 
-    let metadata = path
-        .metadata()
-        .map_err(|err| WrappedIOError::GetMetadata { source: err })?;
-
+    let metadata = path.metadata()?;
     if metadata.is_dir()
         && metadata.st_uid() == owner
         && metadata.st_mode() & mode.bits() == mode.bits()
     {
         Ok(())
     } else {
-        Err(WrappedIOError::MetadataMismatch)
+        Err(MkdirWithModeError::MetadataMismatch)
     }
 }
 
@@ -314,7 +261,7 @@ pub fn ensure_procfs(path: &Path) -> Result<(), EnsureProcfsError> {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use anyhow::Result;
+    use anyhow::{bail, Result};
 
     #[test]
     pub fn test_get_unix_user() {
@@ -364,4 +311,57 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn test_create_dir_all_with_mode() -> Result<()> {
+        {
+            let temdir = tempfile::tempdir()?;
+            let path = temdir.path().join("test");
+            let uid = nix::unistd::getuid().as_raw();
+            let mode = Mode::S_IRWXU;
+            create_dir_all_with_mode(&path, uid, mode)?;
+            let metadata = path.metadata()?;
+            assert!(path.is_dir());
+            assert_eq!(metadata.st_uid(), uid);
+            assert_eq!(metadata.st_mode() & mode.bits(), mode.bits());
+        }
+        {
+            let temdir = tempfile::tempdir()?;
+            let path = temdir.path().join("test");
+            let mode = Mode::S_IRWXU;
+            std::fs::create_dir(&path)?;
+            assert!(path.is_dir());
+            match create_dir_all_with_mode(&path, 8899, mode) {
+                Err(MkdirWithModeError::MetadataMismatch) => {}
+                _ => bail!("should return MetadataMismatch"),
+            }
+        }
+        Ok(())
+    }
+
+    #[test]
+    fn test_io() -> Result<()> {
+        {
+            let tempdir = tempfile::tempdir()?;
+            let path = tempdir.path().join("test");
+            write_file(&path, "test".as_bytes())?;
+            open(&path)?;
+            assert!(create_dir_all(path).is_err());
+        }
+        {
+            let tempdir = tempfile::tempdir()?;
+            let path = tempdir.path().join("test");
+            create_dir_all(&path)?;
+            assert!(write_file(&path, "test".as_bytes()).is_err());
+        }
+        {
+            let tempdir = tempfile::tempdir()?;
+            let path = tempdir.path().join("test");
+            assert!(open(&path).is_err());
+            create_dir_all(&path)?;
+            assert!(path.is_dir())
+        }
+
+        Ok(())
+    }
 }

crates/libcontainer/src/workload/default.rs

@@ -38,7 +38,7 @@ impl Executor for DefaultExecutor {
             ExecutorError::Execution(err.into())
         })?;
 
-        // After do_exec is called, the process is replaced with the container
+        // After execvp is called, the process is replaced with the container
         // payload through execvp, so it should never reach here.
         unreachable!();
     }

docs/src/user/libcontainer.md

@@ -32,4 +32,4 @@ This exposes several modules, each dealing with a specific aspect of working wit
 
 - `tty` : this deals with setting up the tty for the container process.
 
-- `utils` : provides various utility functions, such as `parse_env` to parse the env variables, `do_exec` to do an exec syscall and execute a binary in the container process, `get_cgroups_path`, `create_dir_all_with_mode` etc.
+- `utils` : provides various utility functions, such as `parse_env` to parse the env variables, `get_cgroups_path`, `create_dir_all_with_mode` etc.