From d749a038b33bb7c512f8b192fce9609b0a6cf2dd Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 15 Dec 2023 18:02:05 -0500 Subject: [PATCH] Use `Task` generally over `Command` Especially in places where we call `.output()`, what we generally always want is to get stdout, but keep stderr going to the parent process stderr. The `Command` API doesn't make this obvious to do, and worse forces every caller to manually check the exit status. The `Task` API fixes both of these. (Now, the Task API also defaults to printing a description, which we need suppress in many cases with `.quiet()`) Signed-off-by: Colin Walters --- lib/src/blockdev.rs | 26 +++++--------------------- lib/src/install.rs | 13 +++++-------- lib/src/lib.rs | 3 +-- lib/src/lsm.rs | 22 +++++++--------------- lib/src/podman.rs | 12 +++++------- lib/src/reboot.rs | 8 +++----- lib/src/task.rs | 12 ++++++++++++ 7 files changed, 38 insertions(+), 58 deletions(-) diff --git a/lib/src/blockdev.rs b/lib/src/blockdev.rs index 42406a992..455aa0187 100644 --- a/lib/src/blockdev.rs +++ b/lib/src/blockdev.rs @@ -113,22 +113,6 @@ pub(crate) fn reread_partition_table(file: &mut File, retry: bool) -> Result<()> Ok(()) } -/// Runs the provided Command object, captures its stdout, and swallows its stderr except on -/// failure. Returns a Result describing whether the command failed, and if not, its -/// standard output. Output is assumed to be UTF-8. Errors are adequately prefixed with the full -/// command. -pub(crate) fn cmd_output(cmd: &mut Command) -> Result { - let result = cmd - .output() - .with_context(|| format!("running {:#?}", cmd))?; - if !result.status.success() { - eprint!("{}", String::from_utf8_lossy(&result.stderr)); - anyhow::bail!("{:#?} failed with {}", cmd, result.status); - } - String::from_utf8(result.stdout) - .with_context(|| format!("decoding as UTF-8 output of `{:#?}`", cmd)) -} - /// Parse key-value pairs from lsblk --pairs. /// Newer versions of lsblk support JSON but the one in CentOS 7 doesn't. fn split_lsblk_line(line: &str) -> HashMap { @@ -144,15 +128,15 @@ fn split_lsblk_line(line: &str) -> HashMap { /// hierarchy of `device` capable of containing other partitions. So e.g. parent devices of type /// "part" doesn't match, but "disk" and "mpath" does. pub(crate) fn find_parent_devices(device: &str) -> Result> { - let mut cmd = Command::new("lsblk"); - // Older lsblk, e.g. in CentOS 7.6, doesn't support PATH, but --paths option - cmd.arg("--pairs") + let output = Task::new_quiet("lsblk") + // Older lsblk, e.g. in CentOS 7.6, doesn't support PATH, but --paths option + .arg("--pairs") .arg("--paths") .arg("--inverse") .arg("--output") .arg("NAME,TYPE") - .arg(device); - let output = cmd_output(&mut cmd)?; + .arg(device) + .read()?; let mut parents = Vec::new(); // skip first line, which is the device itself for line in output.lines().skip(1) { diff --git a/lib/src/install.rs b/lib/src/install.rs index 5bb9bf408..2033cfbca 100644 --- a/lib/src/install.rs +++ b/lib/src/install.rs @@ -691,14 +691,11 @@ pub(crate) fn exec_in_host_mountns(args: &[std::ffi::OsString]) -> Result<()> { #[context("Querying skopeo version")] fn skopeo_supports_containers_storage() -> Result { - let o = run_in_host_mountns("skopeo").arg("--version").output()?; - let st = o.status; - if !st.success() { - let _ = std::io::copy(&mut std::io::Cursor::new(o.stderr), &mut std::io::stderr()); // Ignore errors copying stderr - anyhow::bail!("Failed to run skopeo --version: {st:?}"); - } - let stdout = String::from_utf8(o.stdout).context("Parsing skopeo version")?; - let mut v = stdout + let out = Task::new_cmd("skopeo --version", run_in_host_mountns("skopeo")) + .args(["--version"]) + .quiet() + .read()?; + let mut v = out .strip_prefix("skopeo version ") .map(|v| v.split('.')) .ok_or_else(|| anyhow::anyhow!("Unexpected output from skopeo version"))?; diff --git a/lib/src/lib.rs b/lib/src/lib.rs index 43d3c635b..3e8cd9d9a 100644 --- a/lib/src/lib.rs +++ b/lib/src/lib.rs @@ -19,6 +19,7 @@ mod lsm; mod reboot; mod reexec; mod status; +mod task; mod utils; #[cfg(feature = "internal-testing-api")] @@ -38,8 +39,6 @@ pub(crate) mod mount; #[cfg(feature = "install")] mod podman; pub mod spec; -#[cfg(feature = "install")] -mod task; #[cfg(feature = "docgen")] mod docgen; diff --git a/lib/src/lsm.rs b/lib/src/lsm.rs index c7328861f..0f7bd73c7 100644 --- a/lib/src/lsm.rs +++ b/lib/src/lsm.rs @@ -12,7 +12,6 @@ use gvariant::{aligned_bytes::TryAsAligned, Marker, Structure}; #[cfg(feature = "install")] use ostree_ext::ostree; -#[cfg(feature = "install")] use crate::task::Task; /// The mount path for selinux @@ -139,14 +138,11 @@ pub(crate) fn selinux_set_permissive(permissive: bool) -> Result<()> { fn selinux_label_for_path(target: &str) -> Result { // TODO: detect case where SELinux isn't enabled - let o = Command::new("matchpathcon").args(["-n", target]).output()?; - let st = o.status; - if !st.success() { - anyhow::bail!("matchpathcon failed: {st:?}"); - } - let label = String::from_utf8(o.stdout)?; - let label = label.trim(); - Ok(label.to_string()) + let label = Task::new_quiet("matchpathcon") + .args(["-n", target]) + .read()?; + // TODO: trim in place instead of reallocating + Ok(label.trim().to_string()) } // Write filesystem labels (currently just for SELinux) @@ -154,15 +150,11 @@ fn selinux_label_for_path(target: &str) -> Result { pub(crate) fn lsm_label(target: &Utf8Path, as_path: &Utf8Path, recurse: bool) -> Result<()> { let label = selinux_label_for_path(as_path.as_str())?; tracing::debug!("Label for {target} is {label}"); - let st = Command::new("chcon") + Task::new_quiet("chcon") .arg("-h") .args(recurse.then_some("-R")) .args(["-h", label.as_str(), target.as_str()]) - .status()?; - if !st.success() { - anyhow::bail!("Failed to invoke chcon: {st:?}"); - } - Ok(()) + .run() } #[cfg(feature = "install")] diff --git a/lib/src/podman.rs b/lib/src/podman.rs index 3b625fd28..f4cf748c0 100644 --- a/lib/src/podman.rs +++ b/lib/src/podman.rs @@ -2,6 +2,7 @@ use anyhow::{anyhow, Result}; use serde::Deserialize; use crate::install::run_in_host_mountns; +use crate::task::Task; #[derive(Deserialize)] #[serde(rename_all = "PascalCase")] @@ -11,14 +12,11 @@ pub(crate) struct Inspect { /// Given an image ID, return its manifest digest pub(crate) fn imageid_to_digest(imgid: &str) -> Result { - let o = run_in_host_mountns("podman") + let out = Task::new_cmd("podman inspect", run_in_host_mountns("podman")) .args(["inspect", imgid]) - .output()?; - let st = o.status; - if !st.success() { - anyhow::bail!("Failed to execute podman inspect: {st:?}"); - } - let o: Vec = serde_json::from_slice(&o.stdout)?; + .quiet() + .read()?; + let o: Vec = serde_json::from_str(&out)?; let i = o .into_iter() .next() diff --git a/lib/src/reboot.rs b/lib/src/reboot.rs index f86cc9f5f..c69178181 100644 --- a/lib/src/reboot.rs +++ b/lib/src/reboot.rs @@ -1,10 +1,11 @@ //! Handling of system restarts/reboot use std::io::Write; -use std::process::Command; use fn_error_context::context; +use crate::task::Task; + /// Initiate a system reboot. /// This function will only return in case of error. #[context("Initiating reboot")] @@ -12,10 +13,7 @@ pub(crate) fn reboot() -> anyhow::Result<()> { // Flush output streams let _ = std::io::stdout().flush(); let _ = std::io::stderr().flush(); - let st = Command::new("reboot").status()?; - if !st.success() { - anyhow::bail!("Failed to reboot: {st:?}"); - } + Task::new("Rebooting system", "reboot").run()?; tracing::debug!("Initiated reboot, sleeping forever..."); loop { std::thread::park(); diff --git a/lib/src/task.rs b/lib/src/task.rs index f6e53f4af..c894f5e7c 100644 --- a/lib/src/task.rs +++ b/lib/src/task.rs @@ -21,6 +21,13 @@ impl Task { Self::new_cmd(description, Command::new(exe.as_ref())) } + /// This API can be used in place of Command::new() generally and just adds error + /// checking on top. + pub(crate) fn new_quiet(exe: impl AsRef) -> Self { + let exe = exe.as_ref(); + Self::new(exe, exe).quiet() + } + /// Set the working directory for this task. pub(crate) fn cwd(mut self, dir: &Dir) -> Result { self.cmd.cwd_dir(dir.try_clone()?); @@ -55,6 +62,11 @@ impl Task { self } + pub(crate) fn arg>(mut self, arg: S) -> Self { + self.cmd.args([arg]); + self + } + /// Run the command, returning an error if the command does not exit successfully. pub(crate) fn run(self) -> Result<()> { self.run_with_stdin_buf(None)