Repository navigation
nvx-rs: read the time checks' kernel files with one function - #742
Merged
Pedro Henrique Penna (ppenna) merged 1 commit intoOct 22, 2026
Merged
Conversation
check_cpuinfo, check_timer_list, and check_cmdline, nvx-time's
conformance checks C5, C7, and C9 in guest/time/checks.rs, each read a
procfs file with sys::read_file and turned a failure into the check's
Failure in the same ten lines:
sys::read_file(PATH).map_err(|error| {
Failure::new(
ID,
format!(
"read {}: {}",
PATH.to_string_lossy(),
error.desc()
),
)
})?
So each check named its file twice, once to read it and once in the
diagnostic, and the three spelled the diagnostic's form three times.
A private function, read_kernel_file(id, path), now reads the whole of
the file and fails check id with that diagnostic, which names the file
that it read. Each check calls it once, so it names its file and its
check once, and the diagnostic's form has one place. check_clocksource
keeps its own reader: its diagnostic, clocksource sysfs: ERROR, names
neither of its files.
Behavior is unchanged: each check reads the same file through the same
sys::read_file at the same point, and fails with the same CheckId and
the same detail, byte for byte, such as read /proc/cmdline: No such
file or directory, which its conformance event reports as before.
check_timer_list still reads its file again at each poll.
A new unit test checks that read_kernel_file returns the whole of
/proc/cmdline, and that a missing file fails the check with the
diagnostic that names it. nvx-time runs the three checks at every boot
of a guest, and C7 again at capture and restore, which the default
test-microvm suite exercises on each hypervisor.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The localized refactor preserves existing behavior and adds focused coverage, with no unresolved issues.
No open issues 🎉
What changed in this PR
This PR consolidates repeated kernel-file reads and error handling in nvx-time’s conformance checks without changing their behavior.
Changes:
- Adds private
read_kernel_filehelper for C5, C7, and C9, preserving file paths, diagnostics, and polling behavior. - Adds a unit test covering complete file reads and the check-specific failure for a missing file.
| File | Description |
|---|---|
nvx-rs/src/guest/time/checks.rs |
Extracts the shared reader, updates three checks, and adds regression coverage. |
Review effort: Balanced
Pedro Henrique Penna (ppenna)
deleted the
nvx-rs-quality/kernel-file-reads
branch
October 22, 2026 10:16
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
check_cpuinfo,check_timer_list, andcheck_cmdline,nvx-time's conformance checks C5, C7, and C9 innvx-rs/src/guest/time/checks.rs, each read a procfs file withsys::read_fileand turned a failure into the check'sFailurein the same ten lines, such as C7's:So each check named its file twice, once to read it and once in its diagnostic, and the three spelled the diagnostic's form three times. This pull request moves those lines into one private function,
read_kernel_file(id, path), so that each check names its file and its check once, and the diagnostic's form has one place.Changes
All in
nvx-rs/src/guest/time/checks.rs:read_kernel_file(id: CheckId, path: &CStr) -> Result<Vec<u8>, Failure>, a private function beside the checks, reads the whole of the file atpaththroughsys::read_file, and fails checkidwithread PATH: ERROR, aspath.to_string_lossy()and the errno'sdesc()render them.let text = read_kernel_file(ID, PATH)?;:check_cpuinfo: C5,kernel::CPU_INFORMATION(/proc/cpuinfo);check_timer_list: C7,kernel::TIMER_LIST(/proc/timer_list), still inside its polling loop;check_cmdline: C9,kernel::COMMAND_LINE(/proc/cmdline).check_clocksource(C6) keeps its own reader: its diagnostic,clocksource sysfs: ERROR, names neither of its two files, so sharing the function would change it.kernel_files_are_read_whole_or_fail_their_check_naming_the_file, checks both outcomes: the whole of/proc/cmdline, and the C7 failure that a missing file gives,read /proc/nvx-time-missing: No such file or directory.The module loses 15 lines of production code, and the test adds 15: 30 insertions and 30 deletions.
Behavior
Unchanged:
sys::read_file, at the same point, and hands the same bytes to the same decoder.check_timer_liststill reads/proc/timer_listagain at each poll, until it converges or its wait ends.read /proc/cmdline: No such file or directory: the?returns theFailurefrom the check as themap_err(...)?did, and the check's conformance event reports it as before.checks.rsis one of their sources.Follow-ups
Checks::load,cmd_checkandcmd_pre_captureinguest/time/commands.rsandrestore_workanddeferred_boot_checksinguest/time/daemon.rs, each record CPU sets that cannot be read in the same way,Checks::empty(phase)and thenfail(runtime, id, "cannot read the CPU sets"). They differ only in the phase, in the check, C6 or, at boot, C1, and in what follows: failing withCommandError::Operation, going on with the empty set, or exiting.loadcould record that failure itself and return the set that counts it, and leave each caller what follows.guest/time/test_mode.rs,test_cmdline,test_clocksource, and the failure arm oftest_cpuinfoeach print a decoder's verdict asokor as the check's failure event, which one helper could print.Validation
BASE:
83cafb8eabcf4fe32b1d11e6249e58b5b474f232,origin/devwhen the run started. Since then,origin/devhas advanced tof63b4b1f6with the merges of nvx-rs: write the NVXS record header with one function #705 and nvx-rs: lay out execution-helper configurations with one function #689 and two baseline updates, none of which changes this pull request's file, and no open pull request todevchanges it.TREE:
33d72f997ec579ce7ae9c9097d50cb5b37bbe534, the tree of this commit. Every host applied the patch to BASE withgit apply --indexand checked thatgit write-treeequals TREE.Release on every host:
v0.1.0-dev.d1625c50cede, built from BASE's parent, whichdownloadchose because BASE, a[skip ci]baseline update, has no release. Its OpenVMM,9f04d05c5441a9b31f7f81effa0ce7c4fc66a0d3, is the pinned revision, as each host checked inbuild/openvmm.provenance.json.nvx-rs: each host deleted thenvx-rsexecutable thatnvx.pyruns, whichdownloadmay install from the release, rebuilt it from TREE with the Cargo command with whichnvx.pybuilds it, and recorded its provenance withrecord-nvx-rs-provenance. Each host checked that the record names TREE's sources (source_sha256d6be16bac0e4ea810e5a17146440af3679eeeb7b4d36f1f920158484b8df666a, which this machine also computes for TREE) and the new executable, and then built theopenvmm_e2eandmanaged_lifecycletest binaries, after its other Cargo work and before its hypervisor tests.Guest artifacts were rebuilt from TREE on prometheus30, whose user namespaces let Alpine's apk run its scripts (prometheus32 denies them):
build-guest-binaries --output, nine static executables;test-guest-binaries --binaries, where 34 tests passed;build-initramfswithNVX_GUEST_BINARIES.initramfs.cpio.gz(SHA-256a41af1d70858afe9dc35a7479a593c24e47c72897dc10f4bb87f7059da448857), its packages manifest (528dc9e1d8dd466aa6e283f90ccff8480afadc514dcab33a4707f4b5d24fc71b), and its provenance (f925ca0969b443392f14f738a18602a9aded7093340869f25390face1e011a3e) went through this machine to the other two hosts, where the SHA-256 digests matched. The 123 source files that the provenance records match TREE's,checks.rswith TREE's digest.Local checks with Rust 1.95.0. On Windows: rustfmt; Clippy with
-D warningson all targets with all features and on the library without default features; andcargo testwith all features (484 tests in 11 test targets) and with the default ones (407 in 9). On WSL Ubuntu, where theguestmodule compiles: Clippy with-D warningswith all features, on the library without default features, and on all targets of the muslguestconfiguration; and theguest::timeunit tests on the GNU and musl targets, the new test among them.This machine also ran the steps for which the hosts lack the toolchain or target: on Windows, the MSRV (1.89) checks of
nvx,aci_edge_sandboxes, and the runtime adapter's protocol-only and host-only configurations; and, on WSL Ubuntu, the macOS (aarch64-apple-darwin) checks ofnvxandaci_edge_sandboxesand the MSRV check of the runtime adapter's guest-only configuration, which ran as the action defines it, starting with itsrustup target add, which gave the local 1.89 the musl target. I removed that target again afterwards.The change adds, moves, and deletes no file and changes no Markdown, so the CLI tests of validate-nvx and the link check had nothing to cover.
The hosts ran each action's steps with the toolchains and targets that they have, skipping the actions' installation commands. One slip: before any step ran, a probe of prometheus28 listed the targets of the 1.89 toolchain, which that host lacks, and rustup installed it; I uninstalled it two minutes later, and every later command ran with
RUSTUP_AUTO_INSTALL=0.test-aci-edge-sandboxestest-microvm, default suitenvxtests: 898 in 11 test targets with all features and 405 in 9 with the default ones; artifact bundling (15);check-guest-binaries(494 in 9 test targets); the runtime adapter's protocol-only, host-only, and musl guest-only configurations (13, 14, and 21 tests); 158 and 85aci_edge_sandboxestests over the two feature sets; agent bundling (4). The macOS checks and the guest-only MSRV check ran locallynvxtests: 484 in 11 test targets with all features and 407 in 9 with the default ones; artifact bundling (15); the runtime adapter's protocol-only and host-only configurations (13 and 14 tests); 158 and 85aci_edge_sandboxestests over the two feature sets; agent bundling (4). The MSRV checks ran locallyEvery gate and hypervisor test passed at its first run. The Linux gates ran in the foreground of their SSH session, whose shell ignored only
SIGQUIT(SigIgn: 0x4), notSIGINT, so the CLI's Ctrl-C tests ran as intended. Each suite'smanaged-lifecyclescenario ran themanaged_lifecycletest with Cargo, which passed. The time-ABI evidence lines, whose boots, captures, and restores ran the three checks, counted none over its CPU-time budget on any host, and the exhaustive check reportedok/8/0on each. On every host, oneopenvmm_e2etest printedskipped: the host has no second volumefor a check that needs one, and on the Linux hosts the filesystem caller-ownership scenario reported that OpenVMM cannot assume other host identities, so it checked that guest root and foreign guest callers fail withEPERM; both are properties of the hosts, not of this change.Left to CI:
test-openvmm-unitandtest-openvmm, since OpenVMM is unchanged; the VMM suite boots the Alpine image, which CI rebuilds becausechecks.rsis one of its sources;test-microvmscenarios that run only when named:smp-lapic, andmanaged-exec-configandsandbox-lifecycle, which need artifacts that packages omit;verify-guest-determinism;