Conversation
Codecov Report
@@ Coverage Diff @@
## dev #1351 +/- ##
==========================================
+ Coverage 56.43% 56.52% +0.09%
==========================================
Files 141 141
Lines 20084 20098 +14
Branches 4977 4983 +6
==========================================
+ Hits 11335 11361 +26
+ Misses 5868 5839 -29
- Partials 2881 2898 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| pub fn slice<R: std::ops::RangeBounds<usize>>(&self, range: R) -> TypedPluginPtr<T> { | ||
| use std::ops::Bound; | ||
| let excluded_end = match range.end_bound() { | ||
| Bound::Included(e) => e + 1, |
There was a problem hiding this comment.
This can technically overflow, but shouldn't in practice. Maybe use checked_add().unwrap() here?
There was a problem hiding this comment.
IIUC overflow will be caught in Debug builds; given that, I don't think it's worth wrapping every arithmetic operation in checks. rust-lang/rfcs#560
| let excluded_end = match range.end_bound() { | ||
| Bound::Included(e) => e + 1, | ||
| Bound::Excluded(e) => *e, | ||
| Bound::Unbounded => self.count, |
There was a problem hiding this comment.
Should this be self.count + 1 so that it doesn't exclude the last byte?
There was a problem hiding this comment.
I don't think so - we're getting the excluded end-bound. e.g. buf[..buf.len()] is all of buf.
src/main/host/memory_manager.rs
Outdated
| // Low level helper for reading directly from `srcs` to `dsts`. | ||
| // Returns the number of bytes read. Panics if the | ||
| // MemoryManager's process isn't currently active. | ||
| fn readv_iovecs<T: Pod + Debug>( |
There was a problem hiding this comment.
This generic T isn't used anywhere.
There was a problem hiding this comment.
Done (I'm surprised Rust didn't complain)
src/main/host/memory_manager.rs
Outdated
| .memory_manager | ||
| .readv_ptrs(&mut [buf], &[self.ptr.slice(offset..)])?; | ||
| if nread != buf.len() { | ||
| Err(Errno::EFAULT) |
There was a problem hiding this comment.
A debug log here might be useful if we ever need to debug this.
There was a problem hiding this comment.
Done (in helpers)
src/main/host/memory_manager.rs
Outdated
| .memory_manager | ||
| .readv_ptrs(&mut [&mut vec], &[self.ptr])?; | ||
| if nread != vec.len() { | ||
| return Err(Errno::EFAULT); |
There was a problem hiding this comment.
A debug log here might be useful if we ever need to debug this.
There was a problem hiding this comment.
Done (now in memory_manager.read_exact)
| /// Get a reference to the readable prefix of the region. | ||
| pub fn ref_some(&self) -> Result<&[T], Errno> { | ||
| (&*self.reader).ref_some() | ||
| } | ||
|
|
||
| /// Get a reference to the whole region, or return an error. | ||
| pub fn ref_exact(&self) -> Result<&[T], Errno> { | ||
| (&*self.reader).ref_exact() | ||
| } |
There was a problem hiding this comment.
Can the (&*self.reader) be just self.reader?
src/main/host/memory_manager.rs
Outdated
| // Split at page boundaries to allow partial reads. | ||
| let mut slices = Vec::with_capacity((buf.len() + page_size() - 1) / page_size()); | ||
| let mut toread = buf.len(); | ||
| let mut buf = buf; | ||
| while toread > 0 { | ||
| let mut this_toread = toread % page_size(); | ||
| if this_toread == 0 { | ||
| this_toread = page_size(); | ||
| } | ||
| let (prefix, suffix) = buf.split_at_mut(this_toread); | ||
| buf = suffix; | ||
| slices.push(prefix); | ||
| toread -= this_toread; | ||
| } | ||
| let rv = self | ||
| .memory_manager | ||
| .readv_ptrs(&mut slices, &[self.ptr.slice(offset..)]); | ||
| rv |
There was a problem hiding this comment.
- This seems like it's mixing counts (
buf.len()) with bytes (page_size()). Should this be usingbuf.len() * sizeof(T)? - This seems to be reading in chunks of
page_size(), but the chunks don't seem to be aligned at page boundaries ifself.ptritself isn't aligned to a page boundary.
There was a problem hiding this comment.
This seems like it's mixing counts (buf.len()) with bytes (page_size()). Should this be using buf.len() * sizeof(T)?
Oops; you're right. I'd originally implemented only for u8 intead of for T, but missed the corresponding changes here. (I couldn't figure out a way to define methods in a trait only when T=u8)
I changed the MemoryManager helpers to only take u8 and put the conversions here.
This seems to be reading in chunks of page_size(), but the chunks don't seem to be aligned at page boundaries if self.ptr itself isn't aligned to a page boundary.
... wow yeah I totally botched that. Thanks for catching it!
| fn read_some(&self, offset: usize, buf: &mut [T]) -> Result<usize, Errno> { | ||
| self.memory_manager | ||
| .readv_ptrs(&mut [buf], &[self.ptr.slice(offset..)]) | ||
| } |
There was a problem hiding this comment.
In order to stay consistent with the other functions, I think read_some() should return the number of T items, not the number of bytes returned by readv_ptrs(). So it should return self.memory_manager.readv_ptrs() / size_of(T).
| let reader = &*reader; | ||
| let dst = std::slice::from_raw_parts_mut(str as *mut u8, strlen); |
There was a problem hiding this comment.
I think it would be good to assert that str here isn't null.
There was a problem hiding this comment.
I started to add debug_asserts, but I think we can depend on Rust to do it for us. e.g. https://doc.rust-lang.org/src/core/slice/raw.rs.html#130
There was a problem hiding this comment.
I don't think debug asserts in the standard library run, for example this code doesn't panic: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=9fbb2471cca367c2624d2a04767e37e8
There was a problem hiding this comment.
Oh wow, that's surprising. I'll go ahead and add it then. I see I also missed a few other comments somehow; will make another pass
|
I realized that currently if the caller calls I think what I'd like to do is move the public methods returning references into Thank you for your patience and diligence reviewing all these iterations! |
Oh no, that's right I did fix that. But the way I fixed it is that we always go through the more complex |
I don't think I follow these comments, but I'll wait to see what the next change looks like :) |
* Copy file names instead of holding a reference when we also need to get a mutable reference. Holding an immutable + mutable reference simultaneously would break Rust's soundness contract, and currently results in a run-time panic in the C wrappers. * Fix missing NULL checks for return values of process_getReadablePtr and process_getWriteablePtr. * Remove unnecessary NULL checks for user pointers. The MemoryManager functions will themselves return NULL if the user pointer isn't accessible.
Fixes #1332