From 3682812e5746f8b69b64066bbb5933ee1ba5e668 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 17 Sep 2020 18:37:56 +0200 Subject: [PATCH 01/14] rust-agent: Remove unused macros This addresses the following warnings: Compiling rustjail v0.1.0 (/home/ddd/go/src/github.com/kata-containers-2.0/src/agent/rustjail) warning: unused `#[macro_use]` import --> rustjail/src/lib.rs:15:1 | 15 | #[macro_use] | ^^^^^^^^^^^^ | = note: `#[warn(unused_imports)]` on by default warning: unused macro definition --> rustjail/src/lib.rs:38:1 | 38 | / macro_rules! sl { 39 | | () => { 40 | | slog_scope::logger().new(o!("subsystem" => "rustjail")) 41 | | }; 42 | | } | |_^ | = note: `#[warn(unused_macros)]` on by default Fixes: #750 Signed-off-by: Christophe de Dinechin --- src/agent/rustjail/src/lib.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/agent/rustjail/src/lib.rs b/src/agent/rustjail/src/lib.rs index e71b7c643f..3fe93a8c9d 100644 --- a/src/agent/rustjail/src/lib.rs +++ b/src/agent/rustjail/src/lib.rs @@ -15,7 +15,6 @@ #[macro_use] #[cfg(test)] extern crate serial_test; -#[macro_use] extern crate serde; extern crate serde_json; #[macro_use] @@ -37,13 +36,6 @@ extern crate oci; extern crate path_absolutize; extern crate regex; -// Convenience macro to obtain the scope logger -macro_rules! sl { - () => { - slog_scope::logger().new(o!("subsystem" => "rustjail")) - }; -} - pub mod capabilities; pub mod cgroups; pub mod container; From d76ece0cf36dbf53dddc9d85e449978704c54b6a Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 24 Sep 2020 13:04:29 +0200 Subject: [PATCH 02/14] rust-agent: Remove useless braces This addresses the following warning: warning: unnecessary braces around assigned value --> src/rpc.rs:1411:26 | 1411 | detail.init_daemon = { unistd::getpid() == Pid::from_raw(1) }; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove these braces | = note: `#[warn(unused_braces)]` on by default Fixes: #750 Signed-off-by: Christophe de Dinechin --- src/agent/src/rpc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index c1351e6360..2b3ed22077 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -1447,7 +1447,7 @@ fn get_agent_details() -> AgentDetails { detail.set_version(AGENT_VERSION.to_string()); detail.set_supports_seccomp(false); - detail.init_daemon = { unistd::getpid() == Pid::from_raw(1) }; + detail.init_daemon = unistd::getpid() == Pid::from_raw(1); detail.device_handlers = RepeatedField::new(); detail.storage_handlers = RepeatedField::from_vec( From 27efe291c068ca8d25d9878dfb4d76621f8eb6be Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 17 Sep 2020 18:29:49 +0200 Subject: [PATCH 03/14] rust-agent: Remove unused functions Fixes the following warning: Compiling logging v0.1.0 (/home/ddd/go/src/github.com/kata-containers-2.0/pkg/logging) warning: associated function is never used: `set_level` --> /home/ddd/go/src/github.com/kata-containers-2.0/pkg/logging/src/lib.rs:186:8 | 186 | fn set_level(&self, level: slog::Level) { | ^^^^^^^^^ | = note: `#[warn(dead_code)]` on by default Fixes: #750 Signed-off-by: Christophe de Dinechin --- pkg/logging/src/lib.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/pkg/logging/src/lib.rs b/pkg/logging/src/lib.rs index d22fd59e05..6708529643 100644 --- a/pkg/logging/src/lib.rs +++ b/pkg/logging/src/lib.rs @@ -182,12 +182,6 @@ impl RuntimeLevelFilter { level: Mutex::new(level), } } - - fn set_level(&self, level: slog::Level) { - let mut log_level = self.level.lock().unwrap(); - - *log_level = level; - } } impl Drain for RuntimeLevelFilter From 5a1d3311359190a34c4bb2e82b48bd76a0133385 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 24 Sep 2020 13:08:20 +0200 Subject: [PATCH 04/14] rust-agent: Remove or rename unused variables Remove variables that are simply not used. Rename as _ variables where only initialization matters. This addresses the following warnings: warning: unused variable: `writer` --> src/main.rs:130:9 | 130 | let writer = unsafe { File::from_raw_fd(wfd) }; | ^^^^^^ help: if this is intentional, prefix it with an underscore: `_writer` | = note: `#[warn(unused_variables)]` on by default warning: unused variable: `ctx` --> src/rpc.rs:782:9 | 782 | ctx: &ttrpc::TtrpcContext, | ^^^ help: if this is intentional, prefix it with an underscore: `_ctx` warning: unused variable: `ctx` --> src/rpc.rs:808:9 | 808 | ctx: &ttrpc::TtrpcContext, | ^^^ help: if this is intentional, prefix it with an underscore: `_ctx` warning: unused variable: `dns_list` --> src/rpc.rs:1152:16 | 1152 | Ok(dns_list) => { | ^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_dns_list` warning: value assigned to `child_stdin` is never read --> rustjail/src/container.rs:807:13 | 807 | let mut child_stdin = std::process::Stdio::null(); | ^^^^^^^^^^^^^^^ | = note: `#[warn(unused_assignments)]` on by default = help: maybe it is overwritten before being read? warning: value assigned to `child_stdout` is never read --> rustjail/src/container.rs:808:13 | 808 | let mut child_stdout = std::process::Stdio::null(); | ^^^^^^^^^^^^^^^^ | = help: maybe it is overwritten before being read? warning: value assigned to `child_stderr` is never read --> rustjail/src/container.rs:809:13 | 809 | let mut child_stderr = std::process::Stdio::null(); | ^^^^^^^^^^^^^^^^ | = help: maybe it is overwritten before being read? warning: value assigned to `stdin` is never read --> rustjail/src/container.rs:810:13 | 810 | let mut stdin = -1; | ^^^^^^^^^ | = help: maybe it is overwritten before being read? warning: value assigned to `stdout` is never read --> rustjail/src/container.rs:811:13 | 811 | let mut stdout = -1; | ^^^^^^^^^^ | = help: maybe it is overwritten before being read? warning: value assigned to `stderr` is never read --> rustjail/src/container.rs:812:13 | 812 | let mut stderr = -1; | ^^^^^^^^^^ | = help: maybe it is overwritten before being read? Fixes: #750 Signed-off-by: Christophe de Dinechin --- src/agent/rustjail/src/container.rs | 29 +++++++++++++---------------- src/agent/rustjail/src/mount.rs | 16 ++++++++++------ src/agent/rustjail/src/process.rs | 4 ++-- src/agent/src/main.rs | 1 - src/agent/src/mount.rs | 2 +- src/agent/src/rpc.rs | 6 +++--- 6 files changed, 29 insertions(+), 29 deletions(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 35ddb71e39..86745207cd 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -799,26 +799,23 @@ impl BaseContainer for LinuxContainer { unistd::close(pwfd); }); - let mut child_stdin = std::process::Stdio::null(); - let mut child_stdout = std::process::Stdio::null(); - let mut child_stderr = std::process::Stdio::null(); - let mut stdin = -1; - let mut stdout = -1; - let mut stderr = -1; + let mut child_stdin: std::process::Stdio; + let mut child_stdout: std::process::Stdio; + let mut child_stderr: std::process::Stdio; if tty { - let pseduo = pty::openpty(None, None)?; - p.term_master = Some(pseduo.master); - fcntl::fcntl(pseduo.master, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)); - fcntl::fcntl(pseduo.slave, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)); + let pseudo = pty::openpty(None, None)?; + p.term_master = Some(pseudo.master); + fcntl::fcntl(pseudo.master, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)); + fcntl::fcntl(pseudo.slave, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)); - child_stdin = unsafe { std::process::Stdio::from_raw_fd(pseduo.slave) }; - child_stdout = unsafe { std::process::Stdio::from_raw_fd(pseduo.slave) }; - child_stderr = unsafe { std::process::Stdio::from_raw_fd(pseduo.slave) }; + child_stdin = unsafe { std::process::Stdio::from_raw_fd(pseudo.slave) }; + child_stdout = unsafe { std::process::Stdio::from_raw_fd(pseudo.slave) }; + child_stderr = unsafe { std::process::Stdio::from_raw_fd(pseudo.slave) }; } else { - stdin = p.stdin.unwrap(); - stdout = p.stdout.unwrap(); - stderr = p.stderr.unwrap(); + let stdin = p.stdin.unwrap(); + let stdout = p.stdout.unwrap(); + let stderr = p.stderr.unwrap(); child_stdin = unsafe { std::process::Stdio::from_raw_fd(stdin) }; child_stdout = unsafe { std::process::Stdio::from_raw_fd(stdout) }; child_stderr = unsafe { std::process::Stdio::from_raw_fd(stderr) }; diff --git a/src/agent/rustjail/src/mount.rs b/src/agent/rustjail/src/mount.rs index 34d4ae1889..1b4d24d15e 100644 --- a/src/agent/rustjail/src/mount.rs +++ b/src/agent/rustjail/src/mount.rs @@ -111,6 +111,7 @@ lazy_static! { } #[inline(always)] +#[allow(unused_variables)] fn mount( source: Option<&P1>, target: &P2, @@ -125,6 +126,7 @@ fn mount( target: &P, flags: MntFlags, @@ -421,6 +423,7 @@ fn mount_cgroups( Ok(()) } +#[allow(unused_variables)] fn pivot_root( new_root: &P1, put_old: &P2, @@ -553,6 +556,7 @@ fn parse_mount_table() -> Result> { } #[inline(always)] +#[allow(unused_variables)] fn chroot(path: &P) -> Result<(), nix::Error> { #[cfg(not(test))] return unistd::chroot(path); @@ -1004,8 +1008,8 @@ mod tests { // there is no spec.mounts, but should pass let ret = init_rootfs(stdout_fd, &spec, &cpath, &mounts, true); assert!(ret.is_ok(), "Should pass. Got: {:?}", ret); - let ret = fs::remove_dir_all(rootfs.path().join("dev")); - let ret = fs::create_dir(rootfs.path().join("dev")); + let _ = fs::remove_dir_all(rootfs.path().join("dev")); + let _ = fs::create_dir(rootfs.path().join("dev")); // Adding bad mount point to spec.mounts spec.mounts.push(oci::Mount { @@ -1023,8 +1027,8 @@ mod tests { ret ); spec.mounts.pop(); - let ret = fs::remove_dir_all(rootfs.path().join("dev")); - let ret = fs::create_dir(rootfs.path().join("dev")); + let _ = fs::remove_dir_all(rootfs.path().join("dev")); + let _ = fs::create_dir(rootfs.path().join("dev")); // mounting a cgroup spec.mounts.push(oci::Mount { @@ -1037,8 +1041,8 @@ mod tests { let ret = init_rootfs(stdout_fd, &spec, &cpath, &mounts, true); assert!(ret.is_ok(), "Should pass. Got: {:?}", ret); spec.mounts.pop(); - let ret = fs::remove_dir_all(rootfs.path().join("dev")); - let ret = fs::create_dir(rootfs.path().join("dev")); + let _ = fs::remove_dir_all(rootfs.path().join("dev")); + let _ = fs::create_dir(rootfs.path().join("dev")); // mounting /dev spec.mounts.push(oci::Mount { diff --git a/src/agent/rustjail/src/process.rs b/src/agent/rustjail/src/process.rs index 665bd8d071..c832c03c1b 100644 --- a/src/agent/rustjail/src/process.rs +++ b/src/agent/rustjail/src/process.rs @@ -151,11 +151,11 @@ mod tests { #[test] fn test_create_extended_pipe() { // Test the default - let (r, w) = create_extended_pipe(OFlag::O_CLOEXEC, 0).unwrap(); + let (_r, _w) = create_extended_pipe(OFlag::O_CLOEXEC, 0).unwrap(); // Test setting to the max size let max_size = get_pipe_max_size(); - let (r, w) = create_extended_pipe(OFlag::O_CLOEXEC, max_size).unwrap(); + let (_, w) = create_extended_pipe(OFlag::O_CLOEXEC, max_size).unwrap(); let actual_size = get_pipe_size(w); assert_eq!(max_size, actual_size); } diff --git a/src/agent/src/main.rs b/src/agent/src/main.rs index 946adefaa9..e75f55fa04 100644 --- a/src/agent/src/main.rs +++ b/src/agent/src/main.rs @@ -129,7 +129,6 @@ fn main() -> Result<()> { // support vsock log let (rfd, wfd) = unistd::pipe2(OFlag::O_CLOEXEC)?; - let writer = unsafe { File::from_raw_fd(wfd) }; let agentConfig = AGENT_CONFIG.clone(); diff --git a/src/agent/src/mount.rs b/src/agent/src/mount.rs index 2d8ade82bc..a85d5c3a55 100644 --- a/src/agent/src/mount.rs +++ b/src/agent/src/mount.rs @@ -1088,7 +1088,7 @@ mod tests { #[test] fn test_get_cgroup_v2_mounts() { - let dir = tempdir().expect("failed to create tmpdir"); + let _ = tempdir().expect("failed to create tmpdir"); let drain = slog::Discard; let logger = slog::Logger::root(drain, o!()); let result = get_cgroup_mounts(&logger, "", true); diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index 2b3ed22077..a62fe44265 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -790,7 +790,7 @@ impl protocols::agent_ttrpc::AgentService for agentService { fn pause_container( &self, - ctx: &ttrpc::TtrpcContext, + _ctx: &ttrpc::TtrpcContext, req: protocols::agent::PauseContainerRequest, ) -> ttrpc::Result { let cid = req.get_container_id(); @@ -816,7 +816,7 @@ impl protocols::agent_ttrpc::AgentService for agentService { fn resume_container( &self, - ctx: &ttrpc::TtrpcContext, + _ctx: &ttrpc::TtrpcContext, req: protocols::agent::ResumeContainerRequest, ) -> ttrpc::Result { let cid = req.get_container_id(); @@ -1160,7 +1160,7 @@ impl protocols::agent_ttrpc::AgentService for agentService { }; match setup_guest_dns(sl!(), req.dns.to_vec()) { - Ok(dns_list) => { + Ok(_) => { let sandbox = self.sandbox.clone(); let mut s = sandbox.lock().unwrap(); let _ = req From f832d8a651b02ae8e3a38c58b911582cc985bafd Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 17 Sep 2020 18:55:22 +0200 Subject: [PATCH 05/14] rust-agent: Remove or rename unused parameters Parameters that are never used were removed. Parameters that are unused, but necessary because of some common interface were renamed with a _ prefix. In one case, consume the parameter by adding an info! call, and fix a minor typo in a message in the same function. This addresses the following warning: warning: unused variable: `child` --> rustjail/src/container.rs:1128:5 | 1128 | child: &mut Child, | ^^^^^ help: if this is intentional, prefix it with an underscore: `_child` warning: unused variable: `logger` --> rustjail/src/container.rs:1049:22 | 1049 | fn update_namespaces(logger: &Logger, spec: &mut Spec, init_pid: RawFd) -> Result<()> { | ^^^^^^ help: if this is intentional, prefix it with an underscore: `_logger` Fixes: #750 Signed-off-by: Christophe de Dinechin --- src/agent/rustjail/src/cgroups/fs/mod.rs | 6 +++--- src/agent/rustjail/src/container.rs | 5 ++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/agent/rustjail/src/cgroups/fs/mod.rs b/src/agent/rustjail/src/cgroups/fs/mod.rs index 1685ac4a33..13a4c7aa20 100644 --- a/src/agent/rustjail/src/cgroups/fs/mod.rs +++ b/src/agent/rustjail/src/cgroups/fs/mod.rs @@ -230,7 +230,7 @@ impl CgroupManager for Manager { } fn set_network_resources( - cg: &cgroups::Cgroup, + _cg: &cgroups::Cgroup, network: &LinuxNetwork, res: &mut cgroups::Resources, ) -> Result<()> { @@ -259,7 +259,7 @@ fn set_network_resources( } fn set_devices_resources( - cg: &cgroups::Cgroup, + _cg: &cgroups::Cgroup, device_resources: &Vec, res: &mut cgroups::Resources, ) -> Result<()> { @@ -288,7 +288,7 @@ fn set_devices_resources( } fn set_hugepages_resources( - cg: &cgroups::Cgroup, + _cg: &cgroups::Cgroup, hugepage_limits: &Vec, res: &mut cgroups::Resources, ) -> Result<()> { diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 86745207cd..7f3b4d6b08 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -888,7 +888,6 @@ impl BaseContainer for LinuxContainer { &p, self.cgroup_manager.as_ref().unwrap(), &st, - &mut child, pwfd, prfd, ) { @@ -1039,8 +1038,9 @@ 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 container linux field")), + None => return Err(anyhow!("Spec didn't contain linux field")), Some(l) => l, }; @@ -1117,7 +1117,6 @@ fn join_namespaces( p: &Process, cm: &FsManager, st: &OCIState, - _child: &mut Child, pwfd: RawFd, prfd: RawFd, ) -> Result<()> { From c8f406d4c4035331477e467ecc9c8e787c53b9aa Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 17 Sep 2020 18:45:06 +0200 Subject: [PATCH 06/14] rust-agent: Remove uses of deprecated functions This addresses the following: warning: use of deprecated item 'std::error::Error::description': use the Display impl or to_string() --> rustjail/src/container.rs:1598:31 | 1598 | ... e.description(), | ^^^^^^^^^^^ | = note: `#[warn(deprecated)]` on by default Fixes: #750 Signed-off-by: Christophe de Dinechin --- src/agent/rustjail/src/container.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 7f3b4d6b08..68a1c13b40 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -1545,7 +1545,7 @@ fn execute_hook(logger: &Logger, h: &Hook, st: &OCIState) -> Result<()> { info!( logger, "wait child error: {} {}", - e.description(), + e, e.raw_os_error().unwrap() ); From ec24f688ed0e4643fae0211a258ac2d631dcf01a Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 17 Sep 2020 19:08:06 +0200 Subject: [PATCH 07/14] rust-agent: Remove 'mut' where not needed Addresses the following warning (and a few similar ones): warning: variable does not need to be mutable --> rustjail/src/container.rs:369:9 | 369 | let mut oci_process: oci::Process = serde_json::from_str(process_str)?; | ----^^^^^^^^^^^ | | | help: remove this `mut` | = note: `#[warn(unused_mut)]` on by default Fixes: #750 Signed-off-by: Christophe de Dinechin --- src/agent/rustjail/src/cgroups/fs/mod.rs | 4 ++-- src/agent/rustjail/src/container.rs | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/agent/rustjail/src/cgroups/fs/mod.rs b/src/agent/rustjail/src/cgroups/fs/mod.rs index 13a4c7aa20..0999fd5f43 100644 --- a/src/agent/rustjail/src/cgroups/fs/mod.rs +++ b/src/agent/rustjail/src/cgroups/fs/mod.rs @@ -468,7 +468,7 @@ fn build_blk_io_device_throttle_resource( fn linux_device_to_cgroup_device(d: &LinuxDevice) -> DeviceResource { let dev_type = DeviceType::from_char(d.r#type.chars().next()).unwrap(); - let mut permissions = vec![ + let permissions = vec![ DevicePermissions::Read, DevicePermissions::Write, DevicePermissions::MkNod, @@ -518,7 +518,7 @@ fn lines_to_map(content: &str) -> HashMap { .lines() .map(|x| x.split_whitespace().collect::>()) .filter(|x| x.len() == 2 && x[1].parse::().is_ok()) - .fold(HashMap::new(), |mut hm, mut x| { + .fold(HashMap::new(), |mut hm, x| { hm.insert(x[0].to_string(), x[1].parse::().unwrap()); hm }) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 68a1c13b40..3463fbcc75 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -364,7 +364,7 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { let buf = read_sync(crfd)?; let process_str = std::str::from_utf8(&buf)?; - let mut oci_process: oci::Process = serde_json::from_str(process_str)?; + let oci_process: oci::Process = serde_json::from_str(process_str)?; log_child!(cfd_log, "notify parent to send cgroup manager"); write_sync(cwfd, SYNC_SUCCESS, "")?; @@ -799,9 +799,9 @@ impl BaseContainer for LinuxContainer { unistd::close(pwfd); }); - let mut child_stdin: std::process::Stdio; - let mut child_stdout: std::process::Stdio; - let mut child_stderr: std::process::Stdio; + let child_stdin: std::process::Stdio; + let child_stdout: std::process::Stdio; + let child_stderr: std::process::Stdio; if tty { let pseudo = pty::openpty(None, None)?; @@ -865,7 +865,7 @@ impl BaseContainer for LinuxContainer { child = child.env(FIFO_FD, format!("{}", fifofd)); } - let mut child = child.spawn()?; + let child = child.spawn()?; unistd::close(crfd)?; unistd::close(cwfd)?; From c635c46a4b954d54c07c2b41e7deac1f0d720d92 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 17 Sep 2020 19:17:17 +0200 Subject: [PATCH 08/14] rust-agent: Remove unused code that has undefined behavior Some functions have undefined behavior and are not actually used. This addresses the following warning: warning: the type `oci::User` does not permit zero-initialization --> rustjail/src/lib.rs:99:18 | 99 | unsafe { MaybeUninit::zeroed().assume_init() } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | | | this code causes undefined behavior when executed | help: use `MaybeUninit` instead, and only call `assume_init` after initialization is done | = note: `#[warn(invalid_value)]` on by default note: `std::ptr::Unique` must be non-null (in this struct field) warning: the type `protocols::oci::Process` does not permit zero-initialization --> rustjail/src/lib.rs:146:14 | 146 | unsafe { MaybeUninit::zeroed().assume_init() } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | | | this code causes undefined behavior when executed | help: use `MaybeUninit` instead, and only call `assume_init` after initialization is done | note: `std::ptr::Unique` must be non-null (in this struct field) Fixes: #750 Signed-off-by: Christophe de Dinechin --- src/agent/rustjail/src/lib.rs | 29 ++++++----------------------- 1 file changed, 6 insertions(+), 23 deletions(-) diff --git a/src/agent/rustjail/src/lib.rs b/src/agent/rustjail/src/lib.rs index 3fe93a8c9d..d044408474 100644 --- a/src/agent/rustjail/src/lib.rs +++ b/src/agent/rustjail/src/lib.rs @@ -69,7 +69,6 @@ use protocols::oci::{ Root as grpcRoot, Spec as grpcSpec, }; use std::collections::HashMap; -use std::mem::MaybeUninit; pub fn process_grpc_to_oci(p: &grpcProcess) -> ociProcess { let console_size = if p.ConsoleSize.is_some() { @@ -91,7 +90,12 @@ pub fn process_grpc_to_oci(p: &grpcProcess) -> ociProcess { username: u.Username.clone(), } } else { - unsafe { MaybeUninit::zeroed().assume_init() } + ociUser { + uid: 0, + gid: 0, + additional_gids: vec![], + username: String::from(""), + } }; let capabilities = if p.Capabilities.is_some() { @@ -136,11 +140,6 @@ pub fn process_grpc_to_oci(p: &grpcProcess) -> ociProcess { } } -fn process_oci_to_grpc(_p: ociProcess) -> grpcProcess { - // dont implement it for now - unsafe { MaybeUninit::zeroed().assume_init() } -} - fn root_grpc_to_oci(root: &grpcRoot) -> ociRoot { ociRoot { path: root.Path.clone(), @@ -148,10 +147,6 @@ fn root_grpc_to_oci(root: &grpcRoot) -> ociRoot { } } -fn root_oci_to_grpc(_root: &ociRoot) -> grpcRoot { - unsafe { MaybeUninit::zeroed().assume_init() } -} - fn mount_grpc_to_oci(m: &grpcMount) -> ociMount { ociMount { destination: m.destination.clone(), @@ -161,10 +156,6 @@ fn mount_grpc_to_oci(m: &grpcMount) -> ociMount { } } -fn mount_oci_to_grpc(_m: &ociMount) -> grpcMount { - unsafe { MaybeUninit::zeroed().assume_init() } -} - use oci::Hook as ociHook; use protocols::oci::Hook as grpcHook; @@ -195,10 +186,6 @@ fn hooks_grpc_to_oci(h: &grpcHooks) -> ociHooks { } } -fn hooks_oci_to_grpc(_h: &ociHooks) -> grpcHooks { - unsafe { MaybeUninit::zeroed().assume_init() } -} - use oci::{ LinuxDevice as ociLinuxDevice, LinuxIDMapping as ociLinuxIDMapping, LinuxIntelRdt as ociLinuxIntelRdt, LinuxNamespace as ociLinuxNamespace, @@ -565,10 +552,6 @@ pub fn grpc_to_oci(grpc: &grpcSpec) -> ociSpec { } } -pub fn oci_to_grpc(_oci: &ociSpec) -> grpcSpec { - unsafe { MaybeUninit::zeroed().assume_init() } -} - #[cfg(test)] mod tests { #[test] From d5b492a1e755d3af68c185ca4543899d3919cfa8 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 17 Sep 2020 19:47:52 +0200 Subject: [PATCH 09/14] rust-agent: Ignore write errors while writing to the logs When we are writing to the logs and there is an error doing so, there is not much we can do. Chances are that a panic would make things worse. So let it go through. warning: unused `std::result::Result` that must be used --> rustjail/src/sync.rs:26:9 | 26 | write_count(lfd, log_str.as_bytes(), log_str.len()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ::: rustjail/src/container.rs:339:13 | 339 | log_child!(cfd_log, "child exit: {:?}", e); | ------------------------------------------- in this macro invocation | = note: this `Result` may be an `Err` variant, which should be handled = note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info) Fixes: #750 Signed-off-by: Christophe de Dinechin --- src/agent/rustjail/src/sync.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/agent/rustjail/src/sync.rs b/src/agent/rustjail/src/sync.rs index 20bf7b4706..8ce43b270f 100644 --- a/src/agent/rustjail/src/sync.rs +++ b/src/agent/rustjail/src/sync.rs @@ -23,7 +23,8 @@ macro_rules! log_child { let lfd = $fd; let mut log_str = format_args!($($arg)+).to_string(); log_str.push('\n'); - write_count(lfd, log_str.as_bytes(), log_str.len()); + // Ignore error writing to the logger, not much we can do + let _ = write_count(lfd, log_str.as_bytes(), log_str.len()); }) } From ee739c5d598f25250168983dc2be44634c5d0490 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 24 Sep 2020 13:45:58 +0200 Subject: [PATCH 10/14] rust-agent: Report errors to caller if possible Various recently added error-causing calls This addresses the following warning: warning: unused `std::result::Result` that must be used --> rustjail/src/cgroups/fs/mod.rs:93:9 | 93 | cg.add_task(CgroupPid::from(pid as u64)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_must_use)]` on by default = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/cgroups/fs/mod.rs:196:17 | 196 | freezer_controller.thaw(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/cgroups/fs/mod.rs:199:17 | 199 | freezer_controller.freeze(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/cgroups/fs/mod.rs:365:9 | 365 | cpuset_controller.set_cpus(&cpu.cpus); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/cgroups/fs/mod.rs:369:9 | 369 | cpuset_controller.set_mems(&cpu.mems); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/cgroups/fs/mod.rs:381:13 | 381 | cpu_controller.set_shares(shares); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/cgroups/fs/mod.rs:385:5 | 385 | cpu_controller.set_cfs_quota_and_period(cpu.quota, cpu.period); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/cgroups/fs/mod.rs:1061:13 | 1061 | cpuset_controller.set_cpus(cpuset_cpus); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled The specific case of cpu_controller.set_cfs_quota_and_period is addressed in a way that changes the logic following a suggestion by Liu Bin, who had just added the code. Fixes: #750 Suggested-by: Liu Bin Signed-off-by: Christophe de Dinechin --- src/agent/rustjail/src/cgroups/fs/mod.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/agent/rustjail/src/cgroups/fs/mod.rs b/src/agent/rustjail/src/cgroups/fs/mod.rs index 0999fd5f43..8096bebe6b 100644 --- a/src/agent/rustjail/src/cgroups/fs/mod.rs +++ b/src/agent/rustjail/src/cgroups/fs/mod.rs @@ -91,7 +91,7 @@ impl CgroupManager for Manager { let h = cgroups::hierarchies::auto(); let h = Box::new(&*h); let cg = load_or_create(h, &self.cpath); - cg.add_task(CgroupPid::from(pid as u64)); + cg.add_task(CgroupPid::from(pid as u64))?; Ok(()) } @@ -194,10 +194,10 @@ impl CgroupManager for Manager { let freezer_controller: &FreezerController = cg.controller_of().unwrap(); match state { FreezerState::Thawed => { - freezer_controller.thaw(); + freezer_controller.thaw()?; } FreezerState::Frozen => { - freezer_controller.freeze(); + freezer_controller.freeze()?; } _ => { return Err(nix::Error::Sys(Errno::EINVAL).into()); @@ -363,11 +363,11 @@ fn set_cpu_resources(cg: &cgroups::Cgroup, cpu: &LinuxCPU) -> Result<()> { let cpuset_controller: &CpuSetController = cg.controller_of().unwrap(); if !cpu.cpus.is_empty() { - cpuset_controller.set_cpus(&cpu.cpus); + cpuset_controller.set_cpus(&cpu.cpus)?; } if !cpu.mems.is_empty() { - cpuset_controller.set_mems(&cpu.mems); + cpuset_controller.set_mems(&cpu.mems)?; } let cpu_controller: &CpuController = cg.controller_of().unwrap(); @@ -379,11 +379,12 @@ fn set_cpu_resources(cg: &cgroups::Cgroup, cpu: &LinuxCPU) -> Result<()> { shares }; if shares != 0 { - cpu_controller.set_shares(shares); + cpu_controller.set_shares(shares)?; } } - cpu_controller.set_cfs_quota_and_period(cpu.quota, cpu.period); + set_resource!(cpu_controller, set_cfs_quota, cpu, quota); + set_resource!(cpu_controller, set_cfs_period, cpu, period); set_resource!(cpu_controller, set_rt_runtime, cpu, realtime_runtime); set_resource!(cpu_controller, set_rt_period_us, cpu, realtime_period); @@ -1059,7 +1060,7 @@ impl Manager { info!(sl!(), "updating cpuset for path {:?}", &r_path); let cg = load_or_create(h, &r_path); let cpuset_controller: &CpuSetController = cg.controller_of().unwrap(); - cpuset_controller.set_cpus(cpuset_cpus); + cpuset_controller.set_cpus(cpuset_cpus)?; } Ok(()) From d617caf1b53824be07e881fa9fdf0dafbf0584de Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 17 Sep 2020 18:40:56 +0200 Subject: [PATCH 11/14] rust-agent: Remove unused imports This addresses the following warnings (and similar ones):: Compiling rustjail v0.1.0 (/home/ddd/go/src/github.com/kata-containers-2.0/src/agent/rustjail) warning: unused import: `debug` --> rustjail/src/container.rs:57:12 | 57 | use slog::{debug, info, o, Logger}; | ^^^^^ warning: unused imports: `AddressFamily`, `SockFlag`, `SockType`, `self` --> rustjail/src/process.rs:18:24 | 18 | use nix::sys::socket::{self, AddressFamily, SockFlag, SockType}; | ^^^^ ^^^^^^^^^^^^^ ^^^^^^^^ ^^^^^^^^ warning: unused import: `nix::Error` --> rustjail/src/process.rs:23:5 | 23 | use nix::Error; | ^^^^^^^^^^ warning: unused import: `protobuf::RepeatedField` --> rustjail/src/validator.rs:11:5 | 11 | use protobuf::RepeatedField; | ^^^^^^^^^^^^^^^^^^^^^^^ Fixes: #750 Signed-off-by: Christophe de Dinechin --- src/agent/rustjail/src/cgroups/fs/mod.rs | 9 ++-- src/agent/rustjail/src/cgroups/mod.rs | 1 - src/agent/rustjail/src/container.rs | 7 ++- src/agent/rustjail/src/mount.rs | 4 +- src/agent/rustjail/src/process.rs | 2 - src/agent/rustjail/src/validator.rs | 1 - src/agent/src/main.rs | 3 -- src/agent/src/network.rs | 6 +-- src/agent/src/rpc.rs | 3 +- src/agent/src/sandbox.rs | 2 - tools/agent-ctl/Cargo.lock | 63 ++---------------------- tools/agent-ctl/src/utils.rs | 3 +- 12 files changed, 17 insertions(+), 87 deletions(-) diff --git a/src/agent/rustjail/src/cgroups/fs/mod.rs b/src/agent/rustjail/src/cgroups/fs/mod.rs index 8096bebe6b..1b70a28edf 100644 --- a/src/agent/rustjail/src/cgroups/fs/mod.rs +++ b/src/agent/rustjail/src/cgroups/fs/mod.rs @@ -3,7 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 // -use cgroups::blkio::{BlkIo, BlkIoController, BlkIoData, IoService}; +use cgroups::blkio::{BlkIoController, BlkIoData, IoService}; use cgroups::cpu::CpuController; use cgroups::cpuacct::CpuAcctController; use cgroups::cpuset::CpuSetController; @@ -15,18 +15,18 @@ use cgroups::memory::MemController; use cgroups::pid::PidController; use cgroups::{ BlkIoDeviceResource, BlkIoDeviceThrottleResource, Cgroup, CgroupPid, Controller, - DeviceResource, DeviceResources, HugePageResource, MaxValue, NetworkPriority, + DeviceResource, HugePageResource, MaxValue, NetworkPriority, }; use crate::cgroups::Manager as CgroupManager; use crate::container::DEFAULT_DEVICES; -use anyhow::{anyhow, Context, Error, Result}; +use anyhow::{anyhow, Context, Result}; use lazy_static; use libc::{self, pid_t}; use nix::errno::Errno; use oci::{ LinuxBlockIO, LinuxCPU, LinuxDevice, LinuxDeviceCgroup, LinuxHugepageLimit, LinuxMemory, - LinuxNetwork, LinuxPids, LinuxResources, LinuxThrottleDevice, LinuxWeightDevice, + LinuxNetwork, LinuxPids, LinuxResources, }; use protobuf::{CachedSize, RepeatedField, SingularPtrField, UnknownFields}; @@ -34,7 +34,6 @@ use protocols::agent::{ BlkioStats, BlkioStatsEntry, CgroupStats, CpuStats, CpuUsage, HugetlbStats, MemoryData, MemoryStats, PidsStats, ThrottlingData, }; -use regex::Regex; use std::collections::HashMap; use std::fs; use std::path::Path; diff --git a/src/agent/rustjail/src/cgroups/mod.rs b/src/agent/rustjail/src/cgroups/mod.rs index 0e6052542a..c99ef469ac 100644 --- a/src/agent/rustjail/src/cgroups/mod.rs +++ b/src/agent/rustjail/src/cgroups/mod.rs @@ -7,7 +7,6 @@ use anyhow::{anyhow, Result}; use oci::LinuxResources; use protocols::agent::CgroupStats; -use std::collections::HashMap; use cgroups::freezer::FreezerState; diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index 3463fbcc75..d76564d0c1 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -19,7 +19,7 @@ use libc::pid_t; use oci::{LinuxDevice, LinuxIDMapping}; use std::clone::Clone; use std::fmt::Display; -use std::process::{Child, Command}; +use std::process::Command; use cgroups::freezer::FreezerState; @@ -30,7 +30,7 @@ use crate::specconv::CreateOpts; use crate::sync::*; // use crate::stats::Stats; use crate::capabilities::{self, CAPSMAP}; -use crate::cgroups::fs::{self as fscgroup, Manager as FsManager}; +use crate::cgroups::fs::Manager as FsManager; use crate::cgroups::Manager; use crate::{mount, validator}; @@ -55,7 +55,7 @@ use std::io::BufRead; use std::io::BufReader; use std::os::unix::io::FromRawFd; -use slog::{debug, info, o, Logger}; +use slog::{info, o, Logger}; const STATE_FILENAME: &'static str = "state.json"; const EXEC_FIFO_FILENAME: &'static str = "exec.fifo"; @@ -1424,7 +1424,6 @@ fn set_sysctls(sysctls: &HashMap) -> Result<()> { Ok(()) } -use std::error::Error as StdError; use std::io::Read; use std::os::unix::process::ExitStatusExt; use std::process::Stdio; diff --git a/src/agent/rustjail/src/mount.rs b/src/agent/rustjail/src/mount.rs index 1b4d24d15e..0e82c4aee8 100644 --- a/src/agent/rustjail/src/mount.rs +++ b/src/agent/rustjail/src/mount.rs @@ -7,7 +7,9 @@ use anyhow::{anyhow, bail, Context, Error, Result}; use libc::uid_t; use nix::errno::Errno; use nix::fcntl::{self, OFlag}; -use nix::mount::{self, MntFlags, MsFlags}; +#[cfg(not(test))] +use nix::mount; +use nix::mount::{MntFlags, MsFlags}; use nix::sys::stat::{self, Mode, SFlag}; use nix::unistd::{self, Gid, Uid}; use nix::NixPath; diff --git a/src/agent/rustjail/src/process.rs b/src/agent/rustjail/src/process.rs index c832c03c1b..f27c4cda02 100644 --- a/src/agent/rustjail/src/process.rs +++ b/src/agent/rustjail/src/process.rs @@ -15,12 +15,10 @@ use std::sync::mpsc::Sender; use nix::fcntl::{fcntl, FcntlArg, OFlag}; use nix::sys::signal::{self, Signal}; -use nix::sys::socket::{self, AddressFamily, SockFlag, SockType}; use nix::sys::wait::{self, WaitStatus}; use nix::unistd::{self, Pid}; use nix::Result; -use nix::Error; use oci::Process as OCIProcess; use slog::Logger; diff --git a/src/agent/rustjail/src/validator.rs b/src/agent/rustjail/src/validator.rs index 14ffef1bd8..deaf7c14ac 100644 --- a/src/agent/rustjail/src/validator.rs +++ b/src/agent/rustjail/src/validator.rs @@ -8,7 +8,6 @@ use anyhow::{anyhow, Result}; use lazy_static; use nix::errno::Errno; use oci::{LinuxIDMapping, LinuxNamespace, Spec}; -use protobuf::RepeatedField; use std::collections::HashMap; use std::path::{Component, PathBuf}; diff --git a/src/agent/src/main.rs b/src/agent/src/main.rs index e75f55fa04..0dc8667f84 100644 --- a/src/agent/src/main.rs +++ b/src/agent/src/main.rs @@ -25,7 +25,6 @@ extern crate scopeguard; #[macro_use] extern crate slog; -#[macro_use] extern crate netlink; use crate::netlink::{RtnlHandle, NETLINK_ROUTE}; @@ -637,8 +636,6 @@ fn run_debug_console_shell(logger: &Logger, shell: &str, socket_fd: RawFd) -> Re #[cfg(test)] mod tests { use super::*; - use std::fs::File; - use std::io::Write; use tempfile::tempdir; #[test] diff --git a/src/agent/src/network.rs b/src/agent/src/network.rs index 01a088dac9..1fccf5eeec 100644 --- a/src/agent/src/network.rs +++ b/src/agent/src/network.rs @@ -3,15 +3,13 @@ // SPDX-License-Identifier: Apache-2.0 // -use anyhow::{anyhow, Context, Result}; -use nix::mount::{self, MntFlags, MsFlags}; +use anyhow::{anyhow, Result}; +use nix::mount::{self, MsFlags}; use protocols::types::{Interface, Route}; use slog::Logger; use std::collections::HashMap; use std::fs; -use crate::Sandbox; - const KATA_GUEST_SANDBOX_DNS_FILE: &str = "/run/kata-containers/sandbox/resolv.conf"; const GUEST_DNS_FILE: &str = "/etc/resolv.conf"; diff --git a/src/agent/src/rpc.rs b/src/agent/src/rpc.rs index a62fe44265..ed71402742 100644 --- a/src/agent/src/rpc.rs +++ b/src/agent/src/rpc.rs @@ -4,7 +4,7 @@ // use std::path::Path; -use std::sync::mpsc::{channel, Sender}; +use std::sync::mpsc::channel; use std::sync::{Arc, Mutex}; use ttrpc; @@ -40,7 +40,6 @@ use crate::metrics::get_metrics; use crate::mount::{add_storages, remove_mounts, BareMount, STORAGEHANDLERLIST}; use crate::namespace::{NSTYPEIPC, NSTYPEPID, NSTYPEUTS}; use crate::network::setup_guest_dns; -use crate::network::Network; use crate::random; use crate::sandbox::Sandbox; use crate::version::{AGENT_VERSION, API_VERSION}; diff --git a/src/agent/src/sandbox.rs b/src/agent/src/sandbox.rs index 8c5eacbe49..f871d3ef72 100644 --- a/src/agent/src/sandbox.rs +++ b/src/agent/src/sandbox.rs @@ -7,10 +7,8 @@ use crate::linux_abi::*; use crate::mount::{get_mount_fs_type, remove_mounts, TYPEROOTFS}; use crate::namespace::Namespace; -use crate::namespace::NSTYPEPID; use crate::network::Network; use anyhow::{anyhow, Context, Result}; -use cgroups; use libc::pid_t; use netlink::{RtnlHandle, NETLINK_ROUTE}; use oci::{Hook, Hooks}; diff --git a/tools/agent-ctl/Cargo.lock b/tools/agent-ctl/Cargo.lock index 276379c065..ea06728c34 100644 --- a/tools/agent-ctl/Cargo.lock +++ b/tools/agent-ctl/Cargo.lock @@ -1,20 +1,5 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -[[package]] -name = "addr2line" -version = "0.12.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "602d785912f476e480434627e8732e6766b760c045bbf897d9dfaa9f4fbd399c" -dependencies = [ - "gimli", -] - -[[package]] -name = "adler32" -version = "1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "567b077b825e468cc974f0020d4082ee6e03132512f207ef1a02fd5d00d1f32d" - [[package]] name = "aho-corasick" version = "0.7.13" @@ -35,9 +20,9 @@ dependencies = [ [[package]] name = "anyhow" -version = "1.0.31" +version = "1.0.32" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "85bb70cc08ec97ca5450e6eba421deeea5f172c0fc61f78b5357b2a8e8be195f" +checksum = "6b602bfe940d21c130f3895acd65221e8a61270debe89d628b9cb4e3ccb8569b" [[package]] name = "arc-swap" @@ -74,20 +59,6 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f8aac770f1885fd7e387acedd76065302551364496e46b3dd00860b2f8359b9d" -[[package]] -name = "backtrace" -version = "0.3.49" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "05100821de9e028f12ae3d189176b41ee198341eb8f369956407fea2f5cc666c" -dependencies = [ - "addr2line", - "cfg-if", - "libc", - "miniz_oxide", - "object", - "rustc-demangle", -] - [[package]] name = "base64" version = "0.11.0" @@ -240,7 +211,6 @@ version = "0.12.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d371106cc88ffdfb1eabd7111e432da544f16f3e2d7bf1dfe8bf575f1df045cd" dependencies = [ - "backtrace", "version_check", ] @@ -267,12 +237,6 @@ dependencies = [ "wasi", ] -[[package]] -name = "gimli" -version = "0.21.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bcc8e0c9bce37868955864dbecd2b1ab2bdf967e6f28066d65aaac620444b65c" - [[package]] name = "hermit-abi" version = "0.1.14" @@ -361,15 +325,6 @@ version = "2.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3728d817d99e5ac407411fa471ff9800a778d88a24685968b36824eaf4bee400" -[[package]] -name = "miniz_oxide" -version = "0.3.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "791daaae1ed6889560f8c4359194f56648355540573244a5448a83ba1ecc7435" -dependencies = [ - "adler32", -] - [[package]] name = "nix" version = "0.16.1" @@ -415,12 +370,6 @@ dependencies = [ "autocfg", ] -[[package]] -name = "object" -version = "0.20.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1ab52be62400ca80aa00285d25253d7f7c437b7375c4de678f5405d3afe82ca5" - [[package]] name = "oci" version = "0.1.0" @@ -606,19 +555,13 @@ dependencies = [ "crossbeam-utils", ] -[[package]] -name = "rustc-demangle" -version = "0.1.16" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4c691c0e608126e00913e33f0ccf3727d5fc84573623b8d65b2df340b5201783" - [[package]] name = "rustjail" version = "0.1.0" dependencies = [ + "anyhow", "caps", "dirs", - "error-chain", "lazy_static", "libc", "nix 0.17.0", diff --git a/tools/agent-ctl/src/utils.rs b/tools/agent-ctl/src/utils.rs index 6864847605..7dff5d8cb6 100644 --- a/tools/agent-ctl/src/utils.rs +++ b/tools/agent-ctl/src/utils.rs @@ -8,8 +8,7 @@ use anyhow::{anyhow, Result}; use oci::{Process as ociProcess, Root as ociRoot, Spec as ociSpec}; use protocols::oci::{ Box as grpcBox, Linux as grpcLinux, LinuxCapabilities as grpcLinuxCapabilities, - POSIXRlimit as grpcPOSIXRlimit, Process as grpcProcess, Root as grpcRoot, Spec as grpcSpec, - User as grpcUser, + Process as grpcProcess, Root as grpcRoot, Spec as grpcSpec, User as grpcUser, }; use rand::Rng; use slog::{debug, warn}; From 5b2b565249d4ac67c3b801c3413a230450491da8 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Fri, 2 Oct 2020 15:35:34 +0200 Subject: [PATCH 12/14] rust-agent: Log returned errors rather than ignore them In a number of cases, we have functions that return a Result<...> and where the possible error case is simply ignored. This is a bit unhealthy. Add a `check!` macro that allows us to not ignore error values that we want to log, while not interrupting the flow by returning them. This is useful for low-level functions such as `signal::kill` or `unistd::close` where an error is probably significant, but should not necessarily interrupt the flow of the program (i.e. using `call()?` is not the right answer. The check! macro is then used on low-level calls. This addresses the following warnings from #750: This addresses the following warning: warning: unused `std::result::Result` that must be used --> /home/ddd/go/src/github.com/kata-containers-2.0/src/agent/rustjail/src/container.rs:903:17 | 903 | signal::kill(Pid::from_raw(p.pid), Some(Signal::SIGKILL)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_must_use)]` on by default = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> /home/ddd/go/src/github.com/kata-containers-2.0/src/agent/rustjail/src/container.rs:916:17 | 916 | signal::kill(Pid::from_raw(child.id() as i32), Some(Signal::SIGKILL)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:340:13 | 340 | write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_must_use)]` on by default = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:554:13 | 554 | / write_sync( 555 | | cwfd, 556 | | SYNC_FAILED, 557 | | format!("setgroups failed: {:?}", e).as_str(), 558 | | ); | |______________^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:340:13 | 340 | write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:340:13 | 340 | write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_must_use)]` on by default = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:554:13 | 554 | / write_sync( 555 | | cwfd, 556 | | SYNC_FAILED, 557 | | format!("setgroups failed: {:?}", e).as_str(), 558 | | ); | |______________^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:626:5 | 626 | unistd::close(cfd_log); | ^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_must_use)]` on by default = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:627:5 | 627 | unistd::close(crfd); | ^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:628:5 | 628 | unistd::close(cwfd); | ^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:770:9 | 770 | fcntl::fcntl(pfd_log, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_must_use)]` on by default = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:799:9 | 799 | fcntl::fcntl(prfd, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:800:9 | 800 | fcntl::fcntl(pwfd, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:803:13 | 803 | unistd::close(prfd); | ^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:930:9 | 930 | log_handler.join(); | ^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_must_use)]` on by default = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:803:13 | 803 | unistd::close(prfd); | ^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_must_use)]` on by default = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:804:13 | 804 | unistd::close(pwfd); | ^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:842:13 | 842 | sched::setns(old_pid_ns, CloneFlags::CLONE_NEWPID); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/container.rs:843:13 | 843 | unistd::close(old_pid_ns); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled Fixes: #844 Fixes: #750 Suggested-by: Tim Zhang Signed-off-by: Christophe de Dinechin --- src/agent/rustjail/src/container.rs | 93 ++++++++++++++++++++++------- 1 file changed, 70 insertions(+), 23 deletions(-) diff --git a/src/agent/rustjail/src/container.rs b/src/agent/rustjail/src/container.rs index d76564d0c1..e30f4d420a 100644 --- a/src/agent/rustjail/src/container.rs +++ b/src/agent/rustjail/src/container.rs @@ -70,6 +70,17 @@ 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, @@ -336,7 +347,10 @@ pub fn init_child() { Ok(_) => (), Err(e) => { log_child!(cfd_log, "child exit: {:?}", e); - write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()); + check!( + write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()), + "write_sync in init_child()" + ); return; } } @@ -471,11 +485,17 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { if let Err(e) = sched::setns(fd, s) { if s == CloneFlags::CLONE_NEWUSER { if e.as_errno().unwrap() != Errno::EINVAL { - write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()); + check!( + write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()), + "write_sync for CLONE_NEWUSER" + ); return Err(e.into()); } } else { - write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()); + check!( + write_sync(cwfd, SYNC_FAILED, format!("{:?}", e).as_str()), + "write_sync for sched::setns" + ); return Err(e.into()); } } @@ -550,10 +570,13 @@ fn do_init_child(cwfd: RawFd) -> Result<()> { if guser.additional_gids.len() > 0 { setgroups(guser.additional_gids.as_slice()).map_err(|e| { - write_sync( - cwfd, - SYNC_FAILED, - format!("setgroups failed: {:?}", e).as_str(), + check!( + write_sync( + cwfd, + SYNC_FAILED, + format!("setgroups failed: {:?}", e).as_str() + ), + "write_sync for setgroups" ); e })?; @@ -622,9 +645,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"); - unistd::close(cfd_log); - unistd::close(crfd); - unistd::close(cwfd); + check!(unistd::close(cfd_log), "closing cfd log"); + check!(unistd::close(crfd), "closing crfd"); + check!(unistd::close(cwfd), "closing cwfd"); if oci_process.terminal { unistd::setsid()?; @@ -762,7 +785,10 @@ impl BaseContainer for LinuxContainer { let st = self.oci_state()?; let (pfd_log, cfd_log) = unistd::pipe().context("failed to create pipe")?; - fcntl::fcntl(pfd_log, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)); + check!( + fcntl::fcntl(pfd_log, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)), + "fcntl pfd log FD_CLOEXEC" + ); let child_logger = logger.new(o!("action" => "child process log")); let log_handler = thread::spawn(move || { @@ -791,12 +817,18 @@ 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")?; - fcntl::fcntl(prfd, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)); - fcntl::fcntl(pwfd, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)); + 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" + ); defer!({ - unistd::close(prfd); - unistd::close(pwfd); + check!(unistd::close(prfd), "close prfd"); + check!(unistd::close(pwfd), "close pwfd"); }); let child_stdin: std::process::Stdio; @@ -806,8 +838,14 @@ impl BaseContainer for LinuxContainer { if tty { let pseudo = pty::openpty(None, None)?; p.term_master = Some(pseudo.master); - fcntl::fcntl(pseudo.master, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)); - fcntl::fcntl(pseudo.slave, FcntlArg::F_SETFD(FdFlag::FD_CLOEXEC)); + 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" + ); child_stdin = unsafe { std::process::Stdio::from_raw_fd(pseudo.slave) }; child_stdout = unsafe { std::process::Stdio::from_raw_fd(pseudo.slave) }; @@ -834,8 +872,11 @@ impl BaseContainer for LinuxContainer { //restore the parent's process's pid namespace. defer!({ - sched::setns(old_pid_ns, CloneFlags::CLONE_NEWPID); - unistd::close(old_pid_ns); + check!( + sched::setns(old_pid_ns, CloneFlags::CLONE_NEWPID), + "settns CLONE_NEWPID" + ); + check!(unistd::close(old_pid_ns), "close old pid namespace"); }); let pidns = get_pid_namespace(&self.logger, linux)?; @@ -877,7 +918,7 @@ impl BaseContainer for LinuxContainer { } if p.init { - unistd::close(fifofd); + check!(unistd::close(fifofd), "close fifofd"); } info!(logger, "child pid: {}", p.pid); @@ -895,7 +936,10 @@ impl BaseContainer for LinuxContainer { Err(e) => { error!(logger, "create container process error {:?}", e); // kill the child process. - signal::kill(Pid::from_raw(p.pid), Some(Signal::SIGKILL)); + check!( + signal::kill(Pid::from_raw(p.pid), Some(Signal::SIGKILL)), + "signal::kill joining namespaces" + ); return Err(e); } }; @@ -908,7 +952,10 @@ impl BaseContainer for LinuxContainer { let (exit_pipe_r, exit_pipe_w) = unistd::pipe2(OFlag::O_CLOEXEC) .context("failed to create pipe") .map_err(|e| { - signal::kill(Pid::from_raw(child.id() as i32), Some(Signal::SIGKILL)); + check!( + signal::kill(Pid::from_raw(child.id() as i32), Some(Signal::SIGKILL)), + "signal::kill creating pipe" + ); e })?; @@ -922,7 +969,7 @@ impl BaseContainer for LinuxContainer { self.processes.insert(p.pid, p); info!(logger, "wait on child log handler"); - log_handler.join(); + check!(log_handler.join(), "joining log handler"); info!(logger, "create process completed"); return Ok(()); } From 0e4baaabcc5f9831df4523a3242ab7d037575c54 Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Tue, 6 Oct 2020 15:00:17 +0200 Subject: [PATCH 13/14] rust-agent: Identify unused results in tests Assign unused results to _ in order to silence warnings. This addresses the following warnings: warning: unused `std::result::Result` that must be used --> rustjail/src/mount.rs:1182:16 | 1182 | defer!(unistd::chdir(&olddir);); | ^^^^^^^^^^^^^^^^^^^^^^^ | = note: `#[warn(unused_must_use)]` on by default = note: this `Result` may be an `Err` variant, which should be handled warning: unused `std::result::Result` that must be used --> rustjail/src/mount.rs:1183:9 | 1183 | unistd::chdir(tempdir.path()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this `Result` may be an `Err` variant, which should be handled While in regular code, we want to log possible errors, in test code it's OK to simply ignore the returned value. Fixes: #750 Signed-off-by: Christophe de Dinechin --- src/agent/rustjail/src/mount.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/agent/rustjail/src/mount.rs b/src/agent/rustjail/src/mount.rs index 0e82c4aee8..4256a9832d 100644 --- a/src/agent/rustjail/src/mount.rs +++ b/src/agent/rustjail/src/mount.rs @@ -1185,8 +1185,8 @@ mod tests { let tempdir = tempdir().unwrap(); let olddir = unistd::getcwd().unwrap(); - defer!(unistd::chdir(&olddir);); - unistd::chdir(tempdir.path()); + defer!(let _ = unistd::chdir(&olddir);); + let _ = unistd::chdir(tempdir.path()); let dev = oci::LinuxDevice { path: "/fifo".to_string(), From 0e898c6bc4de40ea4fe8c25cdf317053eed3af9c Mon Sep 17 00:00:00 2001 From: Christophe de Dinechin Date: Thu, 24 Sep 2020 13:50:13 +0200 Subject: [PATCH 14/14] rust-agent: Treat warnings as error Avoid the accumulation of warnings we had, as reported in #750. Fixes: #750 Signed-off-by: Christophe de Dinechin --- src/agent/Makefile | 4 ++-- src/trace-forwarder/Makefile | 2 +- tools/agent-ctl/Makefile | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/agent/Makefile b/src/agent/Makefile index b98b9510ef..8b97439e8f 100644 --- a/src/agent/Makefile +++ b/src/agent/Makefile @@ -106,10 +106,10 @@ default: $(TARGET) show-header $(TARGET): $(GENERATED_CODE) $(TARGET_PATH) $(TARGET_PATH): $(SOURCES) | show-summary - @cargo build --target $(TRIPLE) --$(BUILD_TYPE) + @RUSTFLAGS="--deny warnings" cargo build --target $(TRIPLE) --$(BUILD_TYPE) optimize: $(SOURCES) | show-summary show-header - @RUSTFLAGS='-C link-arg=-s' cargo build --target $(TRIPLE) --$(BUILD_TYPE) + @RUSTFLAGS='-C link-arg=-s --deny-warnings' cargo build --target $(TRIPLE) --$(BUILD_TYPE) show-header: @printf "%s - version %s (commit %s)\n\n" "$(TARGET)" "$(VERSION)" "$(COMMIT_MSG)" diff --git a/src/trace-forwarder/Makefile b/src/trace-forwarder/Makefile index e02aef3975..ae73325922 100644 --- a/src/trace-forwarder/Makefile +++ b/src/trace-forwarder/Makefile @@ -6,7 +6,7 @@ default: build build: - cargo build -v + RUSTFLAGS="--deny warnings" cargo build -v clean: cargo clean diff --git a/tools/agent-ctl/Makefile b/tools/agent-ctl/Makefile index e02aef3975..ae73325922 100644 --- a/tools/agent-ctl/Makefile +++ b/tools/agent-ctl/Makefile @@ -6,7 +6,7 @@ default: build build: - cargo build -v + RUSTFLAGS="--deny warnings" cargo build -v clean: cargo clean