Stream: git-wasmtime

Topic: wasmtime / PR #9477 Add permissions to `wasi_config_preop...

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 21:18):

ajalt requested fitzgen for a review on PR #9477.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 21:18):

ajalt requested wasmtime-core-reviewers for a review on PR #9477.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 21:18):

ajalt opened PR #9477 from ajalt:wasi-preopen-dir-perms to bytecodealliance:main:

The current wasi_config_preopen_dir function does not expose the dir_perms and file_perms parameters that were added to preopened_dir. This commit adds them and updates the docs for those functions to reflect the current signature.

This is a prerequisite for https://github.com/bytecodealliance/wasmtime-py/issues/251

This change isn't backwards compatible, so please let me know if there's anything I need to do differently.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:50):

alexcrichton submitted PR review:

Thanks for this! Could you also add some #define entries for the various flags here? Perhaps a flag-per-bit and a *_ALL define which has everything OR'd together?

Also I think it might be best to check for unknown bits in the method itself. For example using from_bits and translating the optional return value into an error being returned.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 22:51):

alexcrichton commented on PR #9477:

Oh sorry, forgot to mention, but:

This change isn't backwards compatible, so please let me know if there's anything I need to do differently.

That's ok, so no worries! We don't provide an ironclad guarantee of stability in the C API at this time, so it's ok to change it.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 23:27):

ajalt updated PR #9477.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 23:30):

ajalt created PR review comment:

I didn't see any other #defines for enums that I could use for a naming convention, so let me know if these should be moved or renamed.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 23:30):

ajalt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 23:32):

ajalt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 23:32):

ajalt created PR review comment:

This feels clunky, but I'm not a rust expert so if you know of a more idiomatic way of handling the error cases, I'd be happy to clean this up.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 23:46):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 23:46):

alexcrichton created PR review comment:

Looks good to me!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 16 2024 at 23:46):

alexcrichton created PR review comment:

Oh for this I might recommend:

let dir_perms = wasmtime_wasi::DirPerms::from_bits(dir_perms)
    .ok_or_else(|| anyhow!("invalid directory permissions"))?;

That way you can avoid the nested closures

view this post on Zulip Wasmtime GitHub notifications bot (Oct 17 2024 at 16:08):

ajalt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 17 2024 at 16:08):

ajalt created PR review comment:

That's certainly cleaner! But wasi_config_preopen_dir returns bool rather than a Result, so it doesn't appear that you can use the ? operator. Is there a way to do an unwrap / early return like that in a function that returns bool?
Or would it be better to change the function signature? Returning strings from a C API would require callers to free the returned result, but we could potentially use a numerical error code instead.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2024 at 09:36):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2024 at 09:36):

alexcrichton created PR review comment:

Ah excellent point! I forgot this returns bool, so sorry about that.

For that I'd recommend a match-per-translation-of-config to return false early and that way it doesn't have to nest.

I do like the idea of changing the function signature too. It's overdue for the WASI APIs here. In doing so though it might be best to rename and shuffle things around. For example the original intention of wasi.h was to become a standard API like the wasm-c-api proposal (which hasn't finished being standardized yet). In lieu of that nowadays it's probably best to rename things to wasmtime_wasi_*, move the header to wasmtime/wasi.h, and update idioms to match the rest of the C API (e.g. wasmtime_error_t* instead of bool). That's a bit of a larger project though so how about we keep the bool for now and defer this refactoring for later?

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2024 at 16:08):

ajalt updated PR #9477.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2024 at 16:10):

ajalt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 21 2024 at 16:10):

ajalt created PR review comment:

Thanks, that's much cleaner. That's also the pattern already being used for the other parameters of the function, which I somehow didn't recognize :face_palm:. Sorry for the churn.

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 14:50):

alexcrichton submitted PR review:

No worries!

view this post on Zulip Wasmtime GitHub notifications bot (Oct 22 2024 at 15:06):

alexcrichton merged PR #9477.

Last updated: Dec 23 2024 at 12:05 UTC