Pure-Rust implementation of WASI's open_parent

[newpavlov]

__wasilibc_find_relpath is not needed for interoperability with C/C++, so we can replace it with a pure-Rust code. I am not sure if I understood everything correctly, so I hope @sunfishcode will review this PR. For example I think we can remove the rights argument, since error should happen later either way and there is no reason to check access rights in open_parent.

r? @alexcrichton

[rust-highfive]

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-09-13T16:11:20.8133714Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-13T16:11:20.8307150Z ##[command]git config gc.auto 0
2019-09-13T16:11:20.8371219Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-13T16:11:20.8428452Z ##[command]git config --get-all http.proxy
2019-09-13T16:11:20.8549875Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64434/merge:refs/remotes/pull/64434/merge
---
2019-09-13T16:17:26.8574758Z     Checking hashbrown v0.5.0
2019-09-13T16:17:27.8998126Z error: incorrect close delimiter: `)`
2019-09-13T16:17:27.8998725Z    --> src/libstd/sys/wasi/fs.rs:631:64
2019-09-13T16:17:27.8999458Z     |
2019-09-13T16:17:27.9000253Z 631 |                 let path = PathBuf::from(OsString { inner: buf );
2019-09-13T16:17:27.9000673Z     |                                         -         -            ^ incorrect close delimiter
2019-09-13T16:17:27.9001287Z     |                                         |         un-closed delimiter
2019-09-13T16:17:27.9001287Z     |                                         |         un-closed delimiter
2019-09-13T16:17:27.9001576Z     |                                         close delimiter possibly meant for this
2019-09-13T16:17:30.2430860Z error: aborting due to previous error
2019-09-13T16:17:30.2431650Z 
2019-09-13T16:17:30.2763532Z error: Could not compile `std`.
2019-09-13T16:17:30.2764180Z 
---
2019-09-13T16:17:30.2859944Z == clock drift check ==
2019-09-13T16:17:30.2875489Z   local time: Fri Sep 13 16:17:30 UTC 2019
2019-09-13T16:17:30.3786523Z   network time: Fri, 13 Sep 2019 16:17:30 GMT
2019-09-13T16:17:30.3791382Z == end clock drift check ==
2019-09-13T16:17:32.5495669Z ##[error]Bash exited with code '1'.
2019-09-13T16:17:32.5524242Z ##[section]Starting: Checkout
2019-09-13T16:17:32.5525674Z ==============================================================================
2019-09-13T16:17:32.5525716Z Task         : Get sources
2019-09-13T16:17:32.5525753Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[bluetech]

If a new preopen-type is added, it will be skipped silently. That's probably the right thing to do though.

[alexcrichton]

Thanks for the PR! I don't think though that this is ready at this time to move into Rust. The canonical definition in the upstream libc is usable by us and by using it we help reduce duplication of its definition. If it were a well-specified function that had a standard I think it'd be reasonable to reimplement in Rust, but currently the function itself is likely to be in flux as the wasi spec updates over time, so to reduce churn I'd prefer that we keep using the C version.

[newpavlov]

Well, if you think that fd_prestat_get and/or fd_prestat_dir_name will be significantly changed in the near future, then sure. But a short search didn't reveal any such plans. Even if __wasi_prestat_t will be extended with additional descriptor types, it will not affect the get_paths function. I hope @sunfishcode will join this discussion.

Note that in the previous PR I've already replaced arguments retrieval to a pure Rust variant, I haven't touched __wasilibc_find_relpath only because I didn't fully understand it and how it can be re-implemented in terms of the Core API. So this PR also makes sense from consistency PoV. Plus it's more efficient than the C variant, requires less allocations and does not contain any dances around null terminated strings. And if rights argument can be indeed removed, it will allow a certain simplification of the code in this module.

[sorpaas]

@newpavlov I think this also happens to fix WebAssembly/WASI#24 right? If I understand it correctly, paths are now initialized on the first call to open_parent, so the libpreopen initialization seems not required any more.

[newpavlov]

@sorpaas
Thank you for linking this issue! Looks like this PR should indeed fix it, even though I didn't know about it. :)

[alexcrichton]

Most of WASI is still in development, and basically nothing is stable other than wasm itself. There working group is moving slowly to start stabilizing APIs, but I would suspsect that the way in which preopened file descriptors are communicated to the wasm module is highly likely to change at least a little over time, the current scheme AFAIK was the first thing implemented after wasi inherited CloudABI mostly.

I agree that fixing WebAssembly/WASI#24 is important, but that can also be done in wasi-libc too.

[newpavlov]

We already use Core API extensively in std, I don't see how replacing one non-standard libc function with two Core API calls makes a big difference. After any breaking change to Core API, we will have to check if they affect std and if necessary to introduce appropriate patches. Also IIUC the same stability argument can be applied to wasilibc functions as well, since they are not stable and may change in future.

[alexcrichton]

We do, yes, but most of that seems relatively more stable than the funkiness with preopened file descriptors. Additionally it's not like the bits from libc are going away, so since they're already doing the work at startup no matter what it seems like we should use it rather than duplicate it.

I'm not really sure what the motivation is here beyond "let's try to use less C" which I feel like isn't a great argument for the change? I understand there's some maybe perf or code size things going on here too but those also seem very inconsequential to the point that it's not really motivation for the change one way or another.

[hdhoang]

ping from triage @newpavlov, any updates on this?

[newpavlov]

The PR is ready, so I am not sure why the S-waiting-on-author tag is used. I believe it's worth to introduce this change since it brings us as close as possible to libc-free WASI target and makes std consistent (i.e. libc for WASI is used only for interoperability with C/C++ and everything else is done via Core API). The bug fix with other minor improvements is a great bonus in addition to that. I hope @sunfishcode will join this discussion as well.

[alexcrichton]

My opinion hasn't changed from before, I don't think the marginal benefits of this outweigh the instability brought on by avoiding an upstream API.

[JohnCSimon]

Ping from triage.
This PR has sat idle for the last 8 days

@newpavlov @alexcrichton I guess this PR won;'t be moving forward. So, I'll be closing it now.
Thank you for contributing.