Repository navigation
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
Open
Pedro Henrique Penna (ppenna) wants to merge 1 commit into
Pedro Henrique Penna (ppenna) wants to merge 1 commit into
Conversation
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>
Contributor
There was a problem hiding this comment.
🟢 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, andMountKind. - Uses
as_strfor 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
This was referenced Oct 21, 2026
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
MountAccess,MountOwner, andMountKind, the settings of a sandbox share innvx-rs/src/host/sandbox/mount.rs, each had afrom_namethat matched its two names as string literals and refused any other name with a message of one shape, which listed the names again:So each setting spelled its names three times: in
as_str, in the match offrom_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 whoseas_strisname, and otherwise refusesnamewith the message, which it builds from the setting and the values'as_strnames. Eachfrom_nameis one call of it, soas_stris the one place that spells a setting's names, asGuest::from_nameandLayerRole::from_namealready 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 insrcandtests, 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 todevchanged when the run started, among themREADME.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, anddoc/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.rswas last changed by #634, in its containment checks, not in these methods. The candidates in files that nonvx-rs-quality/pull request has changed yet, such as the steps that the NVXS session roles'receiveandsendrepeat 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_nameis private and takes the setting's values andas_stras arguments, rather than being a trait of the three public types. The threefrom_namemethods keep their signatures and documentation, and no public item changes.FIRST or SECONDform 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--mountvalue, whose message differs: reusingMountAccess::from_namethere would build an error only to replace it. Follow-ups lists 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 twovalueswhoseas_strisname, and otherwise fails withrefused, which isErrorKind::Unsupported, andunsupported sandbox mount {setting} {repr(name)}; choose {first} or {second}, with the values' names in their order.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_nameaccepts the names that its match accepted,roandrw,vmmandcaller, anddirectoryandfile, which are the names thatas_strgives the same values, and compares them exactly, as the match did. Any other name fails with the samerefusederror,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 inunsupported 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_callerinhost/sandbox/mount/tests.rs, which checks the owner's and the mode's messages and theirErrorKind;launch_contract_attaches_a_file_as_an_aggregate_childinhost/sandbox/tests.rs, which checks the kind's message;mounts_lower_to_the_arguments_that_python_gives,launches_give_the_arguments_and_command_lines_that_python_gives, andsandbox_option_helpers_match_nvx_pyinhost/sandbox/golden.rs, which parse the owners and kinds of the shares that they replay with these methods;tests/cli.rswith one, several, caller-owned, policy, and file shares, in its POSIX and Windows sections, whosestartandexecread 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-rsCLI built from TREE, but no default scenario reads a share's settings by name: the scenarios that drivenvx.py sandboxwith shares,managed-exec-configandsandbox-lifecycle, run only when named, andaci_edge_sandboxesdoes not usehost::sandbox::mount.Follow-ups
Not in this pull request, to keep it to one change. I found no suspected bug.
parse_mount_valueinhost/sandbox/mount.rsmatchesroandrwitself for a--mountvalue, with a message of its own; it could takeMountAccess::from_nameand replace its error.host/cli/sandbox/mod.rslists the owners inMOUNT_OWNERSand matchescallerfor--mount-owner; both could come fromMountOwner::as_str, but nvx-rs: separate distro verification from launch planning #653 changes that file.Guest::from_nameinhost/artifacts.rsand the role check ofSandboxLayer::parse_ininhost/sandbox/layer.rsrefuse names withunsupported ...; choose A, Bmessages 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, andon_protocolinhost/network/mod.rseach build and push aPortSelector.guest/managed_agent/helper.rsandexec_config.rsdefine the samefile_withtest helper, and their tests write EXEC configuration headers byte by byte.protocol/nvxs/guest.rsandhost.rs:sendrepeats four lines after its state check in both roles, besides the lines ofreceivethat nvx-rs: send the guest's attachment, ACK, and CREDIT with one method #684 lists.Validation
BASE:
66159235cd673a401c308a04d1233e0110474a68,origin/devwhen the run started. Before this pull request was opened,origin/devadvanced toc30b75afeby 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.pyand its tests, documentation, anddata/, none of this pull request's files, and no open pull request todevchangesnvx-rs/src/host/sandbox/mount.rs.TREE:
90363a43674659465315ae9a067716cc82683197, the tree of this commit. Every host applied the patch to BASE and checked thatgit write-treeequals 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 inbuild/openvmm.provenance.json.nvx-rs:downloadinstalled the release'snvx-rs, which BASE's sources built, so each host rebuilt it from TREE with the Cargo command with whichnvx.pybuilds it. Cargo recompilednvx, the executable's digest changed, andrecord-nvx-rs-provenancerecorded it; each host checked that the record names TREE's sources (source_sha2569a20b0d89b25f60b91b9d9833ffed8ab385e41ba61be56687412b574a0d28436) and the executable. Each host then built theopenvmm_e2eandmanaged_lifecycletest 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;build-initramfswithNVX_GUEST_BINARIES.initramfs.cpio.gz(SHA-2562d12a797f23e00d7363ba9290f7efdb86b0b401227a380f6bc13c8e58f690f4b), 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 warningson all targets with all features and on the library without default features; andcargo testwith 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 ofnvx,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; nothing was installed on them.
test-aci-edge-sandboxestest-microvm, default suitenvxtests: 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 85aci_edge_sandboxestests over the two feature sets; agent bundling (4). The macOS checks and the guest-only MSRV check ran locallynvxtests: 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 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, where no signal was ignored (
SigIgn: 0), so the CLI's Ctrl-C tests ran as intended. Each suite'smanaged-lifecyclescenario ran themanaged_lifecycletest with Cargo, which passed. OpenVMM warned of a host CPU-profile fallback only in thecpu-profile-fallbackscenario, 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, 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;test-microvmscenarios that run only when named:smp-lapic, andmanaged-exec-configandsandbox-lifecycle, which need the Ubuntu scratch template that packages omit and drivenvx.py sandboxwith shares;verify-guest-determinism;