Cleanup of deprecated functions + Behavioral Clarifications #307
Conversation
roypat
requested review from
alexandruag,
bonzini and
jiangliu
as code owners
roypat
force-pushed
the
deprecation-cleanup
branch
from
1155812 to
514b5b2
Compare
|
With this change, the next release should be |
roypat
force-pushed
the
deprecation-cleanup
branch
from
8d0f62f to
559c3c6
Compare
This fixes a cargo warning printed in recent toolchains. Since we do not support toolchains older than 1.38, there is no need to symlink to the new file to the old one. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
force-pushed
the
deprecation-cleanup
branch
from
559c3c6 to
becfd0e
Compare
Yup! |
| fn get_slice(&self, offset: usize, count: usize) -> Result<VolatileSlice<B>> { | ||
| let _ = self.compute_end_offset(offset, count)?; | ||
| Ok( | ||
| // SAFETY: This is safe because the pointer is range-checked by compute_end_offset, and | ||
| // the lifetime is the same as self. | ||
| unsafe { | ||
| VolatileSlice::with_bitmap( | ||
| self.addr.add(offset), | ||
| count, | ||
| self.bitmap.slice_at(offset), | ||
| self.mmap, | ||
| ) | ||
| }, | ||
| ) |
There was a problem hiding this comment.
If that's the case, would it be better to use a macro to expand code here instead of calling one another 🤔
There was a problem hiding this comment.
I'd prefer keeping the duplicated code over a macro if we think that we don't want to have another layer of indirection here (which is a fair point, the indirection in this crate are already fairly complex as-is). But the compiler should be clever enough to figure out that it can inline the call to subslice, so no actual subroutine call should happen.
| self.try_access(count, addr, |offset, len, caddr, region| -> Result<usize> { | ||
| // Check if something bad happened before doing unsafe things. | ||
| assert!(offset <= count); | ||
|
|
There was a problem hiding this comment.
How about completely remove the offset parameter, and change try_access accordingly.
In my view the only place might need the assertion is before the first execution of f(total, len as usize, start, region) in try_access, but since total defaults to 0 and len is an usize, the assertion there is pointless. The rest cases are well guarded in Ok(len) match arm inside try_access.
There was a problem hiding this comment.
The offset parameter is used in the read and write implementations in Bytes<GuestAddress> for T: GuestMemory :/
Well, technically, we cannot rely on the actual behavior of try_access, since it's a trait function, and some arbitrary implementation of GuestMemory could overwrite it with some absolute nonsense. The main point for me here is that even in that case, it will not cause unsoundness, because there's no unsafe code here, and all bounds are properly checked further down the stack.
There was a problem hiding this comment.
The
offsetparameter is used in thereadandwriteimplementations inBytes<GuestAddress> for T: GuestMemory:/
Oh I didn't read that far, let's just remove the assertion here then 😉
roypat
force-pushed
the
deprecation-cleanup
branch
from
777d39e to
7be0764
Compare
The call to `.subslice()` ensures that the range `[0, total]` is a valid
subslice of the `buf: VolatileSlice` passed in. But `total =
min(buf.len(), self.len()) <= buf.len()`, and thus `[0, total]` is
definitely a valid subslice of `buf`.
The `copy_{from,to}_volatile_slice` functions do not actually care about
the length of the `VolatileSlice` passed in - it relies on the safety
invariant to ensure that the passed slice has length at least `total`.
Thus it doesn't matter if we pass a slice of length `total`, or of a
length greater than `total`. It will simply access the first `total`
bytes in the slice.
Also clarify the safety comment, as some slightly mistakes seemingly
snuck in when copying them.
Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
force-pushed
the
deprecation-cleanup
branch
from
7be0764 to
8fd12ef
Compare
| ) | ||
| }, | ||
| ) | ||
| self.subslice(offset, count) |
There was a problem hiding this comment.
How about we deprecate subslice and remove it in a further release?
There was a problem hiding this comment.
Mhh, we could. But tbh, slice.get_slice(start, count) reads a bit weirdly (and conversely, so does region.subslice(start, count)) :/
| /// assert_eq!(slice.truncate(3).len(), 3); | ||
| /// assert_eq!(slice.truncate(10).len(), 5); | ||
| /// ``` | ||
| pub fn truncate(&self, count: usize) -> VolatileSlice<'a, B> { |
There was a problem hiding this comment.
Truncate somehow implies (to me) that we're updating self because of how truncate on Vec works: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.truncate.
Shouldn't we be able to obtain the same result by calling subslice with offset = 0 & count as the size we want to "truncate" to?
There was a problem hiding this comment.
Argh, true. Naming is hard. Maybe .truncated() instead (e.g. with an extra 'd')?
Shouldn't we be able to obtain the same result by calling subslice with offset = 0 & count as the size we want to "truncate" to?
It'd need to be count = min(size we want to be, length). I didn't like it because subslice does a million bounds checks that are all unnecessary for these arguments. But I should probably actually benchmark it/see if the compiler manages to optimize it away
EDIT: Ah yeah, and because truncate doesn't return a Result, which makes it a bit nicer to use imo.
There was a problem hiding this comment.
Damn, criterion indeed says there's no difference between them. Guess rustc is better than I thought at optimizing xP
So my only argument is "it avoids .unwrap()-ing something that we know can never fail"
Fix the doc comment, and add a unit test for this functions. Particularly the "allow length 0 accesses at the end of the slice" behavior was undocumented before. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
The comment on the assertion was wrong, as we are not even doing anything unsafe in the first place. Additionally, the `offset` variable is unused by these functions, so the assertion is at best a sanity check that the `try_access` implementation is correct, although I don't particularly see the value of that. Remove the assertion to prevent confusion. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
The two functions contain exactly the same body, so just have one call the other. No functional change intended. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Add a macro that automatically retries a given I/O operation if EINTR is returned. This is a fairly common pattern in vm-memory. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
This function reduces the length of a volatile slice. Many of the I/O primitives rely on volatile slice lengths to determine how many bytes should be read/written, and in cases where we want to read/write "up to X bytes", calling `.truncate` is needed. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
The old `Read`/`Write` based APIs for operating on guest memory have been deprecated for a fairly long time now, and are superseceded by their `ReadVolatile`/`WriteVolatile` equivalents. The `fold` and friends functions have been deprecated for way longer. Various `.as_ptr` APIs in volatile_memory.rs are deprecated in favor of pointer guards. Let's clean up the crate and remove all of these. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
The `read_[exact_]_volatile_from` and `write_[all_]volatile_to` functions were intended to be replacements for their `Read` and `Write` based counterparts. However, those used to live in the `Bytes` trait, not the `GuestMemory` trait. Fix up this goof on my part by moving them to the `Bytes` trait. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Capture the actual behavior of various edge cases around empty buffers and containers in the doc comment. Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
Signed-off-by: Patrick Roy <roypat@amazon.co.uk>
roypat
force-pushed
the
deprecation-cleanup
branch
from
8fd12ef to
e4f291d
Compare
Summary of the PR
This PR aim to clean up the API surface of the vm-memory crate in a way that is as unintrusive as possible. It does this in two ways
Additionally, it fixes up a goof I made when I introduced the
ReadVolatileandWriteVolatiletraits in #247, where I put theread_volatile_fromand friends funcions into the wrong trait. When it comes to how to implement these traits forGuestMemoryMmapandVolatileSlice, I've deferred to how the oldReadandWritebased functions were implemented for them.I've compiled the vm-virtio workspace and linux-loader against this. All tests pass in both, only linux-loader needs a tiny change to import the
Bytestrait now.If one squints at the code a bit, one might ask whether all the
read/read_slicefunctions in theBytestrait could be default functions, implemented in terms of the volatile functions (since&mut [u8]implements the volatile access traits). I played around with this, but there are subtle functional and performance changes involved in that (although it would be a great simplification), which is why I'm holding off on doing anything there until a later PR.Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafecode is properly documented.