Skip to content

nvx-rs: build the guest programs' test command lines with one helper - #758

Open
Pedro Henrique Penna (ppenna) wants to merge 1 commit into
devfrom
nvx-rs-quality/guest-test-command-lines
Open

Pedro Henrique Penna (ppenna) wants to merge 1 commit into
devfrom
nvx-rs-quality/guest-test-command-lines

Conversation

@ppenna

Copy link
Copy Markdown
Contributor

Summary

The unit tests of seven guest programs, nvx-console-pending, nvx-device-io, nvx-exit, nvx-mmio-write, nvx-reseed, nvx-time, and nvx-time-probe, each built the command line that they pass the program's run with a copy of the same code, such as nvx-exit's:

fn args(arguments: &[&[u8]]) -> Vec<CString> {
    let mut args = vec![CString::new("nvx-exit").unwrap()];
    args.extend(
        arguments
            .iter()
            .map(|argument| CString::new(*argument).unwrap()),
    );
    args
}

That is: the program's name, then each argument, as C strings, the form in which status::run_main passes a program's run the process's command line. Four modules kept that code in an args helper, two with extend and two with iter::once and chain, and three built the command line inline where they run the program, console_pending.rs twice. A test-only function of guest/status.rs, command_line(program, arguments), now builds it for any program, from arguments of any type that is AsRef<[u8]>, so that the tests that pass &str and those that pass &[u8] share it.

This is the thirty-fourth nvx-rs-quality/ pull request. Of the earlier thirty-three, #603 through #705, #742, and #747 merged, the last two during this run, and #750 and #755 are open; each had a Copilot overview review that recommended approval with no comment to follow, #636 and #637 also have approvals without comments, and none was closed without merging. #747 listed "the guest programs' args test helpers" among its follow-ups. #679 gave eight guest programs one main, status::run_main, but the tests of seven of them still each built the command line that it passes.

This run surveyed the crate by reading, by the follow-ups of the earlier pull requests, and with a throwaway script, kept outside the repository, that reports every window of six or more whitespace-normalized lines that repeats in nvx-rs/src and nvx-rs/tests, ignoring blank lines, comments, and lines of punctuation, exactly and with names and literals abstracted. It skipped every file that an open pull request to dev changed when the run started, among them README.md, Cargo.toml, src/lib.rs, protocol/mod.rs, protocol/nvxs/{mod,guest,host}.rs, guest/mod.rs, guest/sys/{mod,fd,filesystem,ioctl}.rs, guest/{cgroup,exec,mappings,process,spawn}.rs, guest/time/checks.rs, guest/managed_agent/{exec,loopback,seam}.rs, net/, host/{artifacts,filesystem,instance,process,seal}.rs, host/platform, host/network/mod.rs, host/python/{mod,fs,windows}.rs, host/store/{mod,files,tests}.rs, host/supervisor/mod.rs, host/sandbox/{mod,distro,mount,plan}.rs and their tests, host/cli/{mod,args}.rs, host/cli/sandbox, testing/fake_openvmm.rs, tests/cli.rs, and tests/fake_openvmm.rs. Among the remaining candidates, this one gives eight copies of one test fixture, in seven modules, one definition, for 78 changed lines, and a build proves that it changes nothing that the guests run. The production candidates that remained either saved fewer lines than a helper's signature and documentation would cost, or were in nvx-time's boot-time code, where #750 found that a change to shared parsing code slowed the boot checks under the guest profile's fat LTO, or needed a trait with as many parameters as the code that it would share; Follow-ups lists them.

Where the choice was open, I took the conservative option:

  • command_line is #[cfg(test)] pub(crate), like spawn::shell of nvx-rs: share the guest tests' /bin/sh -c command helper #750, and lives in guest/status.rs, beside run_main, whose command line it builds, rather than in guest/mod.rs or guest/sys/mod.rs, which open pull requests change.
  • The four args helpers stay, each a one-line wrapper that names its program, so that their 22 calls do not change; I gave them doc comments.
  • command_line panics on an argument with a NUL byte, as every copy did with unwrap.
  • It has no unit test of its own: the tests of the seven modules that run their program build its command line with it, and their assertions on what the programs read from it, such as nvx-exit's code and nvx-device-io's benchmarks, fail if it builds any other command line.

Changes

30 insertions and 48 deletions in eight files of nvx-rs/src/guest, all in test code.

  • status.rs: command_line(program, arguments), #[cfg(test)] pub(crate) with a doc comment, after run_main, returns program and then each of arguments as CStrings, for any arguments: &[A] where A: AsRef<[u8]>.
  • The sites that now share it:
    • console_pending.rs: the console_pending test helper and the reports_a_failed_write test, which built a two-element array;
    • device_io.rs: args, which takes &[&str];
    • exit.rs: args, which takes &[&[u8]];
    • mmio_write.rs: the mmio_write test helper;
    • reseed.rs: Fixture::run_with;
    • time/mod.rs: args;
    • time_probe/mod.rs: args.

Behavior

Unchanged:

  • Each test passes its program the same C strings in the same order as before, and an argument with a NUL byte still panics. reports_a_failed_write passes a Vec where it passed an array; run takes a slice either way.
  • No production code changes: every change is in #[cfg(test)] code that follows all the code that the guest binaries compile, so no line of that code moves either, which matters because panic locations carry line numbers. The guest binaries built from this change are byte for byte those built from BASE: prometheus30 built both, and all nine executables match (see Validation). No public item, message, error kind, protocol byte, golden fixture, command line, exit status, or output changes. The guest images change only because these files are among their sources.
  • Tests that cover it, none of them removed, skipped, or weakened: the 33 tests of the seven modules (5, 8, 3, 4, 9, 2, and 2), which passed on Linux with all features and for the musl target in check-guest-binaries, and on each hypervisor, the guest binaries in the rebuilt initramfs, which test-aci-edge-sandboxes and the default test-microvm suite ran.

Follow-ups

Not in this pull request, to keep it to one change. I found no suspected bug.

  • The same test modules format their expected diagnostics alike: message in console_pending.rs and reseed.rs builds PROGRAM: OPERATION PATH: REASON\n, from an Errno and from bytes, and perror_text in mmio_write.rs builds PREFIX: REASON\n. One helper could serve the three.
  • guest/managed_agent/dispatch.rs's maps_payload and guest/managed_agent/mappings.rs's payload test helpers are identical; the first could import the second.
  • protocol/tests.rs and protocol/nvxs/tests.rs repeat the hex and bytes test helpers; the second could import the first's.
  • guest/ingress/encode.rs's ListenerTable::add and guest/ingress/diag.rs's note_wildcard_tcp repeat a bounded find-or-insert merge.
  • guest/time_probe/latency.rs and warp.rs repeat the unknown-TSC-rate check after detect_tsc_rate, and guest/time/test_mode.rs's test_cmdline, test_clocksource, and test_cpuinfo print a check's verdict alike. Each would save only a few lines.
  • protocol/exec.rs: decode_exec_payload and decode_guest_exec_payload share their header since nvx-rs: decode the EXEC header once for both payload decoders #603, but still decode the arguments, working directory, and environment in parallel, as text and as bytes, with their own messages and their own default for inherit_default_env; a shared decoder would need a trait that keeps every difference.
  • tests/store.rs's spawn_fake and tests/supervisor.rs's fake_openvmm build the same fake-OpenVMM launch, which two integration tests could share only through a module of their own.
  • Earlier runs' follow-ups remain, among them host/python/dumps.rs's container writer, and net::is_valid_guest_network's parse, which waits on nvx-rs: lower sandbox networks through one typed policy with a proxy-only mode #753.

Validation

  • BASE: 2817f4eb79911ef8380dc2b580b9d5c77e6b5b21, origin/dev when the run started. dev has since merged nvx-rs: read the time checks' kernel files with one function #742 and nvx-rs: encode the NVXS roles' DATA and CREDIT with one function #747 (482814366), which change guest/time/checks.rs and protocol/nvxs, none of this change's files.

  • TREE: fa1e8b48cbf48ed64b63aea1b39cdf5d2c787eb0, 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.e714acbd47bf, which download chose because BASE and its parent, two [skip ci] baseline updates that change only data/, have no release. Its OpenVMM, 9f04d05c5441a9b31f7f81effa0ce7c4fc66a0d3, is the pinned revision, as each host checked in build/openvmm.provenance.json.

  • nvx-rs: download installed the release's nvx-rs, which BASE's sources built (source_sha256 ca621c9931225e7c7a7fc219e098016e1cd9ca5aeff1e1def9d165a74a0a3d48), so each host removed it, built nvx-rs with the Cargo command with which nvx.py builds it, and recorded its provenance with record-nvx-rs-provenance. The record names TREE's sources (source_sha256 61ac9f611af4fd6de2e127877e2784f251ffd438cc8747abfc88e059c306bbf5, which this machine also computes for TREE). Cargo reused each host's cached cli build, since that feature compiles none of the changed code. Each host then built the openvmm_e2e and managed_lifecycle test binaries, after its other Cargo work and before its hypervisor tests, whose logs show no compilation.

  • Guest artifacts were rebuilt from TREE on prometheus30, where unshare --user --map-root-user true succeeds, as Alpine's apk needs for its install scripts:

    • 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 51fd870d670a1f58b54c76d7efb95f9b5756b499b0323682c8469f566fa37ae1), its packages manifest (4303d11e81c33f7306e1094a9ae321952977b867389ed27de45f4d0f2be5def2), and its provenance (814905a028490147f223412acbce916eef2511a1fa8112d4c8b0973801e203ab) went through this machine to the other two hosts, where the SHA-256 digests matched.

  • Byte comparison: prometheus30 then reverted the patch with git apply -R --index, checked that git write-tree equals BASE's tree, built BASE's guest binaries with the same command, toolchain, and target directory, and applied the patch again, checking TREE. All nine executables of the two builds are identical, by the digests of their build manifests and by cmp; only the manifests' source digests differ (e77f484a3348f64609d50365a68324cc3f39b1a934b7d42d05f6b9460f880e83 for TREE, ce00ceaec4ecd0b3e0f56e9d78156636cf456d3d7b40a71045b8c23402320c7d for BASE). This ran before the host's hypervisor tests.

  • Local checks with Rust 1.95.0. On Windows: rustfmt, and Clippy with -D warnings on all targets with all features and on the library without default features. On WSL Ubuntu, where the guest module compiles: the same two Clippy runs, and the 54 unit tests of guest::{console_pending,device_io,exit,mmio_write,reseed,status,time_probe} and guest::time::tests.

  • 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, since prometheus28 has no 1.89; 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. All passed.

  • 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, with RUSTUP_AUTO_INSTALL=0. Before any gate, one probe of mine on prometheus28, rustup target list --installed --toolchain 1.89, ran without that variable, and rustup installed the 1.89 toolchain that the host lacks; I uninstalled it at once, which left the host with the toolchains that it had, stable and 1.95.0, and every later command set the variable.

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: 906 in 11 test targets with all features and 423 in 9 with the default ones; artifact bundling (15); check-guest-binaries (507 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 (42 scenario runs)
prometheus30 mshv baremetal None; prometheus32 and prometheus28 ran the gates Built and tested here (34 tests), and BASE's built here for the byte comparison Pass (18/18) Pass (42 scenario runs)
prometheus28 whp baremetal Pass: every Windows step, 21 commands; nvx tests: 502 in 11 test targets with all features and 425 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 (42 scenario runs)

Every gate and hypervisor test passed at its first run. The test counts are one lower than #747's, on the same BASE, which adds one test. The Linux gates ran in the foreground of their SSH session, whose shell ignored no signal (SigIgn: 0), 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 reported the exhaustive check as ok/8/0 on each host and counted no capture or restore over its CPU-time budget. On kvm and mshv, they counted one boot over it, in both cases a 4-vCPU boot of a console-exit scenario: 12,748 µs against a budget of 12,000 on prometheus32, and 6,075 against 6,000 on prometheus30. CI reports those budgets as evidence without gating on them, and nvx-time, which runs the checks, is byte for byte BASE's, so neither overrun comes from this change. On prometheus32 and prometheus28, 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 the changed files are among 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.

The unit tests of seven guest programs, nvx-console-pending,
nvx-device-io, nvx-exit, nvx-mmio-write, nvx-reseed, nvx-time, and
nvx-time-probe, each built the command line that they pass the
program's run with a copy of the same code: the program's name, then
each argument, as C strings. device_io, exit, time, and time_probe kept
that code in an args helper, and console_pending, mmio_write, and
reseed built the command line inline where they run the program,
console_pending twice.

A test-only function of guest/status.rs, command_line, now builds that
command line for any program from arguments of any type that is
AsRef<[u8]>, so that the tests that pass &str and those that pass &[u8]
share it. It sits beside run_main, which passes a program's run the
process's command line in the same form. The four args helpers keep
their names and now only name their programs, so their callers do not
change, and the other sites call command_line directly.

Behavior is unchanged: each test passes the same C strings in the same
order, and an argument with a NUL byte still panics. Every change is in
#[cfg(test)] code that follows all the code that the guest binaries
compile, so no line of that code moves, and the guest binaries built
from this change are byte for byte those built from its parent.

The tests of the seven programs' modules, 33 in all, pass on Linux,
where the guest module compiles: with all features, and for the musl
target in check-guest-binaries. Those that run a program build its
command line with the helper.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 22, 2026 10:49

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 straightforward test-only refactor preserves argument bytes, ordering, NUL rejection, and existing assertions, with no blocking issues identified.

No open issues 🎉

What changed in this PR

This PR consolidates command-line construction in seven guest programs’ unit tests into one test-only helper, continuing the cleanup around status::run_main without changing production behavior.

Changes:

  • Adds status::command_line for string and byte-slice arguments, preserving argument order and NUL rejection.
  • Replaces eight duplicated constructions while retaining existing program-specific wrappers and test assertions.
File Description
nvx-rs/​src/​guest/​time/​mod.rs Delegates the test argument wrapper to the shared helper.
nvx-rs/​src/​guest/​time_probe/​mod.rs Delegates the test argument wrapper to the shared helper.
nvx-rs/​src/​guest/​status.rs Defines and documents the test-only command-line builder.
nvx-rs/​src/​guest/​reseed.rs Uses the shared builder in the test fixture.
nvx-rs/​src/​guest/​mmio_write.rs Replaces inline test argument construction.
nvx-rs/​src/​guest/​exit.rs Retains the program-specific wrapper with a shared implementation.
nvx-rs/​src/​guest/​device_io.rs Retains the string-argument wrapper with a shared implementation.
nvx-rs/​src/​guest/​console_pending.rs Replaces both test command-line constructions.

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