Conversation
e55628f to
465ffaf
Compare
| /// - the segment MUST NOT contain a slash `/` character | ||
| /// - the segment MUST NOT be empty | ||
| fn new(s: &str, limit: &VfsLimits) -> Result<Self, LimitExceeded> { | ||
| pub(crate) fn new(s: &str, limit: &VfsLimits) -> Result<Self, LimitExceeded> { |
There was a problem hiding this comment.
If I read the changes correctly, you only need this method exposed for tests. I wonder if we could do one of these two things instead:
- just call
PathTraversal::parse(s, limit).unwrap().into_iter().next().unwrap()in this one place in the test - leave the
newfunction private and and a new function just for tests, like:#[cfg(test)] pub(crate) fn new_test(s) -> Self { Self::new(s, &Default::default()).unwrap() }
My reasoning is that it would be nice if we keep the API exposure tight since it usually simplifies reasoning about the code and allows easier refactorings.
| if self.closed { | ||
| return Err(StreamError::Closed); | ||
| } | ||
| Ok(usize::MAX) |
There was a problem hiding this comment.
I had to read up what this method actually does:
Returns the number of bytes that are ready to be written to this stream.
Maybe leave a code comment here stating your reasoning:
| Ok(usize::MAX) | |
| // Since we don't have a buffer or I/O, we can basically accept an arbitrary number of bytes. | |
| Ok(usize::MAX) |
| /// Current write offset. | ||
| offset: u64, |
There was a problem hiding this comment.
I wonder if you have an easier time if this is a usize since you need a good amount of back-and-forth conversions in fn write, but maybe that wouldn't help. Just a thing to consider.
| let old_size = content.len(); | ||
| if end > old_size { | ||
| if let Err(e) = self.limiter.grow(end - old_size) { | ||
| println!("let guest handle space error: {:?}", e); |
| .ok_or_else(|| StreamError::trap("offset overflow"))?; | ||
| let end = usize::try_from(end_u64).map_err(|_| StreamError::trap("offset overflow"))?; | ||
|
|
||
| match &mut self.node.write().unwrap().kind { |
There was a problem hiding this comment.
This code looks very similar to the block in async fn write below. I wonder if that could be moved to a common, shared function or method on VfsNodeKind?
| .grow(std::mem::size_of_val(&new_file)) | ||
| .map_err(|_| FsError::trap(ErrorCode::InsufficientMemory))?; | ||
|
|
||
| match &mut parent_node.write().unwrap().kind { |
There was a problem hiding this comment.
That's something I've missed when reviewing create_directory_at: You should check the VFS node kind first and in the match branch bump the allocator/limiter data. Because if someone will call this method on the wrong VFS node kind, you're basically "leaking" resources (I mean not really, but the guest cannot get them back).
| let mut guard = desc.node.write().unwrap(); | ||
| match &mut guard.kind { | ||
| VfsNodeKind::File { content } => { | ||
| content.clear(); |
There was a problem hiding this comment.
So truncate doesn't free any memory? That's a bit unexpected. I would probably call *content = Vec::with_capacity(0); here and then use the limiter to return the resources.
| } | ||
| } | ||
| } else { | ||
| let desc = maybe_desc?; |
There was a problem hiding this comment.
I was wondering what want_exclusive would do in this case, i.e. when want_create is not set.
| let desc = maybe_desc?; | |
| // NOTE: `wants_create` is `false`. We ignore `wants_exclusive` here. | |
| // | |
| // WASI just refers to POSIX: | |
| // https://github.com/WebAssembly/WASI/blob/184b0c0e9fd437e5e5601d6e327a28feddbbd7f7/proposals/filesystem/wit/types.wit#L149-L150 | |
| // | |
| // and POSIX just says it's "undefined": | |
| // https://pubs.opengroup.org/onlinepubs/9799919799/functions/open.html | |
| let desc = maybe_desc?; |
| let wants_truncate = open_flags.contains(OpenFlags::TRUNCATE); | ||
|
|
||
| let node = if wants_create { | ||
| match maybe_desc { |
There was a problem hiding this comment.
| match maybe_desc { | |
| if want_directory { | |
| // `wants_create` only works for files, NOT for directories, see: | |
| // https://github.com/WebAssembly/WASI/blob/184b0c0e9fd437e5e5601d6e327a28feddbbd7f7/proposals/filesystem/wit/types.wit#L145-L146 | |
| return Err(...); | |
| } | |
| match maybe_desc { |
Closes #337 & #336
Describe your proposed changes here.