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

Suggestion: no-op set_permissions for WASI #68560

Closed
RReverser opened this issue Jan 27, 2020 · 3 comments · Fixed by #69106
Closed

Suggestion: no-op set_permissions for WASI #68560

RReverser opened this issue Jan 27, 2020 · 3 comments · Fixed by #69106

Comments

@RReverser
Copy link
Contributor

Currently, WASI target implements retrieving permissions with a no-op and just returning a default value:

pub fn perm(&self) -> FilePermissions {
// not currently implemented in wasi yet
FilePermissions { readonly: false }
}

However, the corresponding setter method is instead implemented by returning an error:

pub fn set_permissions(&self, _perm: FilePermissions) -> io::Result<()> {
// Permissions haven't been fully figured out in wasi yet, so this is
// likely temporary
unsupported()
}

This is unfortunate and breaks higher-level APIs like std::fs::copy which currently manages to successfully copy contents of file on a WASI target but then fails because the default implementation unconditionally calls set_permissions:

let ret = io::copy(&mut reader, &mut writer)?;
fs::set_permissions(to, perm)?;
Ok(ret)

To both fix such issues and from a semantic point of view, too, it seems it would be better to mirror the behaviour of the getter (permissions method) and just perform no-op in set_permissions on a WASI target, at least until WASI specification decides to extend support and add real permissions model.

Would the team be open to this?

@RReverser
Copy link
Contributor Author

Ping @alexcrichton @fitzgen?

@alexcrichton
Copy link
Member

I would personally prefer to ensure that wasi-libc for C stays in sync with Rust's libstd, so I think that this is probably best a question for the wasi group as opposed to Rust's libstd specifically.

@RReverser
Copy link
Contributor Author

RReverser commented Feb 12, 2020

I see; WASI SDK conditionally excludes chmod for WASI target during compilation, and I suppose that's what Rust wants to avoid and instead simulates with a runtime error. Fair enough.

In that case, would you be open to adding a specific implementation of std::fs::copy for WASI target that just uses std::io::copy but doesn't set permissions? There is no direct analogue for it in the wasi-libc anyway, and it would solve fix specific API function not working at all, without changing the rest.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 15, 2020
Fix std::fs::copy on WASI target

Previously `std::fs::copy` on wasm32-wasi would reuse code from the `sys_common` module and would successfully copy contents of the file just to fail right before closing it.

This was happening because `sys_common::copy` tries to copy permissions of the file, but permissions are not a thing in WASI (at least yet) and `set_permissions` is implemented as an unconditional runtime error.

This change instead adds a custom working implementation of `std::fs::copy` (like Rust already has on some other targets) that doesn't try to call `set_permissions` and is essentially a thin wrapper around `std::io::copy`.

Fixes rust-lang#68560.
@bors bors closed this as completed in 8fb8bb4 Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants