Repository navigation
nvx-rs: convert OSErrors without a filename to CLI exits in one place - #617
Open
Pedro Henrique Penna (ppenna) wants to merge 1 commit into
Open
Pedro Henrique Penna (ppenna) wants to merge 1 commit into
Pedro Henrique Penna (ppenna) wants to merge 1 commit into
Conversation
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>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The narrow refactor preserves existing behavior, adds focused coverage, and has no unresolved findings.
No open issues 🎉
What changed in this PR
Centralizes filename-free OS error conversion in the NVX Rust CLI while preserving messages, exit statuses, and control flow.
Changes:
- Adds
Exit::from_os_errorand replaces repeated conversions. - Removes
output::stream_errorand unused imports. - Adds a test for Python-style error formatting.
| File | Description |
|---|---|
nvx-rs/src/host/cli/session.rs |
Uses shared session error conversion. |
nvx-rs/src/host/cli/sandbox/state.rs |
Uses shared path-resolution error conversion. |
nvx-rs/src/host/cli/sandbox/mod.rs |
Updates output and process-polling conversions. |
nvx-rs/src/host/cli/sandbox/lock.rs |
Updates lock error conversions. |
nvx-rs/src/host/cli/sandbox/lifecycle.rs |
Updates lifecycle error conversions. |
nvx-rs/src/host/cli/output.rs |
Removes the redundant formatting helper. |
nvx-rs/src/host/cli/mod.rs |
Defines the shared conversion and tests its formatting. |
nvx-rs/src/host/cli/egress.rs |
Updates policy-output error conversion. |
nvx-rs/src/host/cli/artifacts.rs |
Updates artifact-output error conversion. |
Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
nvx-rsCLI reports anOSErrorasnvx.pydid: asstr(error), which names a file only when Python'sOSErrorhad one. For theOSErrors that name no file, 21 call sites spelled outExit::error(os_error_message(&error, None)), mostly inside amap_errclosure, andoutput::stream_errorwrapped the same call for 5 more. This change addsExit::from_os_error, which builds that exit once, so that most of the 26 sites now read.map_err(Exit::from_os_error), and removesoutput::stream_error. The CLI now says in one place how anOSErrorwithout a filename reads, and new code has one obvious way to write it.Changes
All of the changes are in
nvx-rs/src/host/cli/.Exit::from_os_error(new,mod.rs) returnsExit::Error(os_error_message(&error, None)). It sits next toExit::errorandExit::from_stop, and takes theio::Errorby value so that it can be passed tomap_errdirectly. These sites now share it:sandbox/lifecycle.rs(13):prepare_state_directory,outcome_destination(2),write_new_json,launch_and_connect(2),end_failed_start(5),wait_interruptibly, andhold_directory.sandbox/lock.rs(4):open_lock_file,try_lock_file,LifecycleLock::acquire, andLifecycleLock::remove.sandbox/mod.rs(3): the output ofrun'sexecand one-shotrunoperations, andpoll.session.rs(2):Session::connectandSession::failure.sandbox/state.rs(1):resolved.artifacts.rs(1) andegress.rs(1):run.mod.rs(1):Exit::report, for help text that cannot be written.output::stream_error(removed) returnedos_error_message(error, None). Its five callers, inartifacts.rs,egress.rs,sandbox/mod.rs(2), andExit::report, are among the sites above.os_errors_without_a_file_read_as_python_formats_them(new test,mod.rs) pins the message for a missing path,[Errno 2] …or[WinError 3] …without the path or Rust's(os error N), and for an error without an OS code, which prints its own text. No existing test reached these messages: the golden CLI scenarios record only anOSErrorthat names a file.session.rsandoutput.rsno longer importos_error_message.Behavior
Exitas before:Exit::Errorofos_error_message(&error, None).Exit::error(message)only moved itsStringintoExit::Error, andstream_errorreturned exactlyos_error_message(error, None). Messages, exit statuses, and output are unchanged, on POSIX and on Windows.map_errstill converts only on failure, and no site gained or lost a check, a system call, or an early return.OSErrors that name a file keep their own conversions:lifecycle.rs'sos_error,lock.rs'sopen_error,state.rs'sos_error_with_two_pathsand its open-coded ones, andsandbox/mod.rs'sspawn_error. So doesidentity.rs, which builds aStringfor its own error type.clifeature compiles these modules, so the library without it,aci_edge_sandboxes, and the guest binaries build as before. The public API, the protocol bytes, the golden fixtures, and the guest binaries' command lines, statuses, and output are unchanged.Follow-ups
lifecycle.rs's privateos_error(error, path), the conversion of anOSErrorthat names a file:state.rs'swrite_json(2) andset_private, andlock.rs'sLifecycleLock::acquireandLifecycleLock::remove. Sharing that helper, for instance besideExit::from_os_error, would complete this consolidation. This change leaves them alone to stay one reviewable step.cgroup.rs'ssetuprepeatsdispatch.rs'sdirect_system, whichloopback.rsbuilds inline too;mappings.rs'spayloadrepeatsdispatch.rs'smaps_payload; and both files carry the same host-export fixture.argstest helpers that build a guest program's argument vector indevice_io.rs,exit.rs,time/mod.rs, andtime_probe/mod.rs, and inconsole_pending.rs,mmio_write.rs, andreseed.rs, which nvx-rs: make the guest failure reporters return their CommandError #614 changes.lifecycle.rsremoves the runtime files in the same order instart_lockedandstop, and reads and checks the runtime record the same way inload_runninganddeprovision.option_values(host/filesystem.rs),rules(host/network/mod.rs), andvalues(host/sandbox/options/tests.rs) are one helper.Validation
830536ed210e0d8bef0ca9166c13fb9636acb614(origin/devwhen this run started).origin/devhas since gained only openvmm: track feature/microvm/dev on microsoft/openvmm #610, which touches none of these files.23e709da2f91f9e129cf639b367ffecdf51c3c8e, the tree of this pull request's commit787fdd0ca0705cf9612c8cad274862b934ea5962.v0.1.0-dev.6c4171aa3166withnvx.py download --hypervisor BACKEND. Its OpenVMM provenance isd0bdbed8f6e3b86271277dc22658109e2e633b2d, which matches the pinned gitlink.nvx-rsCLI: each host rebuilt it from TREE withcargo build --release --locked --features cliand recorded its provenance, with the same source digest (618eb99f…) on all three, so the microVM scenarios that runnvx-rsran this change rather than the release's binary.build-guest-binaries --outputandtest-guest-binaries --binaries(34 passed). The Alpine initramfs was then built withNVX_GUEST_BINARIES=… build-initramfs. The initramfs (54364e73…), its package manifest (97eae955…), and its provenance (1eb6186e…) were copied to the other two hosts with matching SHA-256 digests. No guest binary compiles the changed modules.-D warnings(all features, and the library without default features), the--all-featuresand default tests, including the new test, and rustdoc with-D warnings.checkfornvx-rsandaci_edge_sandboxes, on behalf of the WHP host, which has no 1.89 toolchain.aarch64-apple-darwincheck for both crates, on behalf of the KVM host, which lacks that target.test-aci-edge-sandboxestest-microvm(default suite)check-guest-binaries(320 passed); tests 611 + 263 passed, 1 ignored each; bundling 15 passedunshare --useris denied)The macOS checks for the KVM host's steps and the MSRV checks for the WHP host's steps ran locally, as listed above. No step failed or was rerun.
Left to CI:
test-openvmm-unitandtest-openvmm.test-microvmscenarios that run only when named:smp-lapic,managed-exec-config, andsandbox-lifecycle. The last drives the lifecycle commands whose code this change touches most; the golden CLI scenarios replay those lifecycles locally against the fake OpenVMM.