diff --git a/lib/src/blockdev.rs b/lib/src/blockdev.rs index 3c69080fa..89ee8e02f 100644 --- a/lib/src/blockdev.rs +++ b/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> { - let o = Command::new("lsblk") +#[context("Listing device {dev}")] +pub(crate) fn list_dev(dev: &Utf8Path) -> Result { + 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 { - 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> { - list_impl(None) -} - #[derive(Debug, Deserialize)] struct SfDiskOutput { partitiontable: PartitionTable, diff --git a/lib/src/cmdutils.rs b/lib/src/cmdutils.rs new file mode 100644 index 000000000..73d5d9fc8 --- /dev/null +++ b/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(&mut self) -> Result; +} + +/// 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}"); + "".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(&mut self) -> Result { + 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()); +} diff --git a/lib/src/image.rs b/lib/src/image.rs index e9d7051ef..5fd84b4f3 100644 --- a/lib/src/image.rs +++ b/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()?; diff --git a/lib/src/imgstorage.rs b/lib/src/imgstorage.rs index c8007bfca..705a8a996 100644 --- a/lib/src/imgstorage.rs +++ b/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(()) } diff --git a/lib/src/install.rs b/lib/src/install.rs index 16b19a817..95eb6d8ec 100644 --- a/lib/src/install.rs +++ b/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"; diff --git a/lib/src/lib.rs b/lib/src/lib.rs index b2263b017..6142b0794 100644 --- a/lib/src/lib.rs +++ b/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; diff --git a/lib/src/mount.rs b/lib/src/mount.rs index f91bf31bd..4acb90b11 100644 --- a/lib/src/mount.rs +++ b/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 { - 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 { ]) .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() diff --git a/lib/src/podman.rs b/lib/src/podman.rs index 4e2d40445..d10457e95 100644 --- a/lib/src/podman.rs +++ b/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 { - 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 = crate::install::run_in_host_mountns("podman") .args(["inspect", imgid]) - .quiet() - .read()?; - let o: Vec = serde_json::from_str(&out)?; + .run_and_parse_json()?; let i = o .into_iter() .next() diff --git a/lib/src/utils.rs b/lib/src/utils.rs index 326faa6a4..2d865fdc2 100644 --- a/lib/src/utils.rs +++ b/lib/src/utils.rs @@ -1,5 +1,5 @@ use std::future::Future; -use std::io::{Read, Seek, Write}; +use std::io::Write; use std::os::fd::BorrowedFd; use std::process::Command; use std::time::Duration; @@ -10,87 +10,6 @@ use ostree::glib; use ostree_ext::container::SignatureSource; use ostree_ext::ostree; -/// Helpers intended for [`std::process::Command`]. -pub(crate) trait CommandRunExt { - fn run(&mut self) -> Result<()>; -} - -/// 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}"); - "".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) - } -} - -/// 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) - } -} - /// Try to look for keys injected by e.g. rpm-ostree requesting machine-local /// changes; if any are present, return `true`. pub(crate) fn origin_has_rpmostree_stuff(kf: &glib::KeyFile) -> bool { @@ -271,46 +190,3 @@ fn test_sigpolicy_from_opts() { SignatureSource::ContainerPolicyAllowInsecure ); } - -#[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" - ); -} - -#[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()); -}