From 787fdd0ca0705cf9612c8cad274862b934ea5962 Mon Sep 17 00:00:00 2001 From: Pedro Henrique Penna Date: Tue, 20 Oct 2026 00:40:24 -0300 Subject: [PATCH] nvx-rs: convert OSErrors without a filename to CLI exits in one place The nvx-rs CLI reports an OSError as nvx.py did, as str(error), which names the file of the failed operation only if Python's OSError had one. For the OSErrors that name no file, 21 sites in lifecycle.rs, lock.rs, state.rs, sandbox/mod.rs, and session.rs each spelled out Exit::error(os_error_message(&error, None)), mostly in a map_err closure, and output::stream_error wrapped the same call for the five sites that report a failed write to a standard stream, in artifacts.rs, egress.rs, sandbox/mod.rs, and Exit::report. Exit::from_os_error now builds that exit, next to Exit::error and Exit::from_stop. It takes the io::Error by value, so that most sites become .map_err(Exit::from_os_error), and the CLI says in one place how an OSError without a filename reads. output::stream_error, which nothing else used, goes away. Behavior is unchanged: from_os_error returns Exit::Error(os_error_message(&error, None)), the value that every site built before, so each message and exit status stays the same. The OSErrors that name a file keep their own conversions. A new unit test pins the message for a missing path and for an error without an OS code, which no test reached; the golden CLI scenarios and the sandbox scenarios of the microVM suite run the changed functions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- nvx-rs/src/host/cli/artifacts.rs | 3 +- nvx-rs/src/host/cli/egress.rs | 3 +- nvx-rs/src/host/cli/mod.rs | 34 ++++++++++++++++++++++- nvx-rs/src/host/cli/output.rs | 7 ----- nvx-rs/src/host/cli/sandbox/lifecycle.rs | 35 +++++++++--------------- nvx-rs/src/host/cli/sandbox/lock.rs | 13 ++++----- nvx-rs/src/host/cli/sandbox/mod.rs | 8 ++---- nvx-rs/src/host/cli/sandbox/state.rs | 3 +- nvx-rs/src/host/cli/session.rs | 5 ++-- 9 files changed, 59 insertions(+), 52 deletions(-) diff --git a/nvx-rs/src/host/cli/artifacts.rs b/nvx-rs/src/host/cli/artifacts.rs index e0f19c86c..d8a7b35e7 100644 --- a/nvx-rs/src/host/cli/artifacts.rs +++ b/nvx-rs/src/host/cli/artifacts.rs @@ -49,8 +49,7 @@ pub(super) fn run(root: Option<&str>) -> Result { paths.openvmm = located.openvmm; let value = serde_json::to_value(&paths) .map_err(|error| Exit::error(format!("cannot encode the artifact paths: {error}")))?; - output::print_stdout(&dumps::dumps(&json_value(&value))) - .map_err(|error| Exit::error(output::stream_error(&error))) + output::print_stdout(&dumps::dumps(&json_value(&value))).map_err(Exit::from_os_error) } /// The artifacts that a command runs. diff --git a/nvx-rs/src/host/cli/egress.rs b/nvx-rs/src/host/cli/egress.rs index a73c7a723..5c211fac0 100644 --- a/nvx-rs/src/host/cli/egress.rs +++ b/nvx-rs/src/host/cli/egress.rs @@ -80,6 +80,5 @@ pub(super) fn run(path: &str) -> Result { let policy = compile_policy_file(Path::new(path))?; let rules = |rules: Vec| Value::List(rules.into_iter().map(Value::Str).collect()); let value = dumps::dict([("allow", rules(policy.allow)), ("deny", rules(policy.deny))]); - output::print_stdout(&dumps::dumps(&value)) - .map_err(|error| Exit::error(output::stream_error(&error))) + output::print_stdout(&dumps::dumps(&value)).map_err(Exit::from_os_error) } diff --git a/nvx-rs/src/host/cli/mod.rs b/nvx-rs/src/host/cli/mod.rs index 26de4d070..b6915c717 100644 --- a/nvx-rs/src/host/cli/mod.rs +++ b/nvx-rs/src/host/cli/mod.rs @@ -36,6 +36,7 @@ use std::ffi::OsString; use self::args::{Parser, Receiver, Spec, Stop, Syntax}; use crate::host::platform; use crate::host::python::builtins::repr; +use crate::host::python::fs::os_error_message; /// The name of the program in usage messages unless `--prog` names another. const PROGRAM: &str = "nvx-rs"; @@ -124,6 +125,13 @@ impl Exit { } } + /// Returns the error of an `OSError` that names no file, which prints `str(error)` as + /// [`os_error_message`] formats it, such as `[Errno 32] Broken pipe` for a write to a + /// standard stream whose reader has gone. + pub(crate) fn from_os_error(error: std::io::Error) -> Self { + Self::Error(os_error_message(&error, None)) + } + /// Returns the message that the end prints after `error: `, if it is an error. pub(crate) fn message(&self) -> Option<&str> { match self { @@ -152,7 +160,7 @@ impl Exit { Ok(()) => 0, // Ctrl-C ends a help that waits for its reader, as `KeyboardInterrupt` ends it. Err(_) if platform::interrupted() => Self::Interrupted.report(), - Err(error) => Self::Error(output::stream_error(&error)).report(), + Err(error) => Self::from_os_error(error).report(), }, Self::Interrupted => { output::print_stderr("Interrupted"); @@ -291,3 +299,27 @@ fn run(arguments: &[OsString]) -> Result { Command::Sandbox(options) => sandbox::run(root, *options), } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn os_errors_without_a_file_read_as_python_formats_them() { + let error = std::fs::metadata("definitely/missing/path").unwrap_err(); + let expected = os_error_message(&error, None); + let exit = Exit::from_os_error(error); + let message = exit.message().unwrap(); + assert_eq!(message, expected); + let label = if cfg!(windows) { + "[WinError 3] " + } else { + "[Errno 2] " + }; + assert!(message.starts_with(label), "{message}"); + assert!(!message.contains("os error"), "{message}"); + assert!(!message.contains("missing"), "{message}"); + let error = std::io::Error::other("closed"); + assert_eq!(Exit::from_os_error(error).message(), Some("closed")); + } +} diff --git a/nvx-rs/src/host/cli/output.rs b/nvx-rs/src/host/cli/output.rs index 4756603de..1b0a53afd 100644 --- a/nvx-rs/src/host/cli/output.rs +++ b/nvx-rs/src/host/cli/output.rs @@ -15,7 +15,6 @@ use std::io::{self, Write}; use std::os::fd::AsFd; use crate::host::platform; -use crate::host::python::fs::os_error_message; /// The line separator of this host, `os.linesep`. pub(crate) const LINE_SEPARATOR: &str = if cfg!(windows) { "\r\n" } else { "\n" }; @@ -70,12 +69,6 @@ fn write_all(stream: &mut impl Write, data: &[u8]) -> io::Result<()> { stream.flush() } -/// Returns the message of the `OSError` that writing to a standard stream raises, such as -/// `[Errno 32] Broken pipe`. -pub(crate) fn stream_error(error: &io::Error) -> String { - os_error_message(error, None) -} - #[cfg(test)] mod tests { use super::*; diff --git a/nvx-rs/src/host/cli/sandbox/lifecycle.rs b/nvx-rs/src/host/cli/sandbox/lifecycle.rs index fe4ef1caa..9dd010828 100644 --- a/nvx-rs/src/host/cli/sandbox/lifecycle.rs +++ b/nvx-rs/src/host/cli/sandbox/lifecycle.rs @@ -184,8 +184,7 @@ fn make_private(path: &PurePath) -> Result { /// Resolves the state directory at `path`, and creates it with `create`, as /// `_prepare_state_directory` does. It must be a directory and not a symbolic link. pub(crate) fn prepare_state_directory(path: &str, create: bool) -> Result { - let resolved = fs::resolve(&PurePath::native(path)) - .map_err(|error| Exit::error(os_error_message(&error, None)))?; + let resolved = fs::resolve(&PurePath::native(path)).map_err(Exit::from_os_error)?; if create { make_directories(&resolved, 0o700)?; make_private(&resolved)?; @@ -205,9 +204,7 @@ fn outcome_destination(path: &str) -> Result { let candidate = if path.is_absolute() { path } else { - fs::cwd() - .map_err(|error| Exit::error(os_error_message(&error, None)))? - .join(path.as_str()) + fs::cwd().map_err(Exit::from_os_error)?.join(path.as_str()) }; let parent = candidate .parents() @@ -222,7 +219,7 @@ fn outcome_destination(path: &str) -> Result { return Err(Exit::error("outcome report path has no filename")); } let resolved = fs::resolve(&parent) - .map_err(|error| Exit::error(os_error_message(&error, None)))? + .map_err(Exit::from_os_error)? .join(candidate.name()); if fs::lexists(resolved.as_path()) { return Err(Exit::error(format!( @@ -261,7 +258,7 @@ fn write_new_json(path: &str, value: &Value) -> Result { let text = format!("{}\n", dumps::dumps(value)); file.write_all(text.as_bytes()) .and_then(|()| file.sync_all()) - .map_err(|error| Exit::error(os_error_message(&error, None)))?; + .map_err(Exit::from_os_error)?; match std::fs::hard_link(temporary.as_path(), resolved.as_path()) { Ok(()) => Ok(()), Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => Err(Exit::error( @@ -591,8 +588,7 @@ fn launch_and_connect( launched: &mut Option, ) -> Result { let arguments: Vec = command[1..].iter().map(Into::into).collect(); - let directory = - std::env::current_dir().map_err(|error| Exit::error(os_error_message(&error, None)))?; + let directory = std::env::current_dir().map_err(Exit::from_os_error)?; let child = process::spawn( std::path::Path::new(&command[0]), false, @@ -611,7 +607,7 @@ fn launch_and_connect( let Some(start_time) = start_time else { let exited = child .wait_timeout(Duration::ZERO) - .map_err(|error| Exit::error(os_error_message(&error, None)))?; + .map_err(Exit::from_os_error)?; return Err(Exit::error(if exited.is_some() { "OpenVMM exited during startup" } else { @@ -726,21 +722,17 @@ fn end_failed_start( }; let status = launched .wait_timeout(Duration::ZERO) - .map_err(|error| Exit::error(os_error_message(&error, None)))?; + .map_err(Exit::from_os_error)?; if status.is_none() { - launched - .terminate() - .map_err(|error| Exit::error(os_error_message(&error, None)))?; + launched.terminate().map_err(Exit::from_os_error)?; let exited = launched .wait_timeout(Duration::from_secs(5)) - .map_err(|error| Exit::error(os_error_message(&error, None)))?; + .map_err(Exit::from_os_error)?; if exited.is_none() { - launched - .kill() - .map_err(|error| Exit::error(os_error_message(&error, None)))?; + launched.kill().map_err(Exit::from_os_error)?; let killed = launched .wait_timeout(Duration::from_secs(5)) - .map_err(|error| Exit::error(os_error_message(&error, None)))?; + .map_err(Exit::from_os_error)?; if killed.is_none() { let arguments: Vec = command.iter().map(|argument| repr(argument)).collect(); @@ -765,7 +757,7 @@ fn wait_interruptibly(launched: &mut Launched, deadline: Deadline) -> Result { crate::host::cli::check_interrupt()?; if launched .wait_timeout(slice) - .map_err(|error| Exit::error(os_error_message(&error, None)))? + .map_err(Exit::from_os_error)? .is_some() || deadline.passed() { @@ -931,8 +923,7 @@ fn hold_directory(path: &PurePath) -> Result<((u64, u128), Option)> { .custom_flags((nix::fcntl::OFlag::O_DIRECTORY | nix::fcntl::OFlag::O_NOFOLLOW).bits()) .open(path.as_path()) .map_err(|error| os_error(&error, path))?; - let status = - fs::fstat(&file).map_err(|error| Exit::error(os_error_message(&error, None)))?; + let status = fs::fstat(&file).map_err(Exit::from_os_error)?; Ok((status.identity, Some(file))) } #[cfg(not(unix))] diff --git a/nvx-rs/src/host/cli/sandbox/lock.rs b/nvx-rs/src/host/cli/sandbox/lock.rs index de5becec2..cd71874a4 100644 --- a/nvx-rs/src/host/cli/sandbox/lock.rs +++ b/nvx-rs/src/host/cli/sandbox/lock.rs @@ -133,8 +133,7 @@ fn open_lock_file(path: &PurePath) -> std::result::Result { Err(error) if is_loop(&error) => return Err(OpenFailure::Exit(not_standalone(path))), Err(error) => return Err(OpenFailure::Os(error)), }; - let status = fs::fstat(&file) - .map_err(|error| OpenFailure::Exit(Exit::error(os_error_message(&error, None))))?; + let status = fs::fstat(&file).map_err(|error| OpenFailure::Exit(Exit::from_os_error(error)))?; if !status.regular || status.links != 1 || status.reparse_point { return Err(OpenFailure::Exit(not_standalone(path))); } @@ -150,9 +149,7 @@ fn try_lock_file(file: &File) -> Result { match file.try_lock() { Ok(()) => Ok(true), Err(std::fs::TryLockError::WouldBlock) => Ok(false), - Err(std::fs::TryLockError::Error(error)) => { - Err(Exit::error(os_error_message(&error, None))) - } + Err(std::fs::TryLockError::Error(error)) => Err(Exit::from_os_error(error)), } } @@ -354,8 +351,8 @@ impl LifecycleLock { file: Some(file), }); } - let tombstone = read_lock_file(&file, LOCK_TOMBSTONE.len()) - .map_err(|error| Exit::error(os_error_message(&error, None)))?; + let tombstone = + read_lock_file(&file, LOCK_TOMBSTONE.len()).map_err(Exit::from_os_error)?; // Deprovision leaves the mark only if lingering lock files, such as the one that this // handle may keep, kept it from removing the directory, which the path then still // names. @@ -383,7 +380,7 @@ impl LifecycleLock { pub(crate) fn remove(&mut self) -> Result { let file = self.file.as_ref().expect("the lock is held"); let removed = write_lock_file(file, LOCK_TOMBSTONE) - .map_err(|error| Exit::error(os_error_message(&error, None))) + .map_err(Exit::from_os_error) .and_then(|()| { std::fs::remove_file(self.path.as_path()).map_err(|error| { Exit::error(os_error_message(&error, Some(self.path.as_str()))) diff --git a/nvx-rs/src/host/cli/sandbox/mod.rs b/nvx-rs/src/host/cli/sandbox/mod.rs index f2ed06754..4f413efad 100644 --- a/nvx-rs/src/host/cli/sandbox/mod.rs +++ b/nvx-rs/src/host/cli/sandbox/mod.rs @@ -667,7 +667,7 @@ pub(super) fn run(root: Option<&str>, options: Options) -> Result { )?; output::write_stdout(&result.stdout) .and_then(|()| output::write_stderr(&result.stderr)) - .map_err(|error| Exit::error(output::stream_error(&error)))?; + .map_err(Exit::from_os_error)?; if let Some(report) = &options.outcome_report { lifecycle::write_exec_outcome(report, &result)?; } @@ -713,7 +713,7 @@ pub(super) fn run(root: Option<&str>, options: Options) -> Result { command.extend(["--microvm-report".to_owned(), path_text(report)]); } output::print_stdout(&format!(">> {}", format_command(&command))) - .map_err(|error| Exit::error(output::stream_error(&error)))?; + .map_err(Exit::from_os_error)?; if options.dry_run { return Ok(()); } @@ -770,9 +770,7 @@ fn end_interrupted(child: &mut Child) -> Result { /// Returns the status of `child` if it has exited: `Popen.returncode`, which on Unix is the /// negated signal of a process that a signal ended. fn poll(child: &mut Child) -> Result> { - let status = child - .try_wait() - .map_err(|error| Exit::error(crate::host::python::fs::os_error_message(&error, None)))?; + let status = child.try_wait().map_err(Exit::from_os_error)?; Ok(status.map(|status| { #[cfg(unix)] { diff --git a/nvx-rs/src/host/cli/sandbox/state.rs b/nvx-rs/src/host/cli/sandbox/state.rs index f9ec310b7..d1ea0cb66 100644 --- a/nvx-rs/src/host/cli/sandbox/state.rs +++ b/nvx-rs/src/host/cli/sandbox/state.rs @@ -233,8 +233,7 @@ fn lists_mounts(format: u64) -> bool { /// Returns `path.resolve()` as text. fn resolved(path: &std::path::Path) -> Result { let text = crate::host::cli::sandbox::path_string(path)?; - let resolved = fs::resolve(&PurePath::native(&text)) - .map_err(|error| Exit::error(os_error_message(&error, None)))?; + let resolved = fs::resolve(&PurePath::native(&text)).map_err(Exit::from_os_error)?; Ok(resolved.as_str().to_owned()) } diff --git a/nvx-rs/src/host/cli/session.rs b/nvx-rs/src/host/cli/session.rs index f1aa16fa0..27d70032a 100644 --- a/nvx-rs/src/host/cli/session.rs +++ b/nvx-rs/src/host/cli/session.rs @@ -29,7 +29,6 @@ use std::time::Duration; use super::deadline::{Deadline, RETRY_INTERVAL, sleep}; use super::{Exit, Result, check_interrupt}; use crate::host::platform::{self, Transport}; -use crate::host::python::fs::os_error_message; use crate::host::python::path::PurePath; use crate::host::sandbox::options::validate_exec_environment; use crate::host::session::{ControlSession, SessionError}; @@ -212,7 +211,7 @@ impl Session { .raw_os_error() .is_some_and(|code| raised_at_once(code as u32)) { - return Err(Exit::error(os_error_message(&error, None))); + return Err(Exit::from_os_error(error)); } if deadline.passed() { return Err(Exit::error(format!( @@ -295,7 +294,7 @@ impl Session { if closed || closes(&error) { Exit::EndpointClosed } else { - Exit::error(os_error_message(&error, None)) + Exit::from_os_error(error) } } SessionError::Reset => Exit::error("managed control session was reset"),