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 <[email protected]>

Author: cgwalters

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<String> 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<String> {
-    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<String, String> {
@@ -144,15 +128,15 @@ fn split_lsblk_line(line: &str) -> HashMap<String, String> {
 /// 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<Vec<String>> {
-    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) {

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<bool> {
-    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"))?;

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;

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,30 +138,23 @@ pub(crate) fn selinux_set_permissive(permissive: bool) -> Result<()> {
 
 fn selinux_label_for_path(target: &str) -> Result<String> {
     // 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)
 #[context("Labeling {as_path}")]
 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")]

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<String> {
-    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<Inspect> = serde_json::from_slice(&o.stdout)?;
+        .quiet()
+        .read()?;
+    let o: Vec<Inspect> = serde_json::from_str(&out)?;
     let i = o
         .into_iter()
         .next()

lib/src/reboot.rs

@@ -1,21 +1,19 @@
 //! 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")]
 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();

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<str>) -> 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> {
         self.cmd.cwd_dir(dir.try_clone()?);
@@ -55,6 +62,11 @@ impl Task {
         self
     }
 
+    pub(crate) fn arg<S: AsRef<OsStr>>(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)