haraldh edited PR #5326 from table_immutable
to main
:
<!--
Please ensure that the following steps are all taken care of before submitting
the PR.
[ ] This has been discussed in issue #..., or if not, please tell us why
here.[ ] A short description of what this does, why it is needed; if the
description becomes long, the matter should probably be discussed in an issue
first.[ ] This PR contains test cases, if meaningful.
- [ ] A reviewer from the core maintainer team has been assigned for this PR.
If you don't know who could review this, please indicate so. The list of
suggested reviewers on the right can help you.Please ensure all communication adheres to the code of conduct.
-->
haraldh updated PR #5326 from table_immutable
to main
.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
Race condition here. Another call to drop_caps_to could have dropped different permissions between the capable_of_{dir,file} call and this write, which would result in capabilities being added back despite the other drop_caps_to returning a success,
haraldh submitted PR review.
haraldh created PR review comment:
Thank for spotting this!
haraldh updated PR #5326 from table_immutable
to main
.
haraldh edited PR review comment.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
This should be all one atomic operation which can be protected by one lock, there's no need to use both atomics and locks here.
alexcrichton created PR review comment:
I don't think there's any need to serialize multiple threads like this, so the randomness provider should move to a trait with
&self
rather than&mut self
alexcrichton created PR review comment:
This should probably be an atomic operation rather than two seaparate ones.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
How come these need changing? The prior signatures I think should work?
alexcrichton created PR review comment:
I think ideally this is avoided because a system file doesn't need an extra mutex around it since it should already be able to perform all necessary operations concurrently.
alexcrichton created PR review comment:
Wiggle is used for more than just WASI, and I don't think it's suitable to make this change. Providing
&mut
access shows how Wasmtime gives this ability even if WASI doesn't take advantage of it.Instead the traits should still give access to
&mut self
butWasiCtx
would internally holdArc<Table>
or something like that which is where the requirement for locking comes in (because&mut Arc<T>
still only gives you&T
)
alexcrichton created PR review comment:
Could this perhaps be a method on
DirFdStat
?
abrown submitted PR review.
abrown created PR review comment:
IIRC, @sunfishcode had mentioned to me that this block of functions, being written solely to "set up the context" in a sort of builder pattern, could be re-written entirely if the locking made the current design inconvenient. I mention that because I would have expected methods like
set_*
to be&mut self
. I see why you did this though, just mentioning an alternative.
haraldh submitted PR review.
haraldh created PR review comment:
In the method a
RwLockWriteGuard<BorrowedFd>
would be produced, which can't be dereferenced for return.
haraldh submitted PR review.
haraldh created PR review comment:
Yeah, sure!
haraldh created PR review comment:
Well, it's only
cap_std::fs::File::set_fd_flags(&mut self, set_fd_flags: SetFdFlags<Self>) -> io::Result<()>
which needs it.~~~
haraldh submitted PR review.
haraldh edited PR review comment.
haraldh submitted PR review.
haraldh created PR review comment:
Maybe, we should start removing the
mut
there.
haraldh created PR review comment:
Arc<T>
sounds like a plan. Thanks!
haraldh submitted PR review.
haraldh submitted PR review.
haraldh created PR review comment:
You are right! Thanks!
haraldh submitted PR review.
haraldh created PR review comment:
Yeah, sure, but it doesn't need the
mut
now anymore with the table having interior mutability
haraldh created PR review comment:
yes, thanks
haraldh submitted PR review.
haraldh created PR review comment:
What do you think about:
impl wasi_snapshot_preview1::WasiSnapshotPreview1 for Arc<WasiCtx>
haraldh submitted PR review.
haraldh submitted PR review.
haraldh created PR review comment:
seems to be in crate
system-interface
...trait GetSetFdFlags
haraldh submitted PR review.
haraldh created PR review comment:
because of windows... woah... wildcard impl for all T restricted per method.
#[cfg(windows)] impl<T> GetSetFdFlags for T { /// … fn set_fd_flags(&mut self, set_fd_flags: SetFdFlags<Self>) -> io::Result<()> where Self: AsFilelike, { *self = set_fd_flags.reopened; Ok(()) }
haraldh submitted PR review.
haraldh created PR review comment:
alexcrichton submitted PR review.
alexcrichton created PR review comment:
I would personally prefer to put the
Arc
inside of theWasiCtx
to make it more ergonomic to use but either way achieves the same goal.
alexcrichton created PR review comment:
Sorry let me restate. From a system API perspective a lock is not needed here, so I don't know where the rwlock is motivated from. I would expect that these signatures remain the same since this is intended for bottoming out in the underlying platform-specific interface which handles all synchronization internally in the kernel.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Maybe @sunfishcode could help here, I'm not familiar enough with any of the methods/interfaces here to know what to do off-hand. At a base level though I don't at least currently understand why an
RwLock
or other form of synchronization is required for I/O operations on system files.
haraldh created PR review comment:
It's only because of
File::set_fd_flags()
needing &mut self
haraldh submitted PR review.
haraldh updated PR #5326 from table_immutable
to main
.
haraldh submitted PR review.
haraldh created PR review comment:
deferred this to a later PR
haraldh submitted PR review.
haraldh created PR review comment:
deferred this for a later PR
haraldh submitted PR review.
haraldh created PR review comment:
done
haraldh requested alexcrichton for a review on PR #5326.
haraldh requested bjorn3 for a review on PR #5326.
haraldh requested alexcrichton for a review on PR #5326.
Last updated: Dec 23 2024 at 12:05 UTC