Repository navigation
refactor(guest): replace external chown with syscalls - #1210
Conversation
📦 BoxLite review — couldn't completepowered by BoxLite |
📝 WalkthroughWalkthroughChangesThe PR replaces external recursive ownership repair with descriptor-based traversal. It adds fault-injection and privileged tests for filesystem edge cases, plus a Linux Make target and CI wiring to run them. Guest ownership repair
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI as Linux CI runner
participant Make as make test:guest-perms
participant Cargo as Cargo
participant Tests as Guest permission tests
participant Root as Root or passwordless sudo
CI->>Make: invoke guest permission tests
Make->>Cargo: build tests and emit JSON metadata
Cargo-->>Make: test executable path
Make->>Tests: run unprivileged tests
Make->>Root: run ignored privileged tests in private mount namespace
Root->>Tests: execute privileged tests
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
make/test.mk (2)
252-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConstrain the jq selection to the bin test artifact.
The filter matches any
compiler-artifactwhosetarget.nameisboxlite-guestand whoseprofile.testis true. A library or integration-test target that shares the package name also matches, andlastthen selects the wrong executable. Add atarget.kindcheck so the selection stays deterministic.🔧 Proposed fix
if ! test_binary=$$(jq --slurp --raw-output --exit-status \ - '[.[] | select(.reason == "compiler-artifact" and .profile.test == true and .target.name == "boxlite-guest" and .executable != null) | .executable] | last' \ + '[.[] | select(.reason == "compiler-artifact" and .profile.test == true and .target.name == "boxlite-guest" and (.target.kind | index("bin")) and .executable != null) | .executable] | last' \ "$$test_metadata"); then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@make/test.mk` around lines 252 - 257, Update the jq filter used to assign test_binary so it also requires the artifact target kind to be the bin kind, while preserving the existing compiler-artifact, test-profile, boxlite-guest name, and executable checks. Keep the final selection deterministic by continuing to select the last matching executable.
262-274: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: skip the privileged stage instead of failing on developer machines.
The recipe exits 1 when the caller is neither root nor able to run passwordless
sudo. CI satisfies that condition, so the strict check helps there. On a local checkout the whole target fails after the unprivileged tests already passed. Consider a variable such asGUEST_PERMS_REQUIRE_PRIVILEGED=1that CI sets, and print a skip notice otherwise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@make/test.mk` around lines 262 - 274, Update the privileged ownership test stage around the test_binary invocation to support a GUEST_PERMS_REQUIRE_PRIVILEGED=1 configuration. Continue running the privileged tests for root or passwordless sudo users; when unavailable, skip with a clear notice by default, but retain the existing failure behavior when the variable is enabled for CI.src/guest/src/storage/perms/tests.rs (1)
62-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: do not panic inside
MountGuard::drop.If an assertion fails first, the
expectindroppanics during unwinding. The process then aborts and the original assertion message is lost. Log the unmount error instead, so the real failure stays visible.♻️ Proposed refactor
impl Drop for MountGuard { fn drop(&mut self) { - umount2(&self.0, MntFlags::MNT_DETACH).expect("unmount test bind mount"); + if let Err(error) = umount2(&self.0, MntFlags::MNT_DETACH) { + eprintln!("failed to unmount test bind mount {}: {error}", self.0.display()); + } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/guest/src/storage/perms/tests.rs` around lines 62 - 68, Update MountGuard::drop to handle umount2 errors without panicking: replace the expect-based failure path with logging of the unmount error, while leaving successful cleanup unchanged so an earlier assertion failure remains visible.src/guest/src/storage/perms.rs (1)
166-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: move the fault injection out of the traversal loop.
The loop repeats four
#[cfg(test)]/#[cfg(not(test))]pairs for the read, open, stat, and stream stages. This duplicates the production call in each pair, so a future change can update one arm only. Small private helpers such asread_next(&mut self, frame),open_child(...),stat_child(...), andopen_stream(...)keep the injection inside one place and keepchown_pathlinear.Also use
std::os::fd::RawFdinstead ofi32for theparent_fdparameter ofchown_entry(Line 333) to match the rest of the file.Also applies to: 333-344
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/guest/src/storage/perms.rs` around lines 166 - 190, Refactor the traversal in chown_path so test fault injection for read, open, stat, and stream stages is encapsulated in small private helpers, leaving the loop with a single production-style call for each stage; preserve the existing injected failures and behavior. Also update chown_entry’s parent_fd parameter from i32 to std::os::fd::RawFd to match the file’s descriptor types.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/guest/src/storage/perms/tests.rs`:
- Around line 226-230: Update the temporary-directory setup in the affected test
to derive its parent from Cargo’s configured target directory rather than
assuming ../../target, create that directory before calling tempdir_in, and add
a comment explaining that a target-local directory is used to keep Unix socket
paths within length limits.
---
Nitpick comments:
In `@make/test.mk`:
- Around line 252-257: Update the jq filter used to assign test_binary so it
also requires the artifact target kind to be the bin kind, while preserving the
existing compiler-artifact, test-profile, boxlite-guest name, and executable
checks. Keep the final selection deterministic by continuing to select the last
matching executable.
- Around line 262-274: Update the privileged ownership test stage around the
test_binary invocation to support a GUEST_PERMS_REQUIRE_PRIVILEGED=1
configuration. Continue running the privileged tests for root or passwordless
sudo users; when unavailable, skip with a clear notice by default, but retain
the existing failure behavior when the variable is enabled for CI.
In `@src/guest/src/storage/perms.rs`:
- Around line 166-190: Refactor the traversal in chown_path so test fault
injection for read, open, stat, and stream stages is encapsulated in small
private helpers, leaving the loop with a single production-style call for each
stage; preserve the existing injected failures and behavior. Also update
chown_entry’s parent_fd parameter from i32 to std::os::fd::RawFd to match the
file’s descriptor types.
In `@src/guest/src/storage/perms/tests.rs`:
- Around line 62-68: Update MountGuard::drop to handle umount2 errors without
panicking: replace the expect-based failure path with logging of the unmount
error, while leaving successful cleanup unchanged so an earlier assertion
failure remains visible.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b8ea997-9027-4e0a-a511-ecaafef08acb
📒 Files selected for processing (6)
.github/workflows/test.ymlmake/help.mkmake/test.mksrc/guest/Cargo.tomlsrc/guest/src/storage/perms.rssrc/guest/src/storage/perms/tests.rs
There was a problem hiding this comment.
Pull request overview
This PR refactors guest filesystem ownership repair to remove the PATH-resolved chown -R subprocess and replace it with an in-process, fd-relative syscall-based walker, along with new tests and CI wiring to cover privileged cases.
Changes:
- Replace
Command::new("chown")-based recursive ownership repair with an iterative, fd-relative traversal usingopenat/fstatat/fchownat/fchown. - Add a dedicated guest perms test suite (including root-only tests) and a
make test:guest-permstarget that runs privileged tests inside a private mount namespace. - Update Linux CI to install dependencies and run the new guest ownership tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/guest/src/storage/perms.rs | Swaps external chown -R for an in-process recursive chown implementation and reporting. |
| src/guest/src/storage/perms/tests.rs | Adds focused unprivileged + privileged coverage for recursive chown behavior and edge cases. |
| src/guest/Cargo.toml | Enables the needed nix feature(s) for directory iteration. |
| make/test.mk | Introduces test:guest-perms target, including privileged execution via unshare. |
| make/help.mk | Documents the new make test:guest-perms target. |
| .github/workflows/test.yml | Installs required Linux deps and runs the new guest ownership test target in CI. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Summary
Replace the guest's PATH-resolved
chown -Rsubprocess with an in-process, fd-relative syscall implementation while preserving the existing ownership sampling and best-effort repair behavior.Call graph
Before
mount (BlockDeviceMount · src/guest/src/storage/block_device.rs:25) — mounts and initializes the guest filesystem
└─ fix_if_needed (OwnershipFixer · src/guest/src/storage/perms.rs:17) — samples ownership before repair
├─ ownership_matches (OwnershipFixer · src/guest/src/storage/perms.rs:74) — skips an already-matching tree
└─ Command::new("chown") (std::process::Command · src/guest/src/storage/perms.rs:43) — runs PATH-resolved
chown -RAfter
mount (BlockDeviceMount · src/guest/src/storage/block_device.rs:25) — mounts and initializes the guest filesystem
└─ fix_if_needed (OwnershipFixer · src/guest/src/storage/perms.rs:23) — preserves the existing ownership sample gate
├─ ownership_matches (OwnershipFixer · src/guest/src/storage/perms.rs:82) — keeps the original root-plus-five sampling policy
└─ chown_path (RecursiveChowner · src/guest/src/storage/perms.rs:134) — walks the tree iteratively with fd-relative syscalls
├─ chown_operand (RecursiveChowner · src/guest/src/storage/perms.rs:316) — changes a non-directory root without following symlinks
├─ chown_entry (RecursiveChowner · src/guest/src/storage/perms.rs:329) — changes descendant entries with
fchownat└─ chown_directory (RecursiveChowner · src/guest/src/storage/perms.rs:342) — changes directories post-order with
fchownChanges
chownbinary, PATH lookup, and reaper-fence dependency from guest ownership repair.How to verify
make test:unit:guestmake test:guest-permsmake fmt:check:rustmake clippygit diff --checkRisks / rollout
chown; privileged Linux tests cover recursive repair, read-only mounts, cross-mount traversal, and bind-mount cycles.Summary by CodeRabbit
Bug Fixes
Tests