Merge pull request #1949 from yihuaf/yihuaf/syscall-error

Simplified syscall error

crates/libcontainer/src/error.rs

@@ -1,16 +1,3 @@
-/// 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),
-}
-
 #[derive(Debug, thiserror::Error)]
 pub enum MissingSpecError {
     #[error("missing process in spec")]

crates/libcontainer/src/namespaces.rs

@@ -7,7 +7,6 @@
 //! UTS (hostname and domain information, processes will think they're running on servers with different names),
 //! Cgroup (Resource limits, execution priority etc.)
 
-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};
@@ -17,12 +16,12 @@ type Result<T> = std::result::Result<T, NamespaceError>;
 
 #[derive(Debug, thiserror::Error)]
 pub enum NamespaceError {
-    #[error("failed to set namespace")]
-    ApplyNamespaceSyscallFailed {
-        namespace: Box<LinuxNamespace>,
-        #[source]
-        err: UnifiedSyscallError,
-    },
+    #[error(transparent)]
+    Nix(#[from] nix::Error),
+    #[error(transparent)]
+    IO(#[from] std::io::Error),
+    #[error(transparent)]
+    Syscall(#[from] crate::syscall::SyscallError),
 }
 
 static ORDERED_NAMESPACES: &[CloneFlags] = &[
@@ -88,28 +87,28 @@ impl Namespaces {
         match namespace.path() {
             Some(path) => {
                 let fd = fcntl::open(path, fcntl::OFlag::empty(), stat::Mode::empty()).map_err(
-                    |err| NamespaceError::ApplyNamespaceSyscallFailed {
-                        namespace: Box::new(namespace.to_owned()),
-                        err: err.into(),
+                    |err| {
+                        tracing::error!(?err, ?namespace, "failed to open namespace file");
+                        err
                     },
                 )?;
                 self.command
                     .set_ns(fd, get_clone_flag(namespace.typ()))
-                    .map_err(|err| NamespaceError::ApplyNamespaceSyscallFailed {
-                        namespace: Box::new(namespace.to_owned()),
-                        err: err.into(),
+                    .map_err(|err| {
+                        tracing::error!(?err, ?namespace, "failed to set namespace");
+                        err
                     })?;
-                unistd::close(fd).map_err(|err| NamespaceError::ApplyNamespaceSyscallFailed {
-                    namespace: Box::new(namespace.to_owned()),
-                    err: err.into(),
+                unistd::close(fd).map_err(|err| {
+                    tracing::error!(?err, ?namespace, "failed to close namespace file");
+                    err
                 })?;
             }
             None => {
                 self.command
                     .unshare(get_clone_flag(namespace.typ()))
-                    .map_err(|err| NamespaceError::ApplyNamespaceSyscallFailed {
-                        namespace: Box::new(namespace.to_owned()),
-                        err: err.into(),
+                    .map_err(|err| {
+                        tracing::error!(?err, ?namespace, "failed to unshare namespace");
+                        err
                     })?;
             }
         }

crates/libcontainer/src/process/container_init_process.rs

@@ -170,7 +170,7 @@ fn readonly_path(path: &Path, syscall: &dyn Syscall) -> Result<()> {
         MsFlags::MS_BIND | MsFlags::MS_REC,
         None,
     ) {
-        if let SyscallError::Mount { source: errno } = err {
+        if let SyscallError::Nix(errno) = err {
             // ignore error if path is not exist.
             if matches!(errno, nix::errno::Errno::ENOENT) {
                 return Ok(());
@@ -214,15 +214,11 @@ fn masked_path(path: &Path, mount_label: &Option<String>, syscall: &dyn Syscall)
         None,
     ) {
         match err {
-            SyscallError::Mount {
-                source: nix::errno::Errno::ENOENT,
-            } => {
+            SyscallError::Nix(nix::errno::Errno::ENOENT) => {
                 // ignore error if path is not exist.
                 tracing::warn!("masked path {:?} not exist", path);
             }
-            SyscallError::Mount {
-                source: nix::errno::Errno::ENOTDIR,
-            } => {
+            SyscallError::Nix(nix::errno::Errno::ENOTDIR) => {
                 let label = match mount_label {
                     Some(l) => format!("context=\"{l}\""),
                     None => "".to_string(),
@@ -968,9 +964,7 @@ mod tests {
             .downcast_ref::<TestHelperSyscall>()
             .unwrap();
         mocks.set_ret_err(ArgName::Mount, || {
-            Err(SyscallError::Mount {
-                source: nix::errno::Errno::ENOENT,
-            })
+            Err(SyscallError::Nix(nix::errno::Errno::ENOENT))
         });
 
         assert!(masked_path(Path::new("/proc/self"), &None, syscall.as_ref()).is_ok());
@@ -986,9 +980,7 @@ mod tests {
             .downcast_ref::<TestHelperSyscall>()
             .unwrap();
         mocks.set_ret_err(ArgName::Mount, || {
-            Err(SyscallError::Mount {
-                source: nix::errno::Errno::ENOTDIR,
-            })
+            Err(SyscallError::Nix(nix::errno::Errno::ENOTDIR))
         });
 
         assert!(masked_path(Path::new("/proc/self"), &None, syscall.as_ref()).is_ok());
@@ -1013,9 +1005,7 @@ mod tests {
             .downcast_ref::<TestHelperSyscall>()
             .unwrap();
         mocks.set_ret_err(ArgName::Mount, || {
-            Err(SyscallError::Mount {
-                source: nix::errno::Errno::ENOTDIR,
-            })
+            Err(SyscallError::Nix(nix::errno::Errno::ENOTDIR))
         });
 
         assert!(masked_path(
@@ -1045,9 +1035,7 @@ mod tests {
             .downcast_ref::<TestHelperSyscall>()
             .unwrap();
         mocks.set_ret_err(ArgName::Mount, || {
-            Err(SyscallError::Mount {
-                source: nix::errno::Errno::UnknownErrno,
-            })
+            Err(SyscallError::Nix(nix::errno::Errno::UnknownErrno))
         });
 
         assert!(masked_path(Path::new("/proc/self"), &None, syscall.as_ref()).is_err());

crates/libcontainer/src/rootfs/device.rs

@@ -14,13 +14,11 @@ use std::path::{Path, PathBuf};
 pub enum DeviceError {
     #[error("{0:?} is not a valid device path")]
     InvalidDevicePath(std::path::PathBuf),
-    #[error("failed to bind dev")]
-    BindDev {
-        source: crate::error::UnifiedSyscallError,
-    },
     #[error("failed syscall to create device")]
     Syscall(#[from] crate::syscall::SyscallError),
     #[error(transparent)]
+    Nix(#[from] nix::Error),
+    #[error(transparent)]
     Other(Box<dyn std::error::Error + Send + Sync>),
     #[error("{0}")]
     Custom(String),
@@ -91,9 +89,9 @@ impl Device {
         )
         .map_err(|err| {
             tracing::error!("failed to open bind dev {:?}: {}", full_container_path, err);
-            DeviceError::BindDev { source: err.into() }
+            err
         })?;
-        close(fd).map_err(|err| DeviceError::BindDev { source: err.into() })?;
+        close(fd)?;
         self.syscall
             .mount(
                 Some(dev.path()),
@@ -108,7 +106,7 @@ impl Device {
                     full_container_path,
                     err
                 );
-                DeviceError::BindDev { source: err.into() }
+                err
             })?;
 
         Ok(())

crates/libcontainer/src/rootfs/mount.rs

@@ -522,7 +522,7 @@ impl Mount {
             self.syscall
                 .mount(Some(&*src), dest, typ, mount_option_config.flags, Some(&*d))
         {
-            if let SyscallError::Mount { source: errno, .. } = err {
+            if let SyscallError::Nix(errno) = err {
                 if !matches!(errno, Errno::EINVAL) {
                     tracing::error!("mount of {:?} failed. {}", m.destination(), errno);
                     return Err(err.into());