Skip to content

nvx-rs: read the time checks' kernel files with one function - #742

Merged
Pedro Henrique Penna (ppenna) merged 1 commit into
devfrom
nvx-rs-quality/kernel-file-reads
Oct 22, 2026
Merged

Pedro Henrique Penna (ppenna) merged 1 commit into
devfrom
nvx-rs-quality/kernel-file-reads

Conversation

@ppenna

Copy link
Copy Markdown
Contributor

Summary

check_cpuinfo, check_timer_list, and check_cmdline, nvx-time's conformance checks C5, C7, and C9 in nvx-rs/src/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, such as C7's:

let text = sys::read_file(kernel::TIMER_LIST).map_err(|error| {
    Failure::new(
        CheckId::C7,
        format!(
            "read {}: {}",
            kernel::TIMER_LIST.to_string_lossy(),
            error.desc()
        ),
    )
})?;

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 at path through sys::read_file, and fails check id with read PATH: ERROR, as path.to_string_lossy() and the errno's desc() render them.
  • The sites that now share it, each a single 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.
  • A new unit test, 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:

  • Each check reads the same file through the same sys::read_file, at the same point, and hands the same bytes to the same decoder. check_timer_list still reads /proc/timer_list again at each poll, until it converges or its wait ends.
  • A failed read fails the same check, C5, C7, or C9, with the same detail, byte for byte, such as read /proc/cmdline: No such file or directory: the ? returns the Failure from the check as the map_err(...)? did, and the check's conformance event reports it as before.
  • The function is private to the module: no public item, protocol byte, or guest program's command line, exit status, or output changes. The guest images change only because checks.rs is one of their sources.

Follow-ups

  • The four callers of Checks::load, cmd_check and cmd_pre_capture in guest/time/commands.rs and restore_work and deferred_boot_checks in guest/time/daemon.rs, each record CPU sets that cannot be read in the same way, Checks::empty(phase) and then fail(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 with CommandError::Operation, going on with the empty set, or exiting. load could record that failure itself and return the set that counts it, and leave each caller what follows.
  • In guest/time/test_mode.rs, test_cmdline, test_clocksource, and the failure arm of test_cpuinfo each print a decoder's verdict as ok or as the check's failure event, which one helper could print.

Validation

  • BASE: 83cafb8eabcf4fe32b1d11e6249e58b5b474f232, origin/dev when the run started. Since then, origin/dev has advanced to f63b4b1f6 with 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 to dev changes it.

  • TREE: 33d72f997ec579ce7ae9c9097d50cb5b37bbe534, the tree of this commit. Every host applied the patch to BASE with git apply --index and checked that git write-tree equals TREE.

  • Release on every host: v0.1.0-dev.d1625c50cede, built from BASE's parent, which download chose because BASE, a [skip ci] baseline update, has no release. Its OpenVMM, 9f04d05c5441a9b31f7f81effa0ce7c4fc66a0d3, is the pinned revision, as each host checked in build/openvmm.provenance.json.

  • nvx-rs: each host deleted the nvx-rs executable that nvx.py runs, which download may install from the release, rebuilt it from TREE with the Cargo command with which nvx.py builds it, and recorded its provenance with record-nvx-rs-provenance. Each host checked that the record names TREE's sources (source_sha256 d6be16bac0e4ea810e5a17146440af3679eeeb7b4d36f1f920158484b8df666a, which this machine also computes for TREE) and the new executable, and then built the openvmm_e2e and managed_lifecycle test 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;
    • the Alpine build-initramfs with NVX_GUEST_BINARIES.

    initramfs.cpio.gz (SHA-256 a41af1d70858afe9dc35a7479a593c24e47c72897dc10f4bb87f7059da448857), 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.rs with TREE's digest.

  • Local checks with Rust 1.95.0. On Windows: rustfmt; Clippy with -D warnings on all targets with all features and on the library without default features; and cargo test with all features (484 tests in 11 test targets) and with the default ones (407 in 9). On WSL Ubuntu, where the guest module compiles: Clippy with -D warnings with all features, on the library without default features, and on all targets of the musl guest configuration; and the guest::time unit 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 of nvx and aci_edge_sandboxes and the MSRV check of the runtime adapter's guest-only configuration, which ran as the action defines it, starting with its rustup 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.

Host profile Backend Host type check-nvx-rs and check-aci-edge-sandboxes without a hypervisor Guest artifacts test-aci-edge-sandboxes test-microvm, default suite
prometheus32 kvm baremetal Pass: every Linux step, 29 commands, including the MSRV (1.89) checks; nvx tests: 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 85 aci_edge_sandboxes tests over the two feature sets; agent bundling (4). The macOS checks and the guest-only MSRV check ran locally Copied in, digests match Pass (18/18) Pass (43 scenario runs)
prometheus30 mshv baremetal None; prometheus32 and prometheus28 ran the gates Built and tested here (34 tests) Pass (18/18) Pass (43 scenario runs)
prometheus28 whp baremetal Pass: every Windows step, 21 commands; nvx tests: 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 85 aci_edge_sandboxes tests over the two feature sets; agent bundling (4). The MSRV checks ran locally Copied in, digests match Pass (18/18) Pass (43 scenario runs)

Every 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), not SIGINT, so the CLI's Ctrl-C tests ran as intended. Each suite's managed-lifecycle scenario ran the managed_lifecycle test 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 reported ok/8/0 on each. On every host, one openvmm_e2e test printed skipped: the host has no second volume for 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 with EPERM; both are properties of the hosts, not of this change.

Left to CI:

  • the OpenVMM unit and VMM suites, test-openvmm-unit and test-openvmm, since OpenVMM is unchanged; the VMM suite boots the Alpine image, which CI rebuilds because checks.rs is one of its sources;
  • the test-microvm scenarios that run only when named: smp-lapic, and managed-exec-config and sandbox-lifecycle, which need artifacts that packages omit;
  • the debug-kernel scenarios, the Ubuntu and Azure Linux guests, the Docker guest build, and verify-guest-determinism;
  • the benchmarks and performance gates;
  • the virtual-machine runners;
  • check-quality and validate-nvx, which no Python, shell, or Markdown change touches.

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>
Copilot AI balanced review requested due to automatic review settings October 22, 2026 04:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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_file helper 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants