Stream: git-wasmtime

Topic: wasmtime / PR #5326 WIP: immutable WasiCtx

Wasmtime GitHub notifications bot (Nov 25 2022 at 16:07):

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.
-->

Wasmtime GitHub notifications bot (Nov 25 2022 at 16:08):

haraldh updated PR #5326 from table_immutable to main.

Wasmtime GitHub notifications bot (Nov 25 2022 at 16:54):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Nov 25 2022 at 16:54):

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,

Wasmtime GitHub notifications bot (Nov 28 2022 at 08:58):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 28 2022 at 08:58):

haraldh created PR review comment:

Thank for spotting this!

Wasmtime GitHub notifications bot (Nov 28 2022 at 09:30):

haraldh updated PR #5326 from table_immutable to main.

Wasmtime GitHub notifications bot (Nov 28 2022 at 09:31):

haraldh edited PR review comment.

Wasmtime GitHub notifications bot (Nov 28 2022 at 15:37):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 28 2022 at 15:37):

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.

Wasmtime GitHub notifications bot (Nov 28 2022 at 15:37):

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

Wasmtime GitHub notifications bot (Nov 28 2022 at 15:37):

alexcrichton created PR review comment:

This should probably be an atomic operation rather than two seaparate ones.

Wasmtime GitHub notifications bot (Nov 28 2022 at 15:37):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 28 2022 at 15:37):

alexcrichton created PR review comment:

How come these need changing? The prior signatures I think should work?

Wasmtime GitHub notifications bot (Nov 28 2022 at 15:37):

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.

Wasmtime GitHub notifications bot (Nov 28 2022 at 15:37):

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 but WasiCtx would internally hold Arc<Table> or something like that which is where the requirement for locking comes in (because &mut Arc<T> still only gives you &T)

Wasmtime GitHub notifications bot (Nov 28 2022 at 15:37):

alexcrichton created PR review comment:

Could this perhaps be a method on DirFdStat?

Wasmtime GitHub notifications bot (Nov 29 2022 at 01:01):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 01:01):

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.

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:00):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:00):

haraldh created PR review comment:

In the method a RwLockWriteGuard<BorrowedFd> would be produced, which can't be dereferenced for return.

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:01):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:01):

haraldh created PR review comment:

Yeah, sure!

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:05):

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.

~~~

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:05):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:05):

haraldh edited PR review comment.

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:06):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:06):

haraldh created PR review comment:

Maybe, we should start removing the mut there.

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:07):

haraldh created PR review comment:

Arc<T> sounds like a plan. Thanks!

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:07):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:14):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:14):

haraldh created PR review comment:

You are right! Thanks!

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:27):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 08:27):

haraldh created PR review comment:

Yeah, sure, but it doesn't need the mut now anymore with the table having interior mutability

Wasmtime GitHub notifications bot (Nov 29 2022 at 10:14):

haraldh created PR review comment:

yes, thanks

Wasmtime GitHub notifications bot (Nov 29 2022 at 10:14):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 11:44):

haraldh created PR review comment:

What do you think about:

impl wasi_snapshot_preview1::WasiSnapshotPreview1 for Arc<WasiCtx>

Wasmtime GitHub notifications bot (Nov 29 2022 at 11:44):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 13:10):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 13:10):

haraldh created PR review comment:

seems to be in crate system-interface ... trait GetSetFdFlags

Wasmtime GitHub notifications bot (Nov 29 2022 at 13:13):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 13:13):

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(())
    }

Wasmtime GitHub notifications bot (Nov 29 2022 at 13:35):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 13:35):

haraldh created PR review comment:

https://github.com/bytecodealliance/system-interface/blob/da238e324e752033f315f09c082ad9ce35d42696/src/fs/fd_flags.rs#L210-L217

Wasmtime GitHub notifications bot (Nov 29 2022 at 15:27):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 15:27):

alexcrichton created PR review comment:

I would personally prefer to put the Arc inside of the WasiCtx to make it more ergonomic to use but either way achieves the same goal.

Wasmtime GitHub notifications bot (Nov 29 2022 at 15:29):

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.

Wasmtime GitHub notifications bot (Nov 29 2022 at 15:29):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 15:30):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 29 2022 at 15:30):

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.

Wasmtime GitHub notifications bot (Nov 29 2022 at 15:53):

haraldh created PR review comment:

It's only because of File::set_fd_flags() needing &mut self

Wasmtime GitHub notifications bot (Nov 29 2022 at 15:53):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2022 at 05:13):

haraldh updated PR #5326 from table_immutable to main.

Wasmtime GitHub notifications bot (Nov 30 2022 at 05:18):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2022 at 05:18):

haraldh created PR review comment:

deferred this to a later PR

Wasmtime GitHub notifications bot (Nov 30 2022 at 05:18):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2022 at 05:18):

haraldh created PR review comment:

deferred this for a later PR

Wasmtime GitHub notifications bot (Nov 30 2022 at 05:18):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2022 at 05:18):

haraldh created PR review comment:

done

Wasmtime GitHub notifications bot (Nov 30 2022 at 05:18):

haraldh requested alexcrichton for a review on PR #5326.

Wasmtime GitHub notifications bot (Nov 30 2022 at 05:18):

haraldh requested bjorn3 for a review on PR #5326.

Wasmtime GitHub notifications bot (Nov 30 2022 at 05:18):

haraldh requested alexcrichton for a review on PR #5326.

Last updated: Nov 22 2024 at 16:03 UTC