diff --git a/crates/blockdev/src/blockdev.rs b/crates/blockdev/src/blockdev.rs index f9ee28dc3..019a7e8d5 100644 --- a/crates/blockdev/src/blockdev.rs +++ b/crates/blockdev/src/blockdev.rs @@ -322,7 +322,9 @@ impl LoopbackDevice { let _ = cleanup_handle.child.kill(); } - Command::new("losetup").args(["-d", dev.as_str()]).run() + Command::new("losetup") + .args(["-d", dev.as_str()]) + .run_capture_stderr() } /// Consume this device, unmounting it. diff --git a/crates/lib/src/image.rs b/crates/lib/src/image.rs index ebd927567..02556aeee 100644 --- a/crates/lib/src/image.rs +++ b/crates/lib/src/image.rs @@ -178,5 +178,5 @@ pub(crate) async fn imgcmd_entrypoint( let mut cmd = storage.new_image_cmd()?; cmd.arg(arg); cmd.args(args); - cmd.run() + cmd.run_capture_stderr() } diff --git a/crates/lib/src/imgstorage.rs b/crates/lib/src/imgstorage.rs index 2639cf592..39a885570 100644 --- a/crates/lib/src/imgstorage.rs +++ b/crates/lib/src/imgstorage.rs @@ -138,7 +138,7 @@ pub(crate) fn ensure_floating_c_storage_initialized() { if let Err(e) = Command::new("podman") .args(["system", "info"]) .stdout(Stdio::null()) - .run() + .run_capture_stderr() { // Out of conservatism we don't make this operation fatal right now. // If something went wrong, then we'll probably fail on a later operation @@ -220,7 +220,7 @@ impl Storage { new_podman_cmd_in(&storage_root, &run)? .stdout(Stdio::null()) .arg("images") - .run() + .run_capture_stderr() .context("Initializing images")?; Self::ensure_labeled(&storage_root, sepolicy)?; drop(storage_root); @@ -273,7 +273,7 @@ impl Storage { AsyncCommand::from(cmd) .status() .await? - .check_status(stderr)?; + .check_status_with_stderr(stderr)?; // Spawn a helper thread to avoid blocking the main thread // parsing JSON. tokio::task::spawn_blocking(move || -> Result<_> { diff --git a/crates/lib/src/install.rs b/crates/lib/src/install.rs index f59b5d053..6a24837df 100644 --- a/crates/lib/src/install.rs +++ b/crates/lib/src/install.rs @@ -670,7 +670,7 @@ async fn initialize_ostree_root( Command::new("ostree") .args(["config", "--repo", "ostree/repo", "set", k, v]) .cwd_dir(rootfs_dir.try_clone()?) - .run()?; + .run_capture_stderr()?; } let sysroot = { diff --git a/crates/lib/src/install/completion.rs b/crates/lib/src/install/completion.rs index 6c1dd9e2d..58157fca2 100644 --- a/crates/lib/src/install/completion.rs +++ b/crates/lib/src/install/completion.rs @@ -152,7 +152,7 @@ fn bind_from_host( .args(["-m", "-t", "1", "--", "mount", "--bind"]) .arg(src) .arg(&target) - .run()?; + .run_capture_stderr()?; Ok(()) } diff --git a/crates/lib/src/install/osbuild.rs b/crates/lib/src/install/osbuild.rs index 44b815ab3..a6cd0cf95 100644 --- a/crates/lib/src/install/osbuild.rs +++ b/crates/lib/src/install/osbuild.rs @@ -36,7 +36,7 @@ fn adjust_etc_containers(tempdir: &Dir) -> Result<()> { .args(["-t", "overlay", "overlay", "-o", opts.as_str()]) .arg(etc_containers) .cwd_dir(tempdir.try_clone()?) - .run()?; + .run_capture_stderr()?; Ok(()) } @@ -59,7 +59,7 @@ fn propagate_run_osbuild_containers(root: &Dir) -> Result<()> { .arg("--rbind") .args([osbuild_run_containers, relative_storage]) .cwd_dir(root.try_clone()?) - .run()?; + .run_capture_stderr()?; Ok(()) } diff --git a/crates/lib/src/utils.rs b/crates/lib/src/utils.rs index a7ccb0687..f83a718c3 100644 --- a/crates/lib/src/utils.rs +++ b/crates/lib/src/utils.rs @@ -119,7 +119,7 @@ pub(crate) fn spawn_editor(tmpf: &tempfile::NamedTempFile) -> Result<()> { .args(editor_args) .arg(tmpf.path()) .lifecycle_bind() - .run() + .run_inherited() .with_context(|| format!("Invoking editor {editor} failed")) } diff --git a/crates/mount/src/mount.rs b/crates/mount/src/mount.rs index 86a09f26d..71133d9ed 100644 --- a/crates/mount/src/mount.rs +++ b/crates/mount/src/mount.rs @@ -117,7 +117,7 @@ pub fn is_source_mounted(path: &str, mounted_fs: &Filesystem) -> bool { pub fn mount(dev: &str, target: &Utf8Path) -> Result<()> { Command::new("mount") .args([dev, target.as_str()]) - .run_with_cmd_context() + .run_inherited_with_cmd_context() } /// If the fsid of the passed path matches the fsid of the same path rooted diff --git a/crates/ostree-ext/src/fixture.rs b/crates/ostree-ext/src/fixture.rs index cb2625c78..dd1a28902 100644 --- a/crates/ostree-ext/src/fixture.rs +++ b/crates/ostree-ext/src/fixture.rs @@ -769,7 +769,7 @@ impl Fixture { let _ = Command::new("ostree") .arg(format!("--repo={}", self.path.join("src/repo"))) .args(["ls", "-X", "-C", "-R", commit.as_str()]) - .run(); + .run_capture_stderr(); }); } assert_eq!(CONTENTS_CHECKSUM_V0, content_checksum.as_str()); diff --git a/crates/system-reinstall-bootc/src/main.rs b/crates/system-reinstall-bootc/src/main.rs index 7e45e4ccb..00660f606 100644 --- a/crates/system-reinstall-bootc/src/main.rs +++ b/crates/system-reinstall-bootc/src/main.rs @@ -55,12 +55,12 @@ fn run() -> Result<()> { prompt::temporary_developer_protection_prompt()?; reinstall_podman_command - .run_with_cmd_context() + .run_inherited_with_cmd_context() .context("running reinstall command")?; prompt::reboot()?; - std::process::Command::new("reboot").run()?; + std::process::Command::new("reboot").run_capture_stderr()?; Ok(()) } diff --git a/crates/system-reinstall-bootc/src/podman.rs b/crates/system-reinstall-bootc/src/podman.rs index cc87fc500..8ee1005bd 100644 --- a/crates/system-reinstall-bootc/src/podman.rs +++ b/crates/system-reinstall-bootc/src/podman.rs @@ -118,7 +118,7 @@ pub(crate) fn pull_if_not_present(image: &str) -> Result<()> { println!("Image {} is not present locally, pulling it now.", image); println!(); pull_image_command(image) - .run_with_cmd_context() + .run_inherited_with_cmd_context() .context(format!("pulling image {}", image))?; } @@ -150,7 +150,7 @@ pub(crate) fn ensure_podman_installed() -> Result<()> { ); Command::new(podman_install_script_path()) - .run_with_cmd_context() + .run_inherited_with_cmd_context() .context("installing podman")?; // Make sure the installation was actually successful diff --git a/crates/utils/src/command.rs b/crates/utils/src/command.rs index ca191809b..321c10895 100644 --- a/crates/utils/src/command.rs +++ b/crates/utils/src/command.rs @@ -14,24 +14,53 @@ pub trait CommandRunExt { /// Log (at debug level) the full child commandline. fn log_debug(&mut self) -> &mut Self; - /// Execute the child process. - fn run(&mut self) -> Result<()>; + /// Execute the child process and wait for it to exit. + /// + /// # Streams + /// + /// - stdin, stdout, stderr: All inherited + /// + /// # Errors + /// + /// An non-successful exit status will result in an error. + fn run_inherited(&mut self) -> Result<()>; - /// Execute the child process. In case of failure, include the command and its arguments in the - /// error context - fn run_with_cmd_context(&mut self) -> Result<()>; + /// Execute the child process and wait for it to exit. + /// + /// # Streams + /// + /// - stdin, stdout: Inherited + /// - stderr: captured and included in error + /// + /// # Errors + /// + /// An non-successful exit status will result in an error. + fn run_capture_stderr(&mut self) -> Result<()>; + + /// Execute the child process and wait for it to exit; the + /// complete argument list will be included in the error. + /// + /// # Streams + /// + /// - stdin, stdout, stderr: All nherited + /// + /// # Errors + /// + /// An non-successful exit status will result in an error. + fn run_inherited_with_cmd_context(&mut self) -> Result<()>; /// Ensure the child does not outlive the parent. fn lifecycle_bind(&mut self) -> &mut Self; - /// Execute the child process and capture its output. This uses `run` internally + /// Execute the child process and capture its output. This uses `run_capture_stderr` internally /// and will return an error if the child process exits abnormally. fn run_get_output(&mut self) -> Result>; /// Execute the child process and capture its output as a string. + /// This uses `run_capture_stderr` internally. fn run_get_string(&mut self) -> Result; - /// Execute the child process, parsing its stdout as JSON. This uses `run` internally + /// Execute the child process, parsing its stdout as JSON. This uses `run_capture_stderr` internally /// and will return an error if the child process exits abnormally. fn run_and_parse_json(&mut self) -> Result; @@ -45,7 +74,13 @@ pub trait ExitStatusExt { /// 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<()>; + fn check_status(&mut self) -> Result<()>; + + /// If the exit status signals it was not successful, return an error; + /// this also includes the contents of `stderr`. + /// + /// Otherwise this is the same as [`Self::check_status`]. + fn check_status_with_stderr(&mut self, stderr: std::fs::File) -> Result<()>; } /// Parse the last chunk (e.g. 1024 bytes) from the provided file, @@ -81,7 +116,13 @@ fn last_utf8_content_from_file(mut f: std::fs::File) -> String { } impl ExitStatusExt for std::process::ExitStatus { - fn check_status(&mut self, stderr: std::fs::File) -> Result<()> { + fn check_status(&mut self) -> Result<()> { + if self.success() { + return Ok(()); + } + anyhow::bail!(format!("Subprocess failed: {self:?}")) + } + fn check_status_with_stderr(&mut self, stderr: std::fs::File) -> Result<()> { let stderr_buf = last_utf8_content_from_file(stderr); if self.success() { return Ok(()); @@ -91,12 +132,17 @@ impl ExitStatusExt for std::process::ExitStatus { } impl CommandRunExt for Command { + fn run_inherited(&mut self) -> Result<()> { + tracing::trace!("exec: {self:?}"); + self.status()?.check_status() + } + /// Synchronously execute the child, and return an error if the child exited unsuccessfully. - fn run(&mut self) -> Result<()> { + fn run_capture_stderr(&mut self) -> Result<()> { let stderr = tempfile::tempfile()?; self.stderr(stderr.try_clone()?); tracing::trace!("exec: {self:?}"); - self.status()?.check_status(stderr) + self.status()?.check_status_with_stderr(stderr) } #[allow(unsafe_code)] @@ -124,7 +170,7 @@ impl CommandRunExt for Command { fn run_get_output(&mut self) -> Result> { let mut stdout = tempfile::tempfile()?; self.stdout(stdout.try_clone()?); - self.run()?; + self.run_capture_stderr()?; stdout.seek(std::io::SeekFrom::Start(0)).context("seek")?; Ok(Box::new(std::io::BufReader::new(stdout))) } @@ -142,7 +188,7 @@ impl CommandRunExt for Command { serde_json::from_reader(output).map_err(Into::into) } - fn run_with_cmd_context(&mut self) -> Result<()> { + fn run_inherited_with_cmd_context(&mut self) -> Result<()> { self.status()? .success() .then_some(()) @@ -176,7 +222,7 @@ impl AsyncCommandRunExt for tokio::process::Command { async fn run(&mut self) -> Result<()> { let stderr = tempfile::tempfile()?; self.stderr(stderr.try_clone()?); - self.status().await?.check_status(stderr) + self.status().await?.check_status_with_stderr(stderr) } } @@ -185,15 +231,36 @@ mod tests { use super::*; #[test] - fn command_run_ext() { + fn command_run_inherited() { + // Test successful command + Command::new("true").run_inherited().unwrap(); + + // Test failed command + assert!(Command::new("false").run_inherited().is_err()); + + // Test that stderr is not captured (just check error format) + let e = Command::new("/bin/sh") + .args(["-c", "echo should-not-be-captured 1>&2; exit 1"]) + .run_inherited() + .err() + .unwrap(); + // Should not contain the stderr message since it's inherited + assert_eq!( + e.to_string(), + "Subprocess failed: ExitStatus(unix_wait_status(256))" + ); + } + + #[test] + fn command_run_capture_stderr() { // The basics - Command::new("true").run().unwrap(); - assert!(Command::new("false").run().is_err()); + Command::new("true").run_capture_stderr().unwrap(); + assert!(Command::new("false").run_capture_stderr().is_err()); // Verify we capture stderr let e = Command::new("/bin/sh") .args(["-c", "echo expected-this-oops-message 1>&2; exit 1"]) - .run() + .run_capture_stderr() .err() .unwrap(); similar_asserts::assert_eq!( @@ -207,7 +274,7 @@ mod tests { "-c", r"echo -e 'expected\xf5\x80\x80\x80\x80-foo\xc0bar\xc0\xc0' 1>&2; exit 1", ]) - .run() + .run_capture_stderr() .err() .unwrap(); similar_asserts::assert_eq!( @@ -216,6 +283,49 @@ mod tests { ); } + #[test] + fn exit_status_check_status() { + use std::process::Command; + + // Test successful exit status + let mut success_status = Command::new("true").status().unwrap(); + success_status.check_status().unwrap(); + + // Test failed exit status + let mut fail_status = Command::new("false").status().unwrap(); + let e = fail_status.check_status().err().unwrap(); + assert_eq!( + e.to_string(), + "Subprocess failed: ExitStatus(unix_wait_status(256))" + ); + } + + #[test] + fn exit_status_check_status_with_stderr() { + use std::io::Write; + use std::process::Command; + + // Test successful exit status + let mut success_status = Command::new("true").status().unwrap(); + let temp_stderr = tempfile::tempfile().unwrap(); + success_status + .check_status_with_stderr(temp_stderr) + .unwrap(); + + // Test failed exit status with stderr content + let mut fail_status = Command::new("false").status().unwrap(); + let mut temp_stderr = tempfile::tempfile().unwrap(); + write!(temp_stderr, "test error message").unwrap(); + let e = fail_status + .check_status_with_stderr(temp_stderr) + .err() + .unwrap(); + assert!(e + .to_string() + .contains("Subprocess failed: ExitStatus(unix_wait_status(256))")); + assert!(e.to_string().contains("test error message")); + } + #[test] fn command_run_ext_json() { #[derive(serde::Deserialize)]