From 6ffa8283f03444c6c65b89247ba571b2825b4eb3 Mon Sep 17 00:00:00 2001 From: Tim Zhang Date: Tue, 13 Oct 2020 14:27:29 +0800 Subject: [PATCH 01/12] agent: replace `if let Err` with `map_err` Fixes #934 Signed-off-by: Tim Zhang --- src/agent/rustjail/src/capabilities.rs | 7 ++- src/agent/rustjail/src/container.rs | 43 +++++++--------- src/agent/src/main.rs | 12 ++--- src/agent/src/mount.rs | 5 +- src/agent/src/namespace.rs | 10 ++-- src/agent/src/rpc.rs | 70 ++++++++++---------------- src/agent/src/uevent.rs | 6 +-- 7 files changed, 62 insertions(+), 91 deletions(-) diff --git a/src/agent/rustjail/src/capabilities.rs b/src/agent/rustjail/src/capabilities.rs index f9203efe1b..91f6ea823c 100644 --- a/src/agent/rustjail/src/capabilities.rs +++ b/src/agent/rustjail/src/capabilities.rs @@ -126,13 +126,12 @@ pub fn drop_privileges(cfd_log: RawFd, caps: &LinuxCapabilities) -> Result<()> { ) .map_err(|e| anyhow!(e.to_string()))?; - if let Err(_) = caps::set( + let _ = caps::set( None, CapSet::Ambient, to_capshashset(cfd_log, caps.ambient.as_ref()), - ) { - log_child!(cfd_log, "failed to set ambient capability"); - } + ) + .map_err(|_| log_child!(cfd_log, "failed to set ambient capability")); Ok(()) } diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 377c8124a0..91657e322a 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -3,7 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 // -use anyhow::{anyhow, bail, Context, Result}; +use anyhow::{anyhow, Context, Result}; use dirs; use lazy_static; use libc::pid_t; @@ -457,9 +457,8 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { // Ref: https://github.com/opencontainers/runc/commit/50a19c6ff828c58e5dab13830bd3dacde268afe5 // if !nses.is_empty() { - if let Err(e) = prctl::set_dumpable(false) { - return Err(anyhow!(e).context("set process non-dumpable failed")); - }; + prctl::set_dumpable(false) + .map_err(|e| anyhow!(e).context("set process non-dumpable failed"))?; } if userns { @@ -590,9 +589,7 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { // NoNewPeiviledges, Drop capabilities if oci_process.no_new_privileges { - if let Err(_) = prctl::set_no_new_privileges(true) { - return Err(anyhow!("cannot set no new privileges")); - } + prctl::set_no_new_privileges(true).map_err(|_| anyhow!("cannot set no new privileges"))?; } if oci_process.capabilities.is_some() { @@ -1074,14 +1071,12 @@ fn do_exec(args: &[String]) -> ! { .collect(); let a: Vec<&CStr> = sa.iter().map(|s| s.as_c_str()).collect(); - if let Err(e) = unistd::execvp(p.as_c_str(), a.as_slice()) { - match e { - nix::Error::Sys(errno) => { - std::process::exit(errno as i32); - } - _ => std::process::exit(-2), + let _ = unistd::execvp(p.as_c_str(), a.as_slice()).map_err(|e| match e { + nix::Error::Sys(errno) => { + std::process::exit(errno as i32); } - } + _ => std::process::exit(-2), + }); unreachable!() } @@ -1291,9 +1286,9 @@ fn write_mappings(logger: &Logger, path: &str, maps: &[LinuxIDMapping]) -> Resul fn setid(uid: Uid, gid: Gid) -> Result<()> { // set uid/gid - if let Err(e) = prctl::set_keep_capabilities(true) { - bail!(anyhow!(e).context("set keep capabilities returned")); - }; + prctl::set_keep_capabilities(true) + .map_err(|e| anyhow!(e).context("set keep capabilities returned"))?; + { unistd::setresgid(gid, gid, gid)?; } @@ -1305,9 +1300,9 @@ fn setid(uid: Uid, gid: Gid) -> Result<()> { capabilities::reset_effective()?; } - if let Err(e) = prctl::set_keep_capabilities(false) { - bail!(anyhow!(e).context("set keep capabilities returned")); - }; + prctl::set_keep_capabilities(false) + .map_err(|e| anyhow!(e).context("set keep capabilities returned"))?; + Ok(()) } @@ -1325,13 +1320,13 @@ impl LinuxContainer { // validate oci spec validator::validate(&config)?; - if let Err(e) = fs::create_dir_all(root.as_str()) { + fs::create_dir_all(root.as_str()).map_err(|e| { if e.kind() == std::io::ErrorKind::AlreadyExists { - return Err(e).context(format!("container {} already exists", id.as_str())); + return anyhow!(e).context(format!("container {} already exists", id.as_str())); } - return Err(e).context(format!("fail to create container directory {}", root)); - } + anyhow!(e).context(format!("fail to create container directory {}", root)) + })?; unistd::chown( root.as_str(), diff --git a/src/agent/src/main.rs b/src/agent/src/main.rs index 0dc8667f84..45c73c1cf1 100644 --- a/src/agent/src/main.rs +++ b/src/agent/src/main.rs @@ -512,14 +512,12 @@ fn run_debug_console_shell(logger: &Logger, shell: &str, socket_fd: RawFd) -> Re let args: Vec<&CStr> = vec![]; // run shell - if let Err(e) = unistd::execvp(cmd.as_c_str(), args.as_slice()) { - match e { - nix::Error::Sys(errno) => { - std::process::exit(errno as i32); - } - _ => std::process::exit(-2), + let _ = unistd::execvp(cmd.as_c_str(), args.as_slice()).map_err(|e| match e { + nix::Error::Sys(errno) => { + std::process::exit(errno as i32); } - } + _ => std::process::exit(-2), + }); } Ok(ForkResult::Parent { child: child_pid }) => { diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index 8e988af146..45d89bd520 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -251,10 +251,7 @@ fn ephemeral_storage_handler( return Ok("".to_string()); } - if let Err(err) = fs::create_dir_all(Path::new(&storage.mount_point)) { - return Err(err.into()); - } - + fs::create_dir_all(Path::new(&storage.mount_point))?; common_storage_handler(logger, storage)?; Ok("".to_string()) diff --git a/src/agent/src/namespace.rs b/src/agent/src/namespace.rs index 892332b3d7..6cb42aa768 100644 --- a/src/agent/src/namespace.rs +++ b/src/agent/src/namespace.rs @@ -131,12 +131,12 @@ impl Namespace { }; let bare_mount = BareMount::new(source, destination, "none", flags, "", &logger); - if let Err(err) = bare_mount.mount() { - return Err(format!( + bare_mount.mount().map_err(|e| { + format!( "Failed to mount {} to {} with err:{:?}", - source, destination, err - )); - } + source, destination, e + ) + })?; Ok(()) }); diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index d7b95cd7b6..ed383b21ae 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -258,15 +258,12 @@ impl agentService { }); }); - if let Err(_) = rx.recv_timeout(Duration::from_secs(req.timeout as u64)) { - return Err(anyhow!(nix::Error::from_errno(nix::errno::Errno::ETIME))); - } + rx.recv_timeout(Duration::from_secs(req.timeout as u64)) + .map_err(|_| anyhow!(nix::Error::from_errno(nix::errno::Errno::ETIME)))?; - if let Err(_) = handle.join() { - return Err(anyhow!(nix::Error::from_errno( - nix::errno::Errno::UnknownErrno - ))); - } + handle + .join() + .map_err(|_| anyhow!(nix::Error::from_errno(nix::errno::Errno::UnknownErrno)))?; let s = self.sandbox.clone(); let mut sandbox = s.lock().unwrap(); @@ -903,12 +900,12 @@ impl protocols::agent_ttrpc::AgentService for agentService { }; let err = libc::ioctl(fd, TIOCSWINSZ, &win); - if let Err(e) = Errno::result(err).map(drop) { - return Err(ttrpc::Error::RpcStatus(ttrpc::get_status( + Errno::result(err).map(drop).map_err(|e| { + ttrpc::Error::RpcStatus(ttrpc::get_status( ttrpc::Code::INTERNAL, format!("ioctl error: {:?}", e), - ))); - } + )) + })?; } Ok(Empty::new()) @@ -1062,12 +1059,12 @@ impl protocols::agent_ttrpc::AgentService for agentService { s.running = true; if !req.guest_hook_path.is_empty() { - if let Err(e) = s.add_hooks(&req.guest_hook_path) { + let _ = s.add_hooks(&req.guest_hook_path).map_err(|e| { error!( sl!(), "add guest hook {} failed: {:?}", req.guest_hook_path, e ); - } + }); } if req.sandbox_id.len() > 0 { @@ -1168,12 +1165,9 @@ impl protocols::agent_ttrpc::AgentService for agentService { let s = Arc::clone(&self.sandbox); let sandbox = s.lock().unwrap(); - if let Err(e) = sandbox.online_cpu_memory(&req) { - return Err(ttrpc::Error::RpcStatus(ttrpc::get_status( - ttrpc::Code::INTERNAL, - e.to_string(), - ))); - } + sandbox.online_cpu_memory(&req).map_err(|e| { + ttrpc::Error::RpcStatus(ttrpc::get_status(ttrpc::Code::INTERNAL, e.to_string())) + })?; Ok(Empty::new()) } @@ -1183,12 +1177,9 @@ impl protocols::agent_ttrpc::AgentService for agentService { _ctx: &ttrpc::TtrpcContext, req: protocols::agent::ReseedRandomDevRequest, ) -> ttrpc::Result { - if let Err(e) = random::reseed_rng(req.data.as_slice()) { - return Err(ttrpc::Error::RpcStatus(ttrpc::get_status( - ttrpc::Code::INTERNAL, - e.to_string(), - ))); - } + random::reseed_rng(req.data.as_slice()).map_err(|e| { + ttrpc::Error::RpcStatus(ttrpc::get_status(ttrpc::Code::INTERNAL, e.to_string())) + })?; Ok(Empty::new()) } @@ -1227,12 +1218,9 @@ impl protocols::agent_ttrpc::AgentService for agentService { _ctx: &ttrpc::TtrpcContext, req: protocols::agent::MemHotplugByProbeRequest, ) -> ttrpc::Result { - if let Err(e) = do_mem_hotplug_by_probe(&req.memHotplugProbeAddr) { - return Err(ttrpc::Error::RpcStatus(ttrpc::get_status( - ttrpc::Code::INTERNAL, - e.to_string(), - ))); - } + do_mem_hotplug_by_probe(&req.memHotplugProbeAddr).map_err(|e| { + ttrpc::Error::RpcStatus(ttrpc::get_status(ttrpc::Code::INTERNAL, e.to_string())) + })?; Ok(Empty::new()) } @@ -1242,12 +1230,9 @@ impl protocols::agent_ttrpc::AgentService for agentService { _ctx: &ttrpc::TtrpcContext, req: protocols::agent::SetGuestDateTimeRequest, ) -> ttrpc::Result { - if let Err(e) = do_set_guest_date_time(req.Sec, req.Usec) { - return Err(ttrpc::Error::RpcStatus(ttrpc::get_status( - ttrpc::Code::INTERNAL, - e.to_string(), - ))); - } + do_set_guest_date_time(req.Sec, req.Usec).map_err(|e| { + ttrpc::Error::RpcStatus(ttrpc::get_status(ttrpc::Code::INTERNAL, e.to_string())) + })?; Ok(Empty::new()) } @@ -1257,12 +1242,9 @@ impl protocols::agent_ttrpc::AgentService for agentService { _ctx: &ttrpc::TtrpcContext, req: protocols::agent::CopyFileRequest, ) -> ttrpc::Result { - if let Err(e) = do_copy_file(&req) { - return Err(ttrpc::Error::RpcStatus(ttrpc::get_status( - ttrpc::Code::INTERNAL, - e.to_string(), - ))); - } + do_copy_file(&req).map_err(|e| { + ttrpc::Error::RpcStatus(ttrpc::get_status(ttrpc::Code::INTERNAL, e.to_string())) + })?; Ok(Empty::new()) } diff --git a/src/agent/src/uevent.rs b/src/agent/src/uevent.rs index de79705ec4..35e8515633 100644 --- a/src/agent/src/uevent.rs +++ b/src/agent/src/uevent.rs @@ -99,14 +99,14 @@ impl Uevent { let online_path = format!("{}/{}/online", SYSFS_DIR, &self.devpath); // It's a memory hot-add event. if online_path.starts_with(SYSFS_MEMORY_ONLINE_PATH) { - if let Err(e) = online_device(online_path.as_ref()) { + let _ = online_device(online_path.as_ref()).map_err(|e| { error!( *logger, "failed to online device"; "device" => &self.devpath, "error" => format!("{}", e), - ); - } + ) + }); return; } } From a3c64e5ce5d63fadfcd759e69575dabaf616b596 Mon Sep 17 00:00:00 2001 From: Tim Zhang Date: Tue, 13 Oct 2020 14:35:22 +0800 Subject: [PATCH 02/12] agent: replace `if let Err` with `or_else` Fixes #934 Signed-off-by: Tim Zhang --- src/agent/rustjail/src/container.rs | 11 +++++++---- src/agent/src/mount.rs | 9 ++++++--- src/agent/src/rpc.rs | 8 +++++--- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 91657e322a..4e80799a07 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -487,23 +487,26 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { } log_child!(cfd_log, "join namespace {:?}", s); - if let Err(e) = sched::setns(fd, s) { + sched::setns(fd, s).or_else(|e| { if s == CloneFlags::CLONE_NEWUSER { if e.as_errno().unwrap() != Errno::EINVAL { check!( write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()), "write_sync for CLONE_NEWUSER" ); - return Err(e.into()); + return Err(e); } + + Ok(()) } else { check!( write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()), "write_sync for sched::setns" ); - return Err(e.into()); + Err(e) } - } + })?; + unistd::close(fd)?; if s == CloneFlags::CLONE_NEWUSER { diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index 45d89bd520..a9c2ba1b08 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -479,15 +479,18 @@ fn mount_to_rootfs(logger: &Logger, m: &INIT_MOUNT) -> Result<()> { fs::create_dir_all(Path::new(m.dest)).context("could not create directory")?; - if let Err(err) = bare_mount.mount() { + bare_mount.mount().or_else(|e| { if m.src != "dev" { - return Err(err.into()); + return Err(e); } + error!( logger, "Could not mount filesystem from {} to {}", m.src, m.dest ); - } + + Ok(()) + })?; Ok(()) } diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index ed383b21ae..fcfa4adcfa 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -1610,11 +1610,13 @@ fn do_copy_file(req: &CopyFileRequest) -> Result<()> { PathBuf::from("/") }; - if let Err(e) = fs::create_dir_all(dir.to_str().unwrap()) { + fs::create_dir_all(dir.to_str().unwrap()).or_else(|e| { if e.kind() != std::io::ErrorKind::AlreadyExists { - return Err(e.into()); + return Err(e); } - } + + Ok(()) + })?; std::fs::set_permissions( dir.to_str().unwrap(), From a18899f1a357814a9a0451e97e3f5748b197e647 Mon Sep 17 00:00:00 2001 From: Tim Zhang Date: Tue, 13 Oct 2020 15:17:30 +0800 Subject: [PATCH 03/12] agent: refactor namespace::setup to optimize error handling - Replace the return value with anyhow::Result. - Remove if let Err. - Remove match. Signed-off-by: Tim Zhang --- src/agent/src/namespace.rs | 46 +++++++++++++------------------------- 1 file changed, 16 insertions(+), 30 deletions(-) diff --git a/src/agent/src/namespace.rs b/src/agent/src/namespace.rs index 6cb42aa768..f5c6fa3b0a 100644 --- a/src/agent/src/namespace.rs +++ b/src/agent/src/namespace.rs @@ -3,15 +3,15 @@ // SPDX-License-Identifier: Apache-2.0 // +use anyhow::{anyhow, Result}; use nix::mount::MsFlags; use nix::sched::{unshare, CloneFlags}; use nix::unistd::{getpid, gettid}; use std::fmt; use std::fs; use std::fs::File; -use std::os::unix::io::AsRawFd; use std::path::{Path, PathBuf}; -use std::thread; +use std::thread::{self}; use crate::mount::{BareMount, FLAGS}; use slog::Logger; @@ -77,10 +77,8 @@ impl Namespace { // setup creates persistent namespace without switching to it. // Note, pid namespaces cannot be persisted. - pub fn setup(mut self) -> Result { - if let Err(err) = fs::create_dir_all(&self.persistent_ns_dir) { - return Err(err.to_string()); - } + pub fn setup(mut self) -> Result { + fs::create_dir_all(&self.persistent_ns_dir)?; let ns_path = PathBuf::from(&self.persistent_ns_dir); let ns_type = self.ns_type.clone(); @@ -88,33 +86,23 @@ impl Namespace { let new_ns_path = ns_path.join(&ns_type.get()); - if let Err(err) = File::create(new_ns_path.as_path()) { - return Err(err.to_string()); - } + File::create(new_ns_path.as_path())?; self.path = new_ns_path.clone().into_os_string().into_string().unwrap(); let hostname = self.hostname.clone(); - let new_thread = thread::spawn(move || { + let new_thread = thread::spawn(move || -> Result<()> { let origin_ns_path = get_current_thread_ns_path(&ns_type.get()); - let _origin_ns_fd = match File::open(Path::new(&origin_ns_path)) { - Err(err) => return Err(err.to_string()), - Ok(file) => file.as_raw_fd(), - }; + File::open(Path::new(&origin_ns_path))?; // Create a new netns on the current thread. let cf = ns_type.get_flags().clone(); - if let Err(err) = unshare(cf) { - return Err(err.to_string()); - } + unshare(cf)?; if ns_type == NamespaceType::UTS && hostname.is_some() { - match nix::unistd::sethostname(hostname.unwrap()) { - Err(err) => return Err(err.to_string()), - Ok(_) => (), - } + nix::unistd::sethostname(hostname.unwrap())?; } // Bind mount the new namespace from the current thread onto the mount point to persist it. let source: &str = origin_ns_path.as_str(); @@ -132,22 +120,20 @@ impl Namespace { let bare_mount = BareMount::new(source, destination, "none", flags, "", &logger); bare_mount.mount().map_err(|e| { - format!( + anyhow!( "Failed to mount {} to {} with err:{:?}", - source, destination, e + source, + destination, + e ) })?; Ok(()) }); - match new_thread.join() { - Ok(t) => match t { - Err(err) => return Err(err), - Ok(()) => (), - }, - Err(err) => return Err(format!("Failed to join thread {:?}!", err)), - } + new_thread + .join() + .map_err(|e| anyhow!("Failed to join thread {:?}!", e))??; Ok(self) } From 09aca49ed71b1f258ff17be9081a9f7bb42a3b11 Mon Sep 17 00:00:00 2001 From: Tim Zhang Date: Tue, 13 Oct 2020 15:26:35 +0800 Subject: [PATCH 04/12] agent: remove `check!` in child process because we cant' see logs. The check macro will log the errors but the log in child process can't be seen, just ignore it. Signed-off-by: Tim Zhang --- src/agent/rustjail/src/container.rs | 33 ++++++++++------------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 4e80799a07..a6eac8bceb 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -335,10 +335,7 @@ pub fn init_child() { Ok(_) => (), Err(e) => { log_child!(cfd_log, "child exit: {:?}", e); - check!( - write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()), - "write_sync in init_child()" - ); + let _ = write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()); return; } } @@ -490,19 +487,13 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { sched::setns(fd, s).or_else(|e| { if s == CloneFlags::CLONE_NEWUSER { if e.as_errno().unwrap() != Errno::EINVAL { - check!( - write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()), - "write_sync for CLONE_NEWUSER" - ); + let _ = write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()); return Err(e); } Ok(()) } else { - check!( - write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()), - "write_sync for sched::setns" - ); + let _ = write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()); Err(e) } })?; @@ -578,14 +569,12 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { if guser.additional_gids.len() > 0 { setgroups(guser.additional_gids.as_slice()).map_err(|e| { - check!( - write_sync( - cwfd, - SYNC_FAILED, - format!("setgroups failed: {:?}", e).as_str() - ), - "write_sync for setgroups" + let _ = write_sync( + cwfd, + SYNC_FAILED, + format!("setgroups failed: {:?}", e).as_str(), ); + e })?; } @@ -650,9 +639,9 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { // notify parent that the child's ready to start write_sync(cwfd, SYNC_SUCCESS, "")?; log_child!(cfd_log, "ready to run exec"); - check!(unistd::close(cfd_log), "closing cfd log"); - check!(unistd::close(crfd), "closing crfd"); - check!(unistd::close(cwfd), "closing cwfd"); + let _ = unistd::close(cfd_log); + let _ = unistd::close(crfd); + let _ = unistd::close(cwfd); if oci_process.terminal { unistd::setsid()?; From 7f9e5913e02b8d5e4f751de689bcea9cf7f1a81d Mon Sep 17 00:00:00 2001 From: Tim Zhang Date: Tue, 13 Oct 2020 15:49:09 +0800 Subject: [PATCH 05/12] agent: replace check! with map_err for readability It's ambiguous and not easy to read to call method use macro. Signed-off-by: Tim Zhang --- src/agent/rustjail/src/container.rs | 76 +++++++++++------------------ 1 file changed, 28 insertions(+), 48 deletions(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index a6eac8bceb..d65d6b4279 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -67,17 +67,6 @@ const CLOG_FD: &str = "CLOG_FD"; const FIFO_FD: &str = "FIFO_FD"; const HOME_ENV_KEY: &str = "HOME"; -#[macro_export] -macro_rules! check { - ($what:expr, $where:expr) => ({ - if let Err(e) = $what { - let subsystem = $where; - let logger = slog_scope::logger().new(o!("subsystem" => subsystem)); - warn!(logger, "{:?}", e); - } - }) -} - #[derive(PartialEq, Clone, Copy)] pub enum Status { CREATED, @@ -778,10 +767,9 @@ impl BaseContainer for LinuxContainer { let st = self.oci_state()?; let (pfd_log, cfd_log) = unistd::pipe().context("failed to create pipe")?; - check!( - fcntl::fcntl(pfd_log, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)), - "fcntl pfd log FD_CLOEXEC" - ); + + let _ = fcntl::fcntl(pfd_log, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)) + .map_err(|e| warn!(logger, "fcntl pfd log FD_CLOEXEC {:?}", e)); let child_logger = logger.new(o!("action" => "child process log")); let log_handler = thread::spawn(move || { @@ -810,18 +798,16 @@ impl BaseContainer for LinuxContainer { info!(logger, "exec fifo opened!"); let (prfd, cwfd) = unistd::pipe().context("failed to create pipe")?; let (crfd, pwfd) = unistd::pipe().context("failed to create pipe")?; - check!( - fcntl::fcntl(prfd, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)), - "fcntl prfd FD_CLOEXEC" - ); - check!( - fcntl::fcntl(pwfd, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)), - "fcntl pwfd FD_COLEXEC" - ); + + let _ = fcntl::fcntl(prfd, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)) + .map_err(|e| warn!(logger, "fcntl prfd FD_CLOEXEC {:?}", e)); + + let _ = fcntl::fcntl(pwfd, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)) + .map_err(|e| warn!(logger, "fcntl pwfd FD_COLEXEC {:?}", e)); defer!({ - check!(unistd::close(prfd), "close prfd"); - check!(unistd::close(pwfd), "close pwfd"); + let _ = unistd::close(prfd).map_err(|e| warn!(logger, "close prfd {:?}", e)); + let _ = unistd::close(pwfd).map_err(|e| warn!(logger, "close pwfd {:?}", e)); }); let child_stdin: std::process::Stdio; @@ -831,14 +817,10 @@ impl BaseContainer for LinuxContainer { if tty { let pseudo = pty::openpty(None, None)?; p.term_master = Some(pseudo.master); - check!( - fcntl::fcntl(pseudo.master, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)), - "fnctl pseudo.master" - ); - check!( - fcntl::fcntl(pseudo.slave, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)), - "fcntl pseudo.slave" - ); + let _ = fcntl::fcntl(pseudo.master, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)) + .map_err(|e| warn!(logger, "fnctl pseudo.master {:?}", e)); + let _ = fcntl::fcntl(pseudo.slave, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)) + .map_err(|e| warn!(logger, "fcntl pseudo.slave {:?}", e)); child_stdin = unsafe { std::process::Stdio::from_raw_fd(pseudo.slave) }; child_stdout = unsafe { std::process::Stdio::from_raw_fd(pseudo.slave) }; @@ -865,11 +847,10 @@ impl BaseContainer for LinuxContainer { //restore the parent's process's pid namespace. defer!({ - check!( - sched::setns(old_pid_ns, CloneFlags::CLONE_NEWPID), - "settns CLONE_NEWPID" - ); - check!(unistd::close(old_pid_ns), "close old pid namespace"); + let _ = sched::setns(old_pid_ns, CloneFlags::CLONE_NEWPID) + .map_err(|e| warn!(logger, "settns CLONE_NEWPID {:?}", e)); + let _ = unistd::close(old_pid_ns) + .map_err(|e| warn!(logger, "close old pid namespace {:?}", e)); }); let pidns = get_pid_namespace(&self.logger, linux)?; @@ -911,7 +892,7 @@ impl BaseContainer for LinuxContainer { } if p.init { - check!(unistd::close(fifofd), "close fifofd"); + let _ = unistd::close(fifofd).map_err(|e| warn!(logger, "close fifofd {:?}", e)); } info!(logger, "child pid: {}", p.pid); @@ -929,10 +910,8 @@ impl BaseContainer for LinuxContainer { Err(e) => { error!(logger, "create container process error {:?}", e); // kill the child process. - check!( - signal::kill(Pid::from_raw(p.pid), Some(Signal::SIGKILL)), - "signal::kill joining namespaces" - ); + let _ = signal::kill(Pid::from_raw(p.pid), Some(Signal::SIGKILL)) + .map_err(|e| warn!(logger, "signal::kill joining namespaces {:?}", e)); return Err(e); } }; @@ -945,10 +924,9 @@ impl BaseContainer for LinuxContainer { let (exit_pipe_r, exit_pipe_w) = unistd::pipe2(OFlag::O_CLOEXEC) .context("failed to create pipe") .map_err(|e| { - check!( - signal::kill(Pid::from_raw(child.id() as i32), Some(Signal::SIGKILL)), - "signal::kill creating pipe" - ); + let _ = signal::kill(Pid::from_raw(child.id() as i32), Some(Signal::SIGKILL)) + .map_err(|e| warn!(logger, "signal::kill creating pipe {:?}", e)); + e })?; @@ -962,7 +940,9 @@ impl BaseContainer for LinuxContainer { self.processes.insert(p.pid, p); info!(logger, "wait on child log handler"); - check!(log_handler.join(), "joining log handler"); + let _ = log_handler + .join() + .map_err(|e| warn!(logger, "joining log handler {:?}", e)); info!(logger, "create process completed"); return Ok(()); } From 7bf4073d8d3bf8fd09577f0dfdb3bd19ead479ad Mon Sep 17 00:00:00 2001 From: Tim Zhang Date: Tue, 13 Oct 2020 17:54:10 +0800 Subject: [PATCH 06/12] agent: replace unnecessary `match Result` with `map_err` Replace `match Result` whose Ok hand is useless. Signed-off-by: Tim Zhang --- src/agent/rustjail/src/container.rs | 76 +++++++++------------ src/agent/rustjail/src/mount.rs | 100 ++++++++++++---------------- src/agent/src/device.rs | 19 +++--- src/agent/src/rpc.rs | 91 +++++++++---------------- src/agent/src/sandbox.rs | 7 +- 5 files changed, 117 insertions(+), 176 deletions(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index d65d6b4279..c96f79468b 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -320,14 +320,11 @@ impl Container for LinuxContainer { pub fn init_child() { let cwfd = std::env::var(CWFD_FD).unwrap().parse::().unwrap(); let cfd_log = std::env::var(CLOG_FD).unwrap().parse::().unwrap(); - match do_init_child(cwfd) { - Ok(_) => (), - Err(e) => { - log_child!(cfd_log, "child exit: {:?}", e); - let _ = write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()); - return; - } - } + + let _ = do_init_child(cwfd).map_err(|e| { + log_child!(cfd_log, "child exit: {:?}", e); + let _ = write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()); + }); } fn do_init_child(cwfd: RawFd) -> Result<()> { @@ -392,9 +389,8 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { to_new.set(*s, true); } } else { - let fd = match fcntl::open(ns.path.as_str(), OFlag::O_CLOEXEC, Mode::empty()) { - Ok(v) => v, - Err(e) => { + let fd = + fcntl::open(ns.path.as_str(), OFlag::O_CLOEXEC, Mode::empty()).map_err(|e| { log_child!( cfd_log, "cannot open type: {} path: {}", @@ -402,9 +398,8 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { ns.path.clone() ); log_child!(cfd_log, "error is : {}", e.as_errno().unwrap().desc()); - return Err(e.into()); - } - }; + e + })?; if *s != CloneFlags::CLONE_NEWPID { to_join.push((*s, fd)); @@ -834,16 +829,14 @@ impl BaseContainer for LinuxContainer { child_stderr = unsafe { std::process::Stdio::from_raw_fd(stderr) }; } - let old_pid_ns = match fcntl::open(PID_NS_PATH, OFlag::O_CLOEXEC, Mode::empty()) { - Ok(v) => v, - Err(e) => { + let old_pid_ns = + fcntl::open(PID_NS_PATH, OFlag::O_CLOEXEC, Mode::empty()).map_err(|e| { error!( logger, "cannot open pid ns path: {} with error: {:?}", PID_NS_PATH, e ); - return Err(e.into()); - } - }; + e + })?; //restore the parent's process's pid namespace. defer!({ @@ -897,7 +890,7 @@ impl BaseContainer for LinuxContainer { info!(logger, "child pid: {}", p.pid); - match join_namespaces( + join_namespaces( &logger, &spec, &p, @@ -905,16 +898,15 @@ impl BaseContainer for LinuxContainer { &st, pwfd, prfd, - ) { - Ok(_) => (), - Err(e) => { - error!(logger, "create container process error {:?}", e); - // kill the child process. - let _ = signal::kill(Pid::from_raw(p.pid), Some(Signal::SIGKILL)) - .map_err(|e| warn!(logger, "signal::kill joining namespaces {:?}", e)); - return Err(e); - } - }; + ) + .map_err(|e| { + error!(logger, "create container process error {:?}", e); + // kill the child process. + let _ = signal::kill(Pid::from_raw(p.pid), Some(Signal::SIGKILL)) + .map_err(|e| warn!(logger, "signal::kill joining namespaces {:?}", e)); + + e + })?; info!(logger, "entered namespaces!"); @@ -1085,9 +1077,8 @@ fn get_pid_namespace(logger: &Logger, linux: &Linux) -> Result> { return Ok(None); } - let fd = match fcntl::open(ns.path.as_str(), OFlag::O_CLOEXEC, Mode::empty()) { - Ok(v) => v, - Err(e) => { + let fd = + fcntl::open(ns.path.as_str(), OFlag::O_CLOEXEC, Mode::empty()).map_err(|e| { error!( logger, "cannot open type: {} path: {}", @@ -1095,9 +1086,9 @@ fn get_pid_namespace(logger: &Logger, linux: &Linux) -> Result> { ns.path.clone() ); error!(logger, "error is : {}", e.as_errno().unwrap().desc()); - return Err(e.into()); - } - }; + + e + })?; return Ok(Some(fd)); } @@ -1245,13 +1236,10 @@ fn write_mappings(logger: &Logger, path: &str, maps: &[LinuxIDMapping]) -> Resul if !data.is_empty() { let fd = fcntl::open(path, OFlag::O_WRONLY, Mode::empty())?; defer!(unistd::close(fd).unwrap()); - match unistd::write(fd, data.as_bytes()) { - Ok(_) => {} - Err(e) => { - info!(logger, "cannot write mapping"); - return Err(e.into()); - } - } + unistd::write(fd, data.as_bytes()).map_err(|e| { + info!(logger, "cannot write mapping"); + e + })?; } Ok(()) } diff --git a/src/agent/rustjail/src/mount.rs b/src/agent/rustjail/src/mount.rs index f759456e7a..daa19ce0b0 100644 --- a/src/agent/rustjail/src/mount.rs +++ b/src/agent/rustjail/src/mount.rs @@ -407,20 +407,17 @@ fn mount_cgroups( if key != base { let src = format!("{}/{}", m.destination.as_str(), key); - match unix::fs::symlink(destination.as_str(), &src[1..]) { - Err(e) => { - log_child!( - cfd_log, - "symlink: {} {} err: {}", - key, - destination.as_str(), - e.to_string() - ); + unix::fs::symlink(destination.as_str(), &src[1..]).map_err(|e| { + log_child!( + cfd_log, + "symlink: {} {} err: {}", + key, + destination.as_str(), + e.to_string() + ); - return Err(e.into()); - } - Ok(_) => {} - } + e + })?; } } @@ -689,18 +686,14 @@ fn mount_from( Path::new(&dest) }; - // let _ = fs::create_dir_all(&dir); - match fs::create_dir_all(&dir) { - Ok(_) => {} - Err(e) => { - log_child!( - cfd_log, - "creat dir {}: {}", - dir.to_str().unwrap(), - e.to_string() - ); - } - } + let _ = fs::create_dir_all(&dir).map_err(|e| { + log_child!( + cfd_log, + "creat dir {}: {}", + dir.to_str().unwrap(), + e.to_string() + ) + }); // make sure file exists so we can bind over it if src.is_file() { @@ -717,31 +710,26 @@ fn mount_from( } }; - match stat::stat(dest.as_str()) { - Ok(_) => {} - Err(e) => { - log_child!( - cfd_log, - "dest stat error. {}: {}", - dest.as_str(), - e.as_errno().unwrap().desc() - ); - } - } + let _ = stat::stat(dest.as_str()).map_err(|e| { + log_child!( + cfd_log, + "dest stat error. {}: {}", + dest.as_str(), + e.as_errno().unwrap().desc() + ) + }); - match mount( + mount( Some(src.as_str()), dest.as_str(), Some(m.r#type.as_str()), flags, Some(d.as_str()), - ) { - Ok(_) => {} - Err(e) => { - log_child!(cfd_log, "mount error: {}", e.as_errno().unwrap().desc()); - return Err(e.into()); - } - } + ) + .map_err(|e| { + log_child!(cfd_log, "mount error: {}", e.as_errno().unwrap().desc()); + e + })?; if flags.contains(MsFlags::MS_BIND) && flags.intersects( @@ -753,24 +741,22 @@ fn mount_from( | MsFlags::MS_SLAVE), ) { - match mount( + mount( Some(dest.as_str()), dest.as_str(), None::<&str>, flags | MsFlags::MS_REMOUNT, None::<&str>, - ) { - Err(e) => { - log_child!( - cfd_log, - "remout {}: {}", - dest.as_str(), - e.as_errno().unwrap().desc() - ); - return Err(e.into()); - } - Ok(_) => {} - } + ) + .map_err(|e| { + log_child!( + cfd_log, + "remout {}: {}", + dest.as_str(), + e.as_errno().unwrap().desc() + ); + e + })?; } Ok(()) } diff --git a/src/agent/src/device.rs b/src/agent/src/device.rs index 5dbb166153..121d3a6368 100644 --- a/src/agent/src/device.rs +++ b/src/agent/src/device.rs @@ -130,17 +130,14 @@ fn get_device_name(sandbox: &Arc>, dev_addr: &str) -> Result name, - Err(_) => { - GLOBAL_DEVICE_WATCHER.lock().unwrap().remove_entry(dev_addr); - return Err(anyhow!( - "Timeout reached after {:?} waiting for device {}", - hotplug_timeout, - dev_addr - )); - } - }; + let dev_name = rx.recv_timeout(hotplug_timeout).map_err(|_| { + GLOBAL_DEVICE_WATCHER.lock().unwrap().remove_entry(dev_addr); + anyhow!( + "Timeout reached after {:?} waiting for device {}", + hotplug_timeout, + dev_addr + ) + })?; Ok(format!("{}/{}", SYSTEM_DEV_PATH, &dev_name)) } diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index fcfa4adcfa..7719608994 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -588,13 +588,9 @@ impl protocols::agent_ttrpc::AgentService for agentService { _ctx: &ttrpc::TtrpcContext, req: protocols::agent::WaitProcessRequest, ) -> ttrpc::Result { - match self.do_wait_process(req) { - Err(e) => Err(ttrpc::Error::RpcStatus(ttrpc::get_status( - ttrpc::Code::INTERNAL, - e.to_string(), - ))), - Ok(resp) => Ok(resp), - } + self.do_wait_process(req).map_err(|e| { + ttrpc::Error::RpcStatus(ttrpc::get_status(ttrpc::Code::INTERNAL, e.to_string())) + }) } fn list_processes( @@ -734,13 +730,9 @@ impl protocols::agent_ttrpc::AgentService for agentService { "invalid container id".to_string(), )))?; - match ctr.stats() { - Err(e) => Err(ttrpc::Error::RpcStatus(ttrpc::get_status( - ttrpc::Code::INTERNAL, - e.to_string(), - ))), - Ok(resp) => Ok(resp), - } + ctr.stats().map_err(|e| { + ttrpc::Error::RpcStatus(ttrpc::get_status(ttrpc::Code::INTERNAL, e.to_string())) + }) } fn pause_container( @@ -794,13 +786,9 @@ impl protocols::agent_ttrpc::AgentService for agentService { _ctx: &ttrpc::TtrpcContext, req: protocols::agent::WriteStreamRequest, ) -> ttrpc::Result { - match self.do_write_stream(req) { - Err(e) => Err(ttrpc::Error::RpcStatus(ttrpc::get_status( - ttrpc::Code::INTERNAL, - e.to_string(), - ))), - Ok(resp) => Ok(resp), - } + self.do_write_stream(req).map_err(|e| { + ttrpc::Error::RpcStatus(ttrpc::get_status(ttrpc::Code::INTERNAL, e.to_string())) + }) } fn read_stdout( @@ -808,13 +796,9 @@ impl protocols::agent_ttrpc::AgentService for agentService { _ctx: &ttrpc::TtrpcContext, req: protocols::agent::ReadStreamRequest, ) -> ttrpc::Result { - match self.do_read_stream(req, true) { - Err(e) => Err(ttrpc::Error::RpcStatus(ttrpc::get_status( - ttrpc::Code::INTERNAL, - e.to_string(), - ))), - Ok(resp) => Ok(resp), - } + self.do_read_stream(req, true).map_err(|e| { + ttrpc::Error::RpcStatus(ttrpc::get_status(ttrpc::Code::INTERNAL, e.to_string())) + }) } fn read_stderr( @@ -822,13 +806,9 @@ impl protocols::agent_ttrpc::AgentService for agentService { _ctx: &ttrpc::TtrpcContext, req: protocols::agent::ReadStreamRequest, ) -> ttrpc::Result { - match self.do_read_stream(req, false) { - Err(e) => Err(ttrpc::Error::RpcStatus(ttrpc::get_status( - ttrpc::Code::INTERNAL, - e.to_string(), - ))), - Ok(resp) => Ok(resp), - } + self.do_read_stream(req, false).map_err(|e| { + ttrpc::Error::RpcStatus(ttrpc::get_status(ttrpc::Code::INTERNAL, e.to_string())) + }) } fn close_stdin( @@ -841,15 +821,12 @@ impl protocols::agent_ttrpc::AgentService for agentService { let s = Arc::clone(&self.sandbox); let mut sandbox = s.lock().unwrap(); - let p = match find_process(&mut sandbox, cid.as_str(), eid.as_str(), false) { - Ok(v) => v, - Err(e) => { - return Err(ttrpc::Error::RpcStatus(ttrpc::get_status( - ttrpc::Code::INVALID_ARGUMENT, - format!("invalid argument: {:?}", e), - ))); - } - }; + let p = find_process(&mut sandbox, cid.as_str(), eid.as_str(), false).map_err(|e| { + ttrpc::Error::RpcStatus(ttrpc::get_status( + ttrpc::Code::INVALID_ARGUMENT, + format!("invalid argument: {:?}", e), + )) + })?; if p.term_master.is_some() { let _ = unistd::close(p.term_master.unwrap()); @@ -873,15 +850,12 @@ impl protocols::agent_ttrpc::AgentService for agentService { let eid = req.exec_id.clone(); let s = Arc::clone(&self.sandbox); let mut sandbox = s.lock().unwrap(); - let p = match find_process(&mut sandbox, cid.as_str(), eid.as_str(), false) { - Ok(v) => v, - Err(e) => { - return Err(ttrpc::Error::RpcStatus(ttrpc::get_status( - ttrpc::Code::UNAVAILABLE, - format!("invalid argument: {:?}", e), - ))); - } - }; + let p = find_process(&mut sandbox, cid.as_str(), eid.as_str(), false).map_err(|e| { + ttrpc::Error::RpcStatus(ttrpc::get_status( + ttrpc::Code::UNAVAILABLE, + format!("invalid argument: {:?}", e), + )) + })?; if p.term_master.is_none() { return Err(ttrpc::Error::RpcStatus(ttrpc::get_status( @@ -1335,13 +1309,10 @@ fn get_memory_info(block_size: bool, hotplug: bool) -> Result<(u64, bool)> { return Err(anyhow!("Invalid block size")); } - size = match u64::from_str_radix(v.trim(), 16) { - Ok(h) => h, - Err(_) => { - warn!(sl!(), "failed to parse the str {} to hex", size); - return Err(anyhow!("Invalid block size")); - } - }; + size = u64::from_str_radix(v.trim(), 16).map_err(|_| { + warn!(sl!(), "failed to parse the str {} to hex", size); + anyhow!("Invalid block size") + })?; } Err(e) => { info!(sl!(), "memory block size error: {:?}", e.kind()); diff --git a/src/agent/src/sandbox.rs b/src/agent/src/sandbox.rs index f871d3ef72..f2d92e9728 100644 --- a/src/agent/src/sandbox.rs +++ b/src/agent/src/sandbox.rs @@ -316,10 +316,9 @@ impl Sandbox { thread::spawn(move || { for event in rx { info!(logger, "got an OOM event {:?}", event); - match tx.send(container_id.clone()) { - Err(err) => error!(logger, "failed to send message: {:?}", err), - Ok(_) => {} - } + let _ = tx + .send(container_id.clone()) + .map_err(|e| error!(logger, "failed to send message: {:?}", e)); } }); } From 0dce817ebb1ddd701d505ac902c6ae2b20bd2900 Mon Sep 17 00:00:00 2001 From: Tim Zhang Date: Tue, 13 Oct 2020 19:01:41 +0800 Subject: [PATCH 07/12] agent: replace `match Result` with `or_else` `or_else` is suitable for more complicated situations. We can use it to return Ok in Err handling. Signed-off-by: Tim Zhang --- src/agent/rustjail/src/mount.rs | 33 ++++++++++++++++----------------- src/agent/rustjail/src/sync.rs | 22 ++++++++-------------- 2 files changed, 24 insertions(+), 31 deletions(-) diff --git a/src/agent/rustjail/src/mount.rs b/src/agent/rustjail/src/mount.rs index daa19ce0b0..cc3b928a4a 100644 --- a/src/agent/rustjail/src/mount.rs +++ b/src/agent/rustjail/src/mount.rs @@ -612,24 +612,23 @@ pub fn ms_move_root(rootfs: &str) -> Result { MsFlags::MS_SLAVE | MsFlags::MS_REC, None::<&str>, )?; - match umount2(abs_mount_point, MntFlags::MNT_DETACH) { - Ok(_) => (), - Err(e) => { - if e.ne(&nix::Error::from(Errno::EINVAL)) && e.ne(&nix::Error::from(Errno::EPERM)) { - return Err(anyhow!(e)); - } - - // If we have not privileges for umounting (e.g. rootless), then - // cover the path. - mount( - Some("tmpfs"), - abs_mount_point, - Some("tmpfs"), - MsFlags::empty(), - None::<&str>, - )?; + umount2(abs_mount_point, MntFlags::MNT_DETACH).or_else(|e| { + if e.ne(&nix::Error::from(Errno::EINVAL)) && e.ne(&nix::Error::from(Errno::EPERM)) { + return Err(anyhow!(e)); } - } + + // If we have not privileges for umounting (e.g. rootless), then + // cover the path. + mount( + Some("tmpfs"), + abs_mount_point, + Some("tmpfs"), + MsFlags::empty(), + None::<&str>, + )?; + + Ok(()) + })?; } mount( diff --git a/src/agent/rustjail/src/sync.rs b/src/agent/rustjail/src/sync.rs index 8ce43b270f..9e98b0ad76 100644 --- a/src/agent/rustjail/src/sync.rs +++ b/src/agent/rustjail/src/sync.rs @@ -143,21 +143,15 @@ pub fn write_sync(fd: RawFd, msg_type: i32, data_str: &str) -> Result<()> { }, SYNC_DATA => { let length: i32 = data_str.len() as i32; - match write_count(fd, &length.to_be_bytes(), MSG_SIZE) { - Ok(_count) => (), - Err(e) => { - unistd::close(fd)?; - return Err(anyhow!(e).context("error in send message to process")); - } - } + write_count(fd, &length.to_be_bytes(), MSG_SIZE).or_else(|e| { + unistd::close(fd)?; + Err(anyhow!(e).context("error in send message to process")) + })?; - match write_count(fd, data_str.as_bytes(), data_str.len()) { - Ok(_count) => (), - Err(e) => { - unistd::close(fd)?; - return Err(anyhow!(e).context("error in send message to process")); - } - } + write_count(fd, data_str.as_bytes(), data_str.len()).or_else(|e| { + unistd::close(fd)?; + Err(anyhow!(e).context("error in send message to process")) + })?; } _ => (), From 1d8def6663ca0cfe7ff3f1084aeaa6942a41990b Mon Sep 17 00:00:00 2001 From: Tim Zhang Date: Tue, 13 Oct 2020 19:14:43 +0800 Subject: [PATCH 08/12] agent: Use `ok_or_else` instead of match for Option -> Result Using ok_or is clearer than match. Signed-off-by: Tim Zhang --- src/agent/rustjail/src/container.rs | 15 +++++---------- src/agent/src/device.rs | 16 ++++++++-------- src/agent/src/mount.rs | 20 +++++++++----------- 3 files changed, 22 insertions(+), 29 deletions(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index c96f79468b..9add8d152e 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -612,12 +612,7 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { let exec_file = Path::new(&args[0]); log_child!(cfd_log, "process command: {:?}", &args); if !exec_file.exists() { - match find_file(exec_file) { - Some(_) => (), - None => { - return Err(anyhow!("the file {} is not exist", &args[0])); - } - } + find_file(exec_file).ok_or_else(|| anyhow!("the file {} is not exist", &args[0]))?; } // notify parent that the child's ready to start @@ -1047,10 +1042,10 @@ fn do_exec(args: &[String]) -> ! { fn update_namespaces(logger: &Logger, spec: &mut Spec, init_pid: RawFd) -> Result<()> { info!(logger, "updating namespaces"); - let linux = match spec.linux.as_mut() { - None => return Err(anyhow!("Spec didn't contain linux field")), - Some(l) => l, - }; + let linux = spec + .linux + .as_mut() + .ok_or_else(|| anyhow!("Spec didn't contain linux field"))?; let namespaces = linux.namespaces.as_mut_slice(); for namespace in namespaces.iter_mut() { diff --git a/src/agent/src/device.rs b/src/agent/src/device.rs index 121d3a6368..d5905e1f6c 100644 --- a/src/agent/src/device.rs +++ b/src/agent/src/device.rs @@ -204,10 +204,10 @@ fn update_spec_device_list(device: &Device, spec: &mut Spec) -> Result<()> { )); } - let linux = match spec.linux.as_mut() { - None => return Err(anyhow!("Spec didn't container linux field")), - Some(l) => l, - }; + let linux = spec + .linux + .as_mut() + .ok_or_else(|| anyhow!("Spec didn't container linux field"))?; if !Path::new(&device.vm_path).exists() { return Err(anyhow!("vm_path:{} doesn't exist", device.vm_path)); @@ -365,10 +365,10 @@ pub fn update_device_cgroup(spec: &mut Spec) -> Result<()> { let major = stat::major(rdev) as i64; let minor = stat::minor(rdev) as i64; - let linux = match spec.linux.as_mut() { - None => return Err(anyhow!("Spec didn't container linux field")), - Some(l) => l, - }; + let linux = spec + .linux + .as_mut() + .ok_or_else(|| anyhow!("Spec didn't container linux field"))?; if linux.resources.is_none() { linux.resources = Some(LinuxResources::default()); diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index a9c2ba1b08..a44956c04d 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -446,15 +446,14 @@ pub fn add_storages( "subsystem" => "storage", "storage-type" => handler_name.to_owned())); - let handler = match STORAGEHANDLERLIST.get(&handler_name.as_str()) { - None => { - return Err(anyhow!( + let handler = STORAGEHANDLERLIST + .get(&handler_name.as_str()) + .ok_or_else(|| { + anyhow!( "Failed to find the storage handler {}", storage.driver.to_owned() - )); - } - Some(f) => f, - }; + ) + })?; let mount_point = match handler(&logger, &storage, sandbox.clone()) { // Todo need to rollback the mounted storage if err met. @@ -659,10 +658,9 @@ pub fn remove_mounts(mounts: &Vec) -> Result<()> { fn ensure_destination_exists(destination: &str, fs_type: &str) -> Result<()> { let d = Path::new(destination); if !d.exists() { - let dir = match d.parent() { - Some(d) => d, - None => return Err(anyhow!("mount destination {} doesn't exist", destination)), - }; + let dir = d + .parent() + .ok_or_else(|| anyhow!("mount destination {} doesn't exist", destination))?; if !dir.exists() { fs::create_dir_all(dir).context(format!("create dir all failed on {:?}", dir))?; } From 2f690a2bb05094d2779d7a493b38e2aa02c0aac1 Mon Sep 17 00:00:00 2001 From: Tim Zhang Date: Tue, 13 Oct 2020 19:19:51 +0800 Subject: [PATCH 09/12] agent: remove useless match Remove useless match. Signed-off-by: Tim Zhang --- src/agent/src/mount.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index a44956c04d..c31afab478 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -455,11 +455,8 @@ pub fn add_storages( ) })?; - let mount_point = match handler(&logger, &storage, sandbox.clone()) { - // Todo need to rollback the mounted storage if err met. - Err(e) => return Err(e), - Ok(m) => m, - }; + // Todo need to rollback the mounted storage if err met. + let mount_point = handler(&logger, &storage, sandbox.clone())?; if mount_point.len() > 0 { mount_list.push(mount_point); From 47ff2fb9a058aea6792a80fbe8daa6ce53b8399a Mon Sep 17 00:00:00 2001 From: Tim Zhang Date: Tue, 13 Oct 2020 19:24:22 +0800 Subject: [PATCH 10/12] agent: use anyhow `context` to attach context to `Error` instead of `match` Context is clearer than match for these situations. Signed-off-by: Tim Zhang --- src/agent/src/sandbox.rs | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/src/agent/src/sandbox.rs b/src/agent/src/sandbox.rs index f2d92e9728..ac80f5be0f 100644 --- a/src/agent/src/sandbox.rs +++ b/src/agent/src/sandbox.rs @@ -166,23 +166,17 @@ impl Sandbox { pub fn setup_shared_namespaces(&mut self) -> Result { // Set up shared IPC namespace - self.shared_ipcns = match Namespace::new(&self.logger).as_ipc().setup() { - Ok(ns) => ns, - Err(err) => { - return Err(anyhow!(err).context("Failed to setup persistent IPC namespace")); - } - }; + self.shared_ipcns = Namespace::new(&self.logger) + .as_ipc() + .setup() + .context("Failed to setup persistent IPC namespace")?; // // Set up shared UTS namespace - self.shared_utsns = match Namespace::new(&self.logger) + self.shared_utsns = Namespace::new(&self.logger) .as_uts(self.hostname.as_str()) .setup() - { - Ok(ns) => ns, - Err(err) => { - return Err(anyhow!(err).context("Failed to setup persistent UTS namespace")); - } - }; + .context("Failed to setup persistent UTS namespace")?; + Ok(true) } From e77482fe16198c42a69ba7d81540c03adeefd9ee Mon Sep 17 00:00:00 2001 From: Tim Zhang Date: Tue, 13 Oct 2020 19:28:34 +0800 Subject: [PATCH 11/12] agent: Use `?` instead of `match` when the error returns directly It's more clear and more readable. Signed-off-by: Tim Zhang --- src/agent/src/sandbox.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/agent/src/sandbox.rs b/src/agent/src/sandbox.rs index ac80f5be0f..ee5ab08f98 100644 --- a/src/agent/src/sandbox.rs +++ b/src/agent/src/sandbox.rs @@ -143,16 +143,10 @@ impl Sandbox { // It's assumed that caller is calling this method after // acquiring a lock on sandbox. pub fn unset_and_remove_sandbox_storage(&mut self, path: &str) -> Result<()> { - match self.unset_sandbox_storage(path) { - Ok(res) => { - if res { - return self.remove_sandbox_storage(path); - } - } - Err(err) => { - return Err(err); - } + if self.unset_sandbox_storage(path)? { + return self.remove_sandbox_storage(path); } + Ok(()) } From 6ba294a11e48e8cf2d51ab9d7eb5595aa5e6a46e Mon Sep 17 00:00:00 2001 From: Tim Zhang Date: Wed, 14 Oct 2020 10:18:00 +0800 Subject: [PATCH 12/12] agent: remove `unwrap()` for `e.as_errno()` Use `{:?}` to print `e.as_errno()` instead of using `{}` to print `e.as_errno().unwrap().desc()`. Avoid panic only caused by error's content. Signed-off-by: Tim Zhang --- src/agent/rustjail/src/container.rs | 4 ++-- src/agent/rustjail/src/mount.rs | 13 ++++--------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 9add8d152e..7c166fd24c 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -397,7 +397,7 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { ns.r#type.clone(), ns.path.clone() ); - log_child!(cfd_log, "error is : {}", e.as_errno().unwrap().desc()); + log_child!(cfd_log, "error is : {:?}", e.as_errno()); e })?; @@ -1080,7 +1080,7 @@ fn get_pid_namespace(logger: &Logger, linux: &Linux) -> Result> { ns.r#type.clone(), ns.path.clone() ); - error!(logger, "error is : {}", e.as_errno().unwrap().desc()); + error!(logger, "error is : {:?}", e.as_errno()); e })?; diff --git a/src/agent/rustjail/src/mount.rs b/src/agent/rustjail/src/mount.rs index cc3b928a4a..1942fcc5b8 100644 --- a/src/agent/rustjail/src/mount.rs +++ b/src/agent/rustjail/src/mount.rs @@ -712,9 +712,9 @@ fn mount_from( let _ = stat::stat(dest.as_str()).map_err(|e| { log_child!( cfd_log, - "dest stat error. {}: {}", + "dest stat error. {}: {:?}", dest.as_str(), - e.as_errno().unwrap().desc() + e.as_errno() ) }); @@ -726,7 +726,7 @@ fn mount_from( Some(d.as_str()), ) .map_err(|e| { - log_child!(cfd_log, "mount error: {}", e.as_errno().unwrap().desc()); + log_child!(cfd_log, "mount error: {:?}", e.as_errno()); e })?; @@ -748,12 +748,7 @@ fn mount_from( None::<&str>, ) .map_err(|e| { - log_child!( - cfd_log, - "remout {}: {}", - dest.as_str(), - e.as_errno().unwrap().desc() - ); + log_child!(cfd_log, "remout {}: {:?}", dest.as_str(), e.as_errno()); e })?; }