Skip to content

nvx-rs: parse the mount mode, owner, and kind names with one function - #685

Open
Pedro Henrique Penna (ppenna) wants to merge 1 commit into
devfrom
nvx-rs-quality/mount-setting-names
Open

Pedro Henrique Penna (ppenna) wants to merge 1 commit into
devfrom
nvx-rs-quality/mount-setting-names

Conversation

@ppenna

Copy link
Copy Markdown
Contributor

Summary

MountAccess, MountOwner, and MountKind, the settings of a sandbox share in nvx-rs/src/host/sandbox/mount.rs, each had a from_name that matched its two names as string literals and refused any other name with a message of one shape, which listed the names again:

match name {
    "ro" => Ok(Self::ReadOnly),
    "rw" => Ok(Self::ReadWrite),
    _ => Err(refused(format!(
        "unsupported sandbox mount mode {}; choose ro or rw",
        repr(name)
    ))),
}

So each setting spelled its names three times: in as_str, in the match of from_name, and in its message. A private function, setting_from_name(setting, values, as_str, name), now holds the lookup and the refusal: it returns the value whose as_str is name, and otherwise refuses name with the message, which it builds from the setting and the values' as_str names. Each from_name is one call of it, so as_str is the one place that spells a setting's names, as Guest::from_name and LayerRole::from_name already find their values by their names. #684 listed this as a follow-up.

This is the twenty-seventh nvx-rs-quality/ pull request. The 19 from #603 to #637 merged, as did #678, and #679 through #684 are open. Each had a Copilot overview review that recommended approval with no comment to follow, #636 and #637 also have approvals without comments to follow, and none was closed without merging. This run surveyed the crate by reading, and with a throwaway script, kept outside the repository, that reports every window of six or more whitespace-normalized lines that repeats in src and tests, ignoring blank lines, comments, and lines of punctuation, exactly and with names and literals abstracted, and once more over production code alone. 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, host/mod.rs, guest/mod.rs, most of the managed agent, guest/sys/{fd,mod,port,process}.rs, guest/time, guest/time_probe, the guest programs' entry points, guest/nvxs.rs, host/instance.rs, host/process.rs, host/filesystem.rs, host/platform, host/store/{mod,tests}.rs, host/sandbox/{mod,options,distro,golden}.rs, host/cli/session.rs, host/cli/sandbox/{lifecycle,mod,state}.rs, host/network/{lowering,policy/mod}.rs, host/python/golden.rs, the NVXS unit tests, tests/cli.rs, and doc/project-structure.md.

Of the candidates in the remaining files, this one gives the most production behavior one definition per changed line: 55 changed lines for the name lookups and refusals of three public settings. host/sandbox/mount.rs was last changed by #634, in its containment checks, not in these methods. The candidates in files that no nvx-rs-quality/ pull request has changed yet, such as the steps that the NVXS session roles' receive and send repeat and the two writers of the NVXS record header, save fewer lines or need a new trait, and the duplicated test helpers that the survey found repeat only fixtures. Follow-ups lists them.

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

  • setting_from_name is private and takes the setting's values and as_str as arguments, rather than being a trait of the three public types. The three from_name methods keep their signatures and documentation, and no public item changes.
  • It takes exactly two values, as each setting has, so that its message keeps the FIRST or SECOND form without a join that only two values would exercise.
  • parse_mount_value, in the same file, keeps its own match for the mode of a --mount value, whose message differs: reusing MountAccess::from_name there would build an error only to replace it. Follow-ups lists it.
  • rustfmt lays out the mode's call on six lines, since its arguments are one column longer than rustfmt allows on one line. I kept rustfmt's layout rather than shorten names for it.

Changes

All changes are in nvx-rs/src/host/sandbox/mount.rs: 31 insertions and 24 deletions.

  • setting_from_name(setting, values, as_str, name), private, with a doc comment: returns the one of the setting's two values whose as_str is name, and otherwise fails with refused, which is ErrorKind::Unsupported, and unsupported sandbox mount {setting} {repr(name)}; choose {first} or {second}, with the values' names in their order.
  • The sites that now share it:
    • MountAccess::from_name: setting_from_name("mode", [Self::ReadOnly, Self::ReadWrite], Self::as_str, name);
    • MountOwner::from_name: setting_from_name("owner", [Self::Vmm, Self::Caller], Self::as_str, name);
    • MountKind::from_name: setting_from_name("kind", [Self::Directory, Self::File], Self::as_str, name).

No public item, message, error kind, protocol byte, fixture, or other file changes.

Behavior

Unchanged:

  • Each from_name accepts the names that its match accepted, ro and rw, vmm and caller, and directory and file, which are the names that as_str gives the same values, and compares them exactly, as the match did. Any other name fails with the same refused error, ErrorKind::Unsupported, and the same message, byte for byte: the setting's word and its two names come in the order in which each message listed them, as in unsupported sandbox mount mode 'rx'; choose ro or rw.

  • Tests that cover it, none of them added, removed, skipped, or weakened:

    • mount_owner_defaults_to_vmm_and_forwards_caller in host/sandbox/mount/tests.rs, which checks the owner's and the mode's messages and their ErrorKind;
    • launch_contract_attaches_a_file_as_an_aggregate_child in host/sandbox/tests.rs, which checks the kind's message;
    • the golden tests mounts_lower_to_the_arguments_that_python_gives, launches_give_the_arguments_and_command_lines_that_python_gives, and sandbox_option_helpers_match_nvx_py in host/sandbox/golden.rs, which parse the owners and kinds of the shares that they replay with these methods;
    • the lifecycle scenarios of tests/cli.rs with one, several, caller-owned, policy, and file shares, in its POSIX and Windows sections, whose start and exec read the recorded shares through all three methods.

    These ran in the gates of the Linux and Windows hosts. The hypervisor tests ran the crates and the nvx-rs CLI built from TREE, but no default scenario reads a share's settings by name: the scenarios that drive nvx.py sandbox with shares, managed-exec-config and sandbox-lifecycle, run only when named, and aci_edge_sandboxes does not use host::sandbox::mount.

Follow-ups

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

  • parse_mount_value in host/sandbox/mount.rs matches ro and rw itself for a --mount value, with a message of its own; it could take MountAccess::from_name and replace its error.
  • host/cli/sandbox/mod.rs lists the owners in MOUNT_OWNERS and matches caller for --mount-owner; both could come from MountOwner::as_str, but nvx-rs: separate distro verification from launch planning #653 changes that file.
  • Guest::from_name in host/artifacts.rs and the role check of SandboxLayer::parse_in in host/sandbox/layer.rs refuse names with unsupported ...; choose A, B messages too, whose names , joins; one helper for all of them would take the separator and the error's constructor.
  • EgressRule::on_port, on_port_range, and on_protocol in host/network/mod.rs each build and push a PortSelector.
  • guest/managed_agent/helper.rs and exec_config.rs define the same file_with test helper, and their tests write EXEC configuration headers byte by byte.
  • protocol/nvxs/guest.rs and host.rs: send repeats four lines after its state check in both roles, besides the lines of receive that nvx-rs: send the guest's attachment, ACK, and CREDIT with one method #684 lists.
  • The other follow-ups that nvx-rs: send the guest's attachment, ACK, and CREDIT with one method #684 lists remain.

Validation

  • BASE: 66159235cd673a401c308a04d1233e0110474a68, origin/dev when the run started. Before this pull request was opened, origin/dev advanced to c30b75afe by the merges of release: name the mismatched fields in Ubuntu artifact manifest errors #664, doc: say when the Quick Start needs Rust to build nvx-rs #670, and nvx-rs: build the NVXS unit tests' epoch identities with one function #678 and a baseline update, which change the NVXS unit tests, scripts/nvx_tools/release.py and its tests, documentation, and data/, none of this pull request's files, and no open pull request to dev changes nvx-rs/src/host/sandbox/mount.rs.

  • TREE: 90363a43674659465315ae9a067716cc82683197, the tree of this commit. Every host applied the patch to BASE and checked that git write-tree equals TREE.

  • Release on every host: v0.1.0-dev.89620759a84c, the nearest first-parent ancestor of BASE that had packages, one commit earlier. Its OpenVMM, d0bdbed8f6e3b86271277dc22658109e2e633b2d, 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, so each host rebuilt it from TREE with the Cargo command with which nvx.py builds it. Cargo recompiled nvx, the executable's digest changed, and record-nvx-rs-provenance recorded it; each host checked that the record names TREE's sources (source_sha256 9a20b0d89b25f60b91b9d9833ffed8ab385e41ba61be56687412b574a0d28436) and the executable. 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, whose user namespaces let Alpine's apk run its scripts (prometheus32 denies them):

    • build-guest-binaries --output;
    • test-guest-binaries --binaries, where 34 tests passed;
    • the Alpine build-initramfs with NVX_GUEST_BINARIES.

    initramfs.cpio.gz (SHA-256 2d12a797f23e00d7363ba9290f7efdb86b0b401227a380f6bc13c8e58f690f4b), its packages manifest (80e2937d2bc92057ed7b1b71e6b3ca7cca194c338878832cfd7fbd5b06363982), and its provenance (fd7e198e086d3e525e45f02cf90b913719c6687d06aacb2cc60e89d36032a369) went through this machine to the other two hosts, where the SHA-256 digests matched.

  • 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 and with the default ones. This machine also ran the steps that no host could run, because 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; nothing was installed on them.

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: 736 in 12 test binaries with all features and 359 in 10 with the default ones, besides the runs of single tests that some tests make of their own binary; artifact bundling (15); check-guest-binaries (384); 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 (41 scenario runs)
prometheus30 mshv baremetal None; prometheus32 and prometheus28 ran the gates Built and tested here (34 tests) Pass (18/18) Pass (41 scenario runs)
prometheus28 whp baremetal Pass: every Windows step, 21 commands; nvx tests: 429 with all features and 358 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 Linux gates ran in the foreground of their SSH session, where no signal was ignored (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. OpenVMM warned of a host CPU-profile fallback only in the cpu-profile-fallback scenario, which forces one. The time-ABI evidence lines counted no capture or restore over its CPU-time budget on any host, and no boot on prometheus32 and prometheus28; on prometheus30, they counted 1 of 58 boots, which the evidence reports without failing the suite, in the guest's own conformance checks, which this change does not reach. 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 test-microvm scenarios that run only when named: smp-lapic, and managed-exec-config and sandbox-lifecycle, which need the Ubuntu scratch template that packages omit and drive nvx.py sandbox with shares;
  • 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.

MountAccess, MountOwner, and MountKind, the settings of a sandbox share
in host/sandbox/mount.rs, each had a from_name that matched its two
names as string literals and refused any other name with a message of
one shape, which listed the names again:

    unsupported sandbox mount SETTING 'NAME'; choose FIRST or SECOND

So each setting spelled its names three times: in as_str, in the match
of from_name, and in its message.

A private function, setting_from_name, now takes the setting's name,
its two values, and its as_str. It returns the value whose as_str is
the name, and otherwise refuses the name with the message, which it
builds from the setting and the values' as_str names. Each from_name is
one call of it, so as_str is the one place that spells a setting's
names, as Guest::from_name and LayerRole::from_name already find their
values by their names.

Behavior is unchanged: the same names give the same values, and any
other name fails with ErrorKind::Unsupported and the same message, byte
for byte, since each from_name passes its values in the order in which
its message listed them. The public signatures and documentation of
the three methods stay as they were.

The share tests check the owner's and the mode's messages, and the
launch tests the kind's. The golden sandbox tests parse the owner and
the kind of each share that they replay with these methods, and the
CLI's lifecycle scenarios with one, several, caller-owned, policy, and
file shares read recorded shares through all three.

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

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 small refactor preserves accepted names, error kinds, and messages, with no unresolved issues.

No open issues 🎉

What changed in this PR

Consolidates sandbox mount-setting parsing in nvx-rs, removing duplicated lookups and error formatting.

Changes:

  • Adds a private helper shared by MountAccess, MountOwner, and MountKind.
  • Uses as_str for accepted names and error choices, preserving public signatures and behavior.
File Description
nvx-rs/​src/​host/​sandbox/​mount.rs Replaces three parsing implementations with one shared helper.

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