Deprecate `try_access()` #349

roypat · 2025-09-26T16:02:56Z

Summary of the PR

Following up on some discussions in #339, attempt to figure out how feasible it is to replace try_access() with get_slices() + functions in Rust's iterator trait.

cc @XanClic
cc @bonzini

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

All commits in this PR have Signed-Off-By trailers (with
git commit -s), and the commit message has max 60 characters for the
summary and max 75 characters for each description line.
All added/changed functionality has a corresponding unit/integration
test.
All added/changed public-facing functionality has entries in the "Upcoming
Release" section of CHANGELOG.md (if no such section exists, please create one).
Any newly added unsafe code is properly documented.

XanClic

Nice! You make it look easy.

Typo note on the first commit: Should be “range” instead of “rnage” in the title.

src/guest_memory.rs

roypat · 2025-10-01T10:32:48Z

Changes v1 -> v2:

Add an sanity assert to get_slices() so that we uphold the doc statement about cumulative slice length even in the face of incorrect GuestMemoryRegion implementations (this simplifies the iterator chain in check_range())
Rename .partial() to .stop_on_error()
Change partial()/stop_on_error() to terminate iteration on all errors after at least one slice has been produced successfully (and if not even one slice can be returned, return an error from stop_on_error() instead).

src/guest_memory.rs

bonzini · 2025-10-01T10:55:00Z

@roypat sorry I messed up using the suggestion feature :/ if you squash my misguided commit, I'll approve.

roypat · 2025-10-01T11:02:08Z

@roypat sorry I messed up using the suggestion feature :/ if you squash my misguided commit, I'll approve.

haha, no worries! squashed (and fixed a typo I noticed, "when if" -> "when it") :)

bonzini · 2025-10-01T11:17:07Z

thread 'mmap::tests::read_to_and_write_from_mem' (1574) panicked at src/mmap/mod.rs:482:18:
called `Result::unwrap()` on an `Err` value: PartialBuffer { expected: 4, completed: 2 }

No idea why that would fail only under Miri... I cannot even reproduce it locally.

roypat · 2025-10-01T12:02:04Z

thread 'mmap::tests::read_to_and_write_from_mem' (1574) panicked at src/mmap/mod.rs:482:18:
called `Result::unwrap()` on an `Err` value: PartialBuffer { expected: 4, completed: 2 }
No idea why that would fail only under Miri... I cannot even reproduce it locally.

Argh, that must be the same thing as 029c652. Let me add a commit marking this one as cfg(not(miri)) as well

Switching the check_range() implementation from try_access() to get_slices() will mean that the test requires a working implementation of get_slice(), which MockRegion does not have. So instead have the test use the actual memory regions defined in the mmap module. Signed-off-by: Patrick Roy <[email protected]>

Declare `len` as type `usize`, which reduce the number of casts between usize and u64 (the variable is most often uses as a usize, and only once needs to be u64). Signed-off-by: Patrick Roy <[email protected]>

Panic in get_slices() if the call to .get_slice() returns a slice that does not match the `len` parameter passed to it. If this happens there is an application bug somewhere in the GuestMemoryRegion implementation, and the library code in vm-memory, while not doing anything unsound, will probably react unpredictably to this situation. This way, the final paragraph in the get_slices() documentatoin about cumulative lengths is actually true. Signed-off-by: Patrick Roy <[email protected]>

Deprecate 'GuestMemory::try_access()` in favor of the external iterator provided by `GuestMemory::get_slices()`. Rewrite all internal usages of `try_access()` in terms of `get_slices()`. Signed-off-by: Patrick Roy <[email protected]>

Miri sometimes non-deterministically has its mocks for read(2)/write(2) do only partial reads/writes, which our unittests cannot deal with. See also commit 029c652 ("fix(test): do not try to read files under miri"). Mark these test cases as cfg(not(miri)). Signed-off-by: Patrick Roy <[email protected]>

XanClic · 2025-10-01T13:52:32Z

Looks good to me, thanks!

(Is it common to use ntoskrnl.exe as a known-content file under Windows 😅?)

XanClic

Ah, this is how I need to do it.

roypat · 2025-10-01T14:13:30Z

(Is it common to use ntoskrnl.exe as a known-content file under Windows 😅?)

I have no idea, I haven't touched a windows system since high school haha

bonzini · 2025-10-01T14:26:45Z

@roypat any objections to inviting Hanna to the repo so that her reviews count as approving?

roypat · 2025-10-01T15:17:52Z

@roypat any objections to inviting Hanna to the repo so that her reviews count as approving?

No concern whatsoever :)

@XanClic wanna open a PR to add yourself to CODEOWNERS and then I can get github and crates.io permissions set up?

XanClic · 2025-10-01T15:57:29Z

@roypat any objections to inviting Hanna to the repo so that her reviews count as approving?

No concern whatsoever :)

@XanClic wanna open a PR to add yourself to CODEOWNERS and then I can get github and crates.io permissions set up?

Sure! Thanks to @stefano-garzarella it’s not as pressing anymore, but I can take on some reviewing anyway.

As discussed on PR rust-vmm#349, adding myself to the CODEOWNERS file to help with reviews. Signed-off-by: Hanna Czenczek <[email protected]>

As discussed on PR #349, adding myself to the CODEOWNERS file to help with reviews. Signed-off-by: Hanna Czenczek <[email protected]>

roypat requested review from alexandruag, bonzini, jiangliu and ShadowCurse as code owners September 26, 2025 16:02

roypat force-pushed the deprecate-try-access branch 3 times, most recently from 92a0e25 to 59e3811 Compare September 27, 2025 07:04

XanClic reviewed Sep 29, 2025

View reviewed changes

src/guest_memory.rs Outdated Show resolved Hide resolved

src/guest_memory.rs Show resolved Hide resolved

roypat force-pushed the deprecate-try-access branch 2 times, most recently from a9ecfa4 to e70475c Compare October 1, 2025 10:30

roypat requested a review from XanClic October 1, 2025 10:34

bonzini reviewed Oct 1, 2025

View reviewed changes

src/guest_memory.rs Outdated Show resolved Hide resolved

roypat force-pushed the deprecate-try-access branch from dd33c70 to db00f74 Compare October 1, 2025 11:00

roypat added 4 commits October 1, 2025 13:04

refactor: change casts in GuestMemorySliceIter::do_next

63eb648

Declare `len` as type `usize`, which reduce the number of casts between usize and u64 (the variable is most often uses as a usize, and only once needs to be u64). Signed-off-by: Patrick Roy <[email protected]>

deprecate GuestMemory::try_access()

a9aa4de

Deprecate 'GuestMemory::try_access()` in favor of the external iterator provided by `GuestMemory::get_slices()`. Rewrite all internal usages of `try_access()` in terms of `get_slices()`. Signed-off-by: Patrick Roy <[email protected]>

roypat force-pushed the deprecate-try-access branch 2 times, most recently from d4469c1 to 8b0d3c1 Compare October 1, 2025 12:05

roypat force-pushed the deprecate-try-access branch from 8b0d3c1 to 9c8fb09 Compare October 1, 2025 13:39

Merge branch 'main' into deprecate-try-access

c2fbc6c

XanClic approved these changes Oct 1, 2025

View reviewed changes

roypat linked an issue Oct 1, 2025 that may be closed by this pull request

Test GuestMemory::try_access error cases #272

Closed

bonzini approved these changes Oct 1, 2025

View reviewed changes

roypat changed the title ~~[RFC] Deprecate try_access()~~ Deprecate try_access() Oct 1, 2025

stefano-garzarella approved these changes Oct 1, 2025

View reviewed changes

roypat merged commit f059f7e into rust-vmm:main Oct 1, 2025
2 checks passed

XanClic added a commit to XanClic/vm-memory that referenced this pull request Oct 1, 2025

Update CODEOWNERS

e382e47

As discussed on PR rust-vmm#349, adding myself to the CODEOWNERS file to help with reviews. Signed-off-by: Hanna Czenczek <[email protected]>

XanClic mentioned this pull request Oct 1, 2025

Update CODEOWNERS #353

Merged

4 tasks

bonzini pushed a commit that referenced this pull request Oct 1, 2025

Update CODEOWNERS

b8401e6

As discussed on PR #349, adding myself to the CODEOWNERS file to help with reviews. Signed-off-by: Hanna Czenczek <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Deprecate `try_access()` #349

Deprecate `try_access()` #349

Uh oh!

roypat commented Sep 26, 2025

Uh oh!

XanClic left a comment

Uh oh!

Uh oh!

Uh oh!

roypat commented Oct 1, 2025

Uh oh!

Uh oh!

bonzini commented Oct 1, 2025

Uh oh!

roypat commented Oct 1, 2025

Uh oh!

bonzini commented Oct 1, 2025 •

edited

Loading

Uh oh!

roypat commented Oct 1, 2025

Uh oh!

XanClic commented Oct 1, 2025

Uh oh!

XanClic left a comment

Uh oh!

roypat commented Oct 1, 2025

Uh oh!

bonzini commented Oct 1, 2025

Uh oh!

roypat commented Oct 1, 2025

Uh oh!

Uh oh!

XanClic commented Oct 1, 2025

Uh oh!

Uh oh!

Deprecate try_access() #349

Deprecate try_access() #349

Uh oh!

Conversation

roypat commented Sep 26, 2025

Summary of the PR

Requirements

Uh oh!

XanClic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

roypat commented Oct 1, 2025

Uh oh!

Uh oh!

bonzini commented Oct 1, 2025

Uh oh!

roypat commented Oct 1, 2025

Uh oh!

bonzini commented Oct 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roypat commented Oct 1, 2025

Uh oh!

XanClic commented Oct 1, 2025

Uh oh!

XanClic left a comment

Choose a reason for hiding this comment

Uh oh!

roypat commented Oct 1, 2025

Uh oh!

bonzini commented Oct 1, 2025

Uh oh!

roypat commented Oct 1, 2025

Uh oh!

Uh oh!

XanClic commented Oct 1, 2025

Uh oh!

Uh oh!

Deprecate `try_access()` #349

Deprecate `try_access()` #349

bonzini commented Oct 1, 2025 •

edited

Loading