Merge pull request #746 from cgwalters/block-cmd-cleanup

A few misc cleanups

Author: cgwalters

lib/src/blockdev.rs

@@ -10,6 +10,7 @@ use fn_error_context::context;
 use regex::Regex;
 use serde::Deserialize;
 
+use crate::cmdutils::CommandRunExt;
 use crate::install::run_in_host_mountns;
 use crate::task::Task;
 
@@ -92,35 +93,21 @@ pub(crate) fn wipefs(dev: &Utf8Path) -> Result<()> {
     )
 }
 
-fn list_impl(dev: Option<&Utf8Path>) -> Result<Vec<Device>> {
-    let o = Command::new("lsblk")
+#[context("Listing device {dev}")]
+pub(crate) fn list_dev(dev: &Utf8Path) -> Result<Device> {
+    let mut devs: DevicesOutput = Command::new("lsblk")
         .args(["-J", "-b", "-O"])
-        .args(dev)
-        .output()?;
-    if !o.status.success() {
-        return Err(anyhow::anyhow!("Failed to list block devices"));
-    }
-    let mut devs: DevicesOutput = serde_json::from_reader(&*o.stdout)?;
+        .arg(dev)
+        .run_and_parse_json()?;
     for dev in devs.blockdevices.iter_mut() {
         dev.backfill_missing()?;
     }
-    Ok(devs.blockdevices)
-}
-
-#[context("Listing device {dev}")]
-pub(crate) fn list_dev(dev: &Utf8Path) -> Result<Device> {
-    let devices = list_impl(Some(dev))?;
-    devices
+    devs.blockdevices
         .into_iter()
         .next()
         .ok_or_else(|| anyhow!("no device output from lsblk for {dev}"))
 }
 
-#[allow(dead_code)]
-pub(crate) fn list() -> Result<Vec<Device>> {
-    list_impl(None)
-}
-
 #[derive(Debug, Deserialize)]
 struct SfDiskOutput {
     partitiontable: PartitionTable,

lib/src/cmdutils.rs

@@ -0,0 +1,156 @@
+use std::{
+    io::{Read, Seek},
+    process::Command,
+};
+
+use anyhow::{Context, Result};
+
+/// Helpers intended for [`std::process::Command`].
+pub(crate) trait CommandRunExt {
+    fn run(&mut self) -> Result<()>;
+    /// Execute the child process, parsing its stdout as JSON.
+    fn run_and_parse_json<T: serde::de::DeserializeOwned>(&mut self) -> Result<T>;
+}
+
+/// Helpers intended for [`std::process::ExitStatus`].
+pub(crate) trait ExitStatusExt {
+    /// If the exit status signals it was not successful, return an error.
+    /// Note that we intentionally *don't* include the command string
+    /// in the output; we leave it to the caller to add that if they want,
+    /// as it may be verbose.
+    fn check_status(&mut self, stderr: std::fs::File) -> Result<()>;
+}
+
+/// Parse the last chunk (e.g. 1024 bytes) from the provided file,
+/// ensure it's UTF-8, and return that value. This function is infallible;
+/// if the file cannot be read for some reason, a copy of a static string
+/// is returned.
+fn last_utf8_content_from_file(mut f: std::fs::File) -> String {
+    // u16 since we truncate to just the trailing bytes here
+    // to avoid pathological error messages
+    const MAX_STDERR_BYTES: u16 = 1024;
+    let size = f
+        .metadata()
+        .map_err(|e| {
+            tracing::warn!("failed to fstat: {e}");
+        })
+        .map(|m| m.len().try_into().unwrap_or(u16::MAX))
+        .unwrap_or(0);
+    let size = size.min(MAX_STDERR_BYTES);
+    let seek_offset = -(size as i32);
+    let mut stderr_buf = Vec::with_capacity(size.into());
+    // We should never fail to seek()+read() really, but let's be conservative
+    let r = match f
+        .seek(std::io::SeekFrom::End(seek_offset.into()))
+        .and_then(|_| f.read_to_end(&mut stderr_buf))
+    {
+        Ok(_) => String::from_utf8_lossy(&stderr_buf),
+        Err(e) => {
+            tracing::warn!("failed seek+read: {e}");
+            "<failed to read stderr>".into()
+        }
+    };
+    (&*r).to_owned()
+}
+
+impl ExitStatusExt for std::process::ExitStatus {
+    fn check_status(&mut self, stderr: std::fs::File) -> Result<()> {
+        let stderr_buf = last_utf8_content_from_file(stderr);
+        if self.success() {
+            return Ok(());
+        }
+        anyhow::bail!(format!("Subprocess failed: {self:?}\n{stderr_buf}"))
+    }
+}
+
+impl CommandRunExt for Command {
+    /// Synchronously execute the child, and return an error if the child exited unsuccessfully.
+    fn run(&mut self) -> Result<()> {
+        let stderr = tempfile::tempfile()?;
+        self.stderr(stderr.try_clone()?);
+        self.status()?.check_status(stderr)
+    }
+
+    fn run_and_parse_json<T: serde::de::DeserializeOwned>(&mut self) -> Result<T> {
+        let mut stdout = tempfile::tempfile()?;
+        self.stdout(stdout.try_clone()?);
+        self.run()?;
+        stdout.seek(std::io::SeekFrom::Start(0)).context("seek")?;
+        let stdout = std::io::BufReader::new(stdout);
+        serde_json::from_reader(stdout).map_err(Into::into)
+    }
+}
+
+/// Helpers intended for [`tokio::process::Command`].
+#[allow(dead_code)]
+pub(crate) trait AsyncCommandRunExt {
+    async fn run(&mut self) -> Result<()>;
+}
+
+impl AsyncCommandRunExt for tokio::process::Command {
+    /// Asynchronously execute the child, and return an error if the child exited unsuccessfully.
+    ///
+    async fn run(&mut self) -> Result<()> {
+        let stderr = tempfile::tempfile()?;
+        self.stderr(stderr.try_clone()?);
+        self.status().await?.check_status(stderr)
+    }
+}
+
+#[test]
+fn command_run_ext() {
+    // The basics
+    Command::new("true").run().unwrap();
+    assert!(Command::new("false").run().is_err());
+
+    // Verify we capture stderr
+    let e = Command::new("/bin/sh")
+        .args(["-c", "echo expected-this-oops-message 1>&2; exit 1"])
+        .run()
+        .err()
+        .unwrap();
+    similar_asserts::assert_eq!(
+        e.to_string(),
+        "Subprocess failed: ExitStatus(unix_wait_status(256))\nexpected-this-oops-message\n"
+    );
+
+    // Ignoring invalid UTF-8
+    let e = Command::new("/bin/sh")
+        .args([
+            "-c",
+            r"echo -e 'expected\xf5\x80\x80\x80\x80-foo\xc0bar\xc0\xc0' 1>&2; exit 1",
+        ])
+        .run()
+        .err()
+        .unwrap();
+    similar_asserts::assert_eq!(
+        e.to_string(),
+        "Subprocess failed: ExitStatus(unix_wait_status(256))\nexpected�����-foo�bar��\n"
+    );
+}
+
+#[test]
+fn command_run_ext_json() {
+    #[derive(serde::Deserialize)]
+    struct Foo {
+        a: String,
+        b: u32,
+    }
+    let v: Foo = Command::new("echo")
+        .arg(r##"{"a": "somevalue", "b": 42}"##)
+        .run_and_parse_json()
+        .unwrap();
+    assert_eq!(v.a, "somevalue");
+    assert_eq!(v.b, 42);
+}
+
+#[tokio::test]
+async fn async_command_run_ext() {
+    use tokio::process::Command as AsyncCommand;
+    let mut success = AsyncCommand::new("true");
+    let mut fail = AsyncCommand::new("false");
+    // Run these in parallel just because we can
+    let (success, fail) = tokio::join!(success.run(), fail.run(),);
+    success.unwrap();
+    assert!(fail.is_err());
+}

lib/src/image.rs

@@ -6,7 +6,7 @@ use anyhow::{Context, Result};
 use fn_error_context::context;
 use ostree_ext::container::{ImageReference, Transport};
 
-use crate::{imgstorage::Storage, utils::CommandRunExt};
+use crate::{cmdutils::CommandRunExt, imgstorage::Storage};
 
 /// The name of the image we push to containers-storage if nothing is specified.
 const IMAGE_DEFAULT: &str = "localhost/bootc";
@@ -22,7 +22,7 @@ pub(crate) async fn list_entrypoint() -> Result<()> {
     for image in images {
         println!("{image}");
     }
-    println!("");
+    println!();
 
     println!("# Logically bound images");
     let mut listcmd = sysroot.imgstore.new_image_cmd()?;

lib/src/imgstorage.rs

@@ -22,7 +22,7 @@ use fn_error_context::context;
 use std::os::fd::OwnedFd;
 use tokio::process::Command as AsyncCommand;
 
-use crate::utils::{AsyncCommandRunExt, CommandRunExt, ExitStatusExt};
+use crate::cmdutils::{AsyncCommandRunExt, CommandRunExt, ExitStatusExt};
 
 // Pass only 100 args at a time just to avoid potentially overflowing argument
 // vectors; not that this should happen in reality, but just in case.
@@ -126,10 +126,9 @@ impl Storage {
     }
 
     fn init_globals() -> Result<()> {
-        // Ensure our global storage alias dirs exist
-        for d in [STORAGE_ALIAS_DIR] {
-            std::fs::create_dir_all(d).with_context(|| format!("Creating {d}"))?;
-        }
+        // Ensure our global storage alias dir exists
+        std::fs::create_dir_all(STORAGE_ALIAS_DIR)
+            .with_context(|| format!("Creating {STORAGE_ALIAS_DIR}"))?;
         Ok(())
     }
 

lib/src/install.rs

@@ -43,12 +43,13 @@ use rustix::fs::{FileTypeExt, MetadataExt as _};
 use serde::{Deserialize, Serialize};
 
 use self::baseline::InstallBlockDeviceOpts;
+use crate::cmdutils::CommandRunExt;
 use crate::containerenv::ContainerExecutionInfo;
 use crate::mount::Filesystem;
 use crate::spec::ImageReference;
 use crate::store::Storage;
 use crate::task::Task;
-use crate::utils::{sigpolicy_from_opts, CommandRunExt};
+use crate::utils::sigpolicy_from_opts;
 
 /// The default "stateroot" or "osname"; see https://github.com/ostreedev/ostree/issues/2794
 const STATEROOT_DEFAULT: &str = "default";

lib/src/lib.rs

@@ -10,6 +10,7 @@
 
 mod boundimage;
 pub mod cli;
+mod cmdutils;
 pub(crate) mod deploy;
 pub(crate) mod generator;
 mod image;

lib/src/mount.rs

@@ -1,11 +1,13 @@
 //! Helpers for interacting with mountpoints
 
-use anyhow::{anyhow, Context, Result};
+use std::process::Command;
+
+use anyhow::{anyhow, Result};
 use camino::Utf8Path;
 use fn_error_context::context;
 use serde::Deserialize;
 
-use crate::task::Task;
+use crate::{cmdutils::CommandRunExt, task::Task};
 
 #[derive(Deserialize, Debug)]
 #[serde(rename_all = "kebab-case")]
@@ -26,8 +28,7 @@ pub(crate) struct Findmnt {
 }
 
 fn run_findmnt(args: &[&str], path: &str) -> Result<Filesystem> {
-    let desc = format!("Inspecting {path}");
-    let o = Task::new(desc, "findmnt")
+    let o: Findmnt = Command::new("findmnt")
         .args([
             "-J",
             "-v",
@@ -36,9 +37,7 @@ fn run_findmnt(args: &[&str], path: &str) -> Result<Filesystem> {
         ])
         .args(args)
         .arg(path)
-        .quiet()
-        .read()?;
-    let o: Findmnt = serde_json::from_str(&o).context("Parsing findmnt output")?;
+        .run_and_parse_json()?;
     o.filesystems
         .into_iter()
         .next()

lib/src/podman.rs

@@ -3,8 +3,6 @@ use camino::Utf8Path;
 use cap_std_ext::cap_std::fs::Dir;
 use serde::Deserialize;
 
-use crate::task::Task;
-
 /// Where we look inside our container to find our own image
 /// for use with `bootc install`.
 pub(crate) const CONTAINER_STORAGE: &str = "/var/lib/containers";
@@ -26,12 +24,10 @@ pub(crate) struct ImageListEntry {
 /// Given an image ID, return its manifest digest
 #[cfg(feature = "install")]
 pub(crate) fn imageid_to_digest(imgid: &str) -> Result<String> {
-    use crate::install::run_in_host_mountns;
-    let out = Task::new_cmd("podman inspect", run_in_host_mountns("podman"))
+    use crate::cmdutils::CommandRunExt;
+    let o: Vec<Inspect> = crate::install::run_in_host_mountns("podman")
         .args(["inspect", imgid])
-        .quiet()
-        .read()?;
-    let o: Vec<Inspect> = serde_json::from_str(&out)?;
+        .run_and_parse_json()?;
     let i = o
         .into_iter()
         .next()