Add permissions to wasi_config_preopen_dir C API#9477
Conversation
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
github-actions
Bot
added
wasi
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
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.
|
Oh sorry, forgot to mention, but:
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. |
| dir_perms | ||
| .and_then(|dir_perms| { | ||
| file_perms.map(|file_perms| { | ||
| config | ||
| .builder | ||
| .preopened_dir(host_path, guest_path, dir_perms, file_perms) | ||
| }) | ||
| }) |
There was a problem hiding this 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.
There was a problem hiding this 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
There was a problem hiding this 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.
There was a problem hiding this 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?
There was a problem hiding this 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 🤦. Sorry for the churn.
2 participants
The current
wasi_config_preopen_dirfunction does not expose thedir_permsandfile_permsparameters that were added topreopened_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.