Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add permissions to wasi_config_preopen_dir C API #9477

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

ajalt
Copy link
Contributor

@ajalt ajalt commented Oct 16, 2024

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 bytecodealliance/wasmtime-py#251

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

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 update the docs for those functions to reflect the current signature.

This is a prerequisite for bytecodealliance/wasmtime-py#251
@ajalt ajalt requested a review from a team as a code owner October 16, 2024 21:18
@ajalt ajalt requested review from fitzgen and removed request for a team October 16, 2024 21:18
@github-actions github-actions bot added wasi Issues pertaining to WASI wasmtime:c-api Issues pertaining to the C API. labels Oct 16, 2024
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@alexcrichton
Copy link
Member

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.

Comment on lines 191 to 198
dir_perms
.and_then(|dir_perms| {
file_perms.map(|file_perms| {
config
.builder
.preopened_dir(host_path, guest_path, dir_perms, file_perms)
})
})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤦. Sorry for the churn.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries!

@alexcrichton alexcrichton added this pull request to the merge queue Oct 22, 2024
Merged via the queue into bytecodealliance:main with commit d6ce1dd Oct 22, 2024
41 checks passed
@ajalt ajalt deleted the wasi-preopen-dir-perms branch October 22, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants