Stream: git-wasmtime

Topic: wasmtime / PR #4967 Allow WASI to open directories withou...

Wasmtime GitHub notifications bot (Sep 27 2022 at 01:34):

sunfishcode opened PR #4967 from sunfishcode/open-dir-or-not to main:

O_DIRECTORY says that a directory is required, but O_DIRECTORY is not required for opening directories. To implement this on top of the current APIs, use open_dir to try to open a directory first, and then fall back to trying to open it as a file if that fails.

In the future, we may be able to simplify this code even more, using [maybe_dir], which is needed to allow Windows to be able to open a directory, though it needs bytecodealliance/cap-std#277 for Windows.

The testcase here is the testcase from #4947.

[maybe_dir]: https://docs.rs/cap-fs-ext/latest/cap_fs_ext/struct.OpenOptions.html#method.maybe_dir`

<!--

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 (Sep 27 2022 at 16:56):

sunfishcode updated PR #4967 from sunfishcode/open-dir-or-not to main.

Wasmtime GitHub notifications bot (Sep 27 2022 at 21:09):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Sep 27 2022 at 21:09):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Sep 27 2022 at 21:09):

rvolosatovs created PR review comment:

It looks like the error handling in this crate can benefit from quite some refactoring as this does not look like a maintainable way to match on errors.

Note, that this gets much worse for external users of this crate providing e.g. multiple WasiFile or WasiDir implementations, since users must return wasi-common::Error constructed internally from an ErrorKind in custom implementations or a few other special cases, e.g. from https://github.com/bytecodealliance/wasmtime/blob/29c7de73401347af6d8c7111bdb7a11f1b04c3b7/crates/wasi-common/src/snapshots/preview_1.rs#L48-L64, anything else results in a trap and terminates execution.
Every assertion on the error kind now requires a downcast and this runtime reflection step can fail if e.g. the wasi-common crate changes the set of specific error types returned or downcasted internally on update.

So perhaps these utilities should actually be public to expose them to users who may need to match on the error kind from outside this crate?

Wasmtime GitHub notifications bot (Sep 27 2022 at 21:13):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Sep 27 2022 at 22:23):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Sep 27 2022 at 22:23):

sunfishcode created PR review comment:

I agree that the current code is not great. It sounds like another way to approach this would be to get rid of the anyhow and downcasting altogether and just use an enum. I may be up for doing the work for this refactoring, but in this PR I'd like to focus on just fixing the underlying bug first, and we can figure out refactoring separately.

Wasmtime GitHub notifications bot (Sep 27 2022 at 22:24):

sunfishcode updated PR #4967 from sunfishcode/open-dir-or-not to main.

Wasmtime GitHub notifications bot (Sep 27 2022 at 22:32):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Sep 27 2022 at 22:32):

rvolosatovs created PR review comment:

Yes, I agree that does not belong in this PR :)
Another option could be an associated type Error: AsRef<ErrorKind> or type Error: AsRef<Errno>, whichever is the most applicable.

Wasmtime GitHub notifications bot (Sep 28 2022 at 10:59):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Sep 28 2022 at 10:59):

rvolosatovs submitted PR review.

Wasmtime GitHub notifications bot (Sep 28 2022 at 10:59):

rvolosatovs created PR review comment:

(nit, I suppose?) I think this if statement is way too complex and hard to read, which is exactly why I introduced https://github.com/rvolosatovs/wasmtime/blob/5ffb7f8990d1562bf9a1b6b29e3faadc934d024f/crates/wasi-common/src/snapshots/preview_1.rs#L895-L898 in #4947

Wasmtime GitHub notifications bot (Sep 28 2022 at 10:59):

rvolosatovs created PR review comment:

this is very difficult to read and comprehend especially due to the nesting

Wasmtime GitHub notifications bot (Sep 28 2022 at 10:59):

rvolosatovs created PR review comment:

You mentioned a race condition in https://github.com/bytecodealliance/wasmtime/pull/4947/files#r980506965 as reasoning for this PR, where the underlying filetype stored under a path could change between the calls, but doesn't this suffer from exact same race condition, where:

1. initial open_dir fails on path A with ENOTDIR
2. in the meantime, A is replaced by a directory
3. open_file fails, because A is now a directory
   ?

Wasmtime GitHub notifications bot (Sep 29 2022 at 22:45):

sunfishcode updated PR #4967 from sunfishcode/open-dir-or-not to main.

Wasmtime GitHub notifications bot (Sep 30 2022 at 00:39):

sunfishcode created PR review comment:

That's a good point. I need to look at this more closely.

Wasmtime GitHub notifications bot (Sep 30 2022 at 00:39):

sunfishcode submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2022 at 13:26):

haraldh created PR review comment:

        let non_dir_oflags = oflags.intersects(OFlags::CREATE | OFlags::EXCLUSIVE | OFlags::TRUNCATE);

Wasmtime GitHub notifications bot (Oct 06 2022 at 13:26):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2022 at 13:26):

haraldh submitted PR review.

Wasmtime GitHub notifications bot (Oct 06 2022 at 13:26):

haraldh created PR review comment:

        let write = file_caps
            .intersects(FileCaps::WRITE | FileCaps::ALLOCATE | FileCaps::FILESTAT_SET_SIZE);

Wasmtime GitHub notifications bot (Jan 23 2023 at 18:48):

sunfishcode updated PR #4967 from sunfishcode/open-dir-or-not to main.

Wasmtime GitHub notifications bot (Jan 23 2023 at 20:26):

sunfishcode updated PR #4967 from sunfishcode/open-dir-or-not to main.

Wasmtime GitHub notifications bot (Apr 21 2023 at 17:33):

pchickey closed without merge PR #4967.

Last updated: Nov 22 2024 at 16:03 UTC