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

Expose WASI symlink helper #68574

Closed
RReverser opened this issue Jan 27, 2020 · 9 comments · Fixed by #81542
Closed

Expose WASI symlink helper #68574

RReverser opened this issue Jan 27, 2020 · 9 comments · Fixed by #81542
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@RReverser
Copy link
Contributor

Currently, Unix and Windows expose std::os::unix::symlink and std::os::windows::fs::symlink_file correspondingly, which both simply accept source and destination by Path.

However, the currently exposed WASI method - std::os::wasi::fs::symlink is lower-level and requires user to have a file descriptor of the preopened directory in addition to the file paths:

pub fn symlink<P: AsRef<Path>, U: AsRef<Path>>(
old_path: P,
fd: &File,
new_path: U,
) -> io::Result<()> {
fd.as_inner()
.fd()
.symlink(osstr2str(old_path.as_ref().as_ref())?, osstr2str(new_path.as_ref().as_ref())?)
}

This is different from all other fs methods and not ideal, because it requires calling even more lower-level methods to find and retrieve that preopened directory by file path first, and libpreopen isn't exposed to users, so they have to do this all manually.

Moreover, the codebase already contains a higher-level helper for symlinking files by path and hiding these syscall details, it's just not currently exposed to users:

pub fn symlink(src: &Path, dst: &Path) -> io::Result<()> {
let (dst, dst_file) = open_parent(dst)?;
dst.symlink(osstr2str(src.as_ref())?, osstr2str(dst_file.as_ref())?)
}

If this is not too late in terms of backwards compatibility, it would be better to remove the currently exposed low-level syscall and expose this helper instead, so that users could work on paths more easily like they do on other platforms.

Alternatively, it's necessary to at least expose helpers for retrieving the preopen directory from a file path, as otherwise it's not possible to construct symlinks from userland without invoking raw syscalls.

@jonas-schievink jonas-schievink added C-feature-request Category: A feature request, i.e: not implemented / a PR. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 27, 2020
@RReverser
Copy link
Contributor Author

I'm sorry, but since it's been half a year... friendly ping? @alexcrichton do you think we can expose that symlink helper?

@alexcrichton
Copy link
Member

I'm not really sure what this is proposing myself, so I'm not sure? What is a "symlink helper"?

@RReverser
Copy link
Contributor Author

RReverser commented Jul 6, 2020

The second function referenced above - basically a way to create a symlink with just source and dest paths, like you can on other platforms with std::os::unix::fs::symlink or std::os::windows::symlink_file.

Right now the exposedstd::os::wasi::fs::symlink variant of that function requires not only paths, but also knowing a file descriptor of the preopened dir, making it impossible to use from safe code (since preopened dirs are managed internally by WASI libc).

@alexcrichton
Copy link
Member

I would personally think that a better option would be to expose open_parent in one way or another for wasi since that seems more appropriate for a sys-module-functionality.

@RReverser
Copy link
Contributor Author

I totally agree it makes sense for sys module in general, but it seems to be more of an implementation detail in this case.

The std::os::(somename)::fs often provide OS-specific extensions to regular fs methods, not necessarily low-level ones. Symlink, in particular, seems like something that should work as straightforward as, say, File::open, std::fs::copy or any other universal APIs.

That is, I think all those OS-specific implementations should still follow the deprecated universal API from older Rust times - std::fs::soft_link - with same ease of use, just under different namespaces.

@alexcrichton
Copy link
Member

I'm not exactly the gatekeeper of the wasm module or anything, I'm basically just saying that I would not personally pursue adding this method to the standard library. If you'd like to do so you can certainly do that, however.

@RReverser
Copy link
Contributor Author

I've mostly asked you because you authored that helper function that I'd love to see exposed.

I think it's useful for ease of use and consistency with other platforms. I'm happy to make a PR myself if it would be accepted.

@RReverser
Copy link
Contributor Author

Looks like people are already misusing the currently exposed API for symlinks - since there is no way to use it right :( vitiral/path_abs#50 (comment)

@RReverser
Copy link
Contributor Author

Given above, finally got back to this and made a PR to fix it: #81542

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 3, 2021
Expose correct symlink API on WASI

As described in rust-lang#68574, the currently exposed API for symlinks is, in fact, a thin wrapper around the corresponding syscall, and not suitable for public usage.

The reason is that the 2nd param in the call is expected to be a handle of a "preopened directory" (a WASI concept for exposing dirs), and the only way to retrieve such handle right now is by tinkering with a private `__wasilibc_find_relpath` API, which is an implementation detail and definitely not something we want users to call directly.

Making matters worse, the semantics of this param aren't obvious from its name (`fd`), and easy to misinterpret, resulting in people trying to pass a handle of the target file itself (as in vitiral/path_abs#50), which doesn't work as expected.

I did a [codesearch among open-source repos](https://sourcegraph.com/search?q=std%3A%3Aos%3A%3Awasi%3A%3Afs%3A%3Asymlink&patternType=literal), and the usage above is so far the only usage of this API at all, but we should fix it before more people start using it incorrectly.

While this is technically a breaking API change, I believe it's a justified one, as 1) it's OS-specific and 2) there was strictly no way to correctly use the previous form of the API, and if someone does use it, they're likely doing it wrong like in the example above.

The new API does not lead to the same confusion, as it mirrors `std::os::unix::fs::symlink` and `std::os::windows::fs::symlink_{file,dir}` variants by accepting source/target paths.

Fixes rust-lang#68574.

r? `@alexcrichton`
m-ou-se added a commit to m-ou-se/rust that referenced this issue Feb 5, 2021
Expose correct symlink API on WASI

As described in rust-lang#68574, the currently exposed API for symlinks is, in fact, a thin wrapper around the corresponding syscall, and not suitable for public usage.

The reason is that the 2nd param in the call is expected to be a handle of a "preopened directory" (a WASI concept for exposing dirs), and the only way to retrieve such handle right now is by tinkering with a private `__wasilibc_find_relpath` API, which is an implementation detail and definitely not something we want users to call directly.

Making matters worse, the semantics of this param aren't obvious from its name (`fd`), and easy to misinterpret, resulting in people trying to pass a handle of the target file itself (as in vitiral/path_abs#50), which doesn't work as expected.

I did a [codesearch among open-source repos](https://sourcegraph.com/search?q=std%3A%3Aos%3A%3Awasi%3A%3Afs%3A%3Asymlink&patternType=literal), and the usage above is so far the only usage of this API at all, but we should fix it before more people start using it incorrectly.

While this is technically a breaking API change, I believe it's a justified one, as 1) it's OS-specific and 2) there was strictly no way to correctly use the previous form of the API, and if someone does use it, they're likely doing it wrong like in the example above.

The new API does not lead to the same confusion, as it mirrors `std::os::unix::fs::symlink` and `std::os::windows::fs::symlink_{file,dir}` variants by accepting source/target paths.

Fixes rust-lang#68574.

r? `@alexcrichton`
m-ou-se added a commit to m-ou-se/rust that referenced this issue Feb 5, 2021
Expose correct symlink API on WASI

As described in rust-lang#68574, the currently exposed API for symlinks is, in fact, a thin wrapper around the corresponding syscall, and not suitable for public usage.

The reason is that the 2nd param in the call is expected to be a handle of a "preopened directory" (a WASI concept for exposing dirs), and the only way to retrieve such handle right now is by tinkering with a private `__wasilibc_find_relpath` API, which is an implementation detail and definitely not something we want users to call directly.

Making matters worse, the semantics of this param aren't obvious from its name (`fd`), and easy to misinterpret, resulting in people trying to pass a handle of the target file itself (as in vitiral/path_abs#50), which doesn't work as expected.

I did a [codesearch among open-source repos](https://sourcegraph.com/search?q=std%3A%3Aos%3A%3Awasi%3A%3Afs%3A%3Asymlink&patternType=literal), and the usage above is so far the only usage of this API at all, but we should fix it before more people start using it incorrectly.

While this is technically a breaking API change, I believe it's a justified one, as 1) it's OS-specific and 2) there was strictly no way to correctly use the previous form of the API, and if someone does use it, they're likely doing it wrong like in the example above.

The new API does not lead to the same confusion, as it mirrors `std::os::unix::fs::symlink` and `std::os::windows::fs::symlink_{file,dir}` variants by accepting source/target paths.

Fixes rust-lang#68574.

r? ``@alexcrichton``
@bors bors closed this as completed in 5882cce Feb 5, 2021
RReverser added a commit to RReverser/path_abs that referenced this issue Feb 19, 2021
As described in vitiral#51, current implementation is incorrect and won't work and, as described in rust-lang/rust#68574, there was no correct way to use that API.

I went ahead and added a new API for symlinks in rust-lang/rust#81542, and now that it's landed, it can be used to correctly create symlinks.

Fixes vitiral#51.
vitiral pushed a commit to vitiral/path_abs that referenced this issue Feb 22, 2021
As described in #51, current implementation is incorrect and won't work and, as described in rust-lang/rust#68574, there was no correct way to use that API.

I went ahead and added a new API for symlinks in rust-lang/rust#81542, and now that it's landed, it can be used to correctly create symlinks.

Fixes #51.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants