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 nix support for missing host function calls. #521

Closed
sunfishcode opened this issue May 14, 2019 · 23 comments
Closed

Add nix support for missing host function calls. #521

sunfishcode opened this issue May 14, 2019 · 23 comments
Labels
wasi:impl Issues pertaining to WASI implementation in Wasmtime

Comments

@sunfishcode
Copy link
Member

At a quick glance, the following functions are not yet provided by nix, but would be useful to have in nix for use by wasi-common: mkdirat, unlinkat, renameat, linkat, clock_getres, clock_gettime, posix_fallocate, posix_fadvise.

Currently code in wasi-common and lucet-wasi is calling libc interfaces for these functions directly, which requires unsafe code. Having nix provide these will mean the unsafe code is shifted to nix instead. This may not seem like a win on a theoretical level, but the more we can push unsafe code into dedicated utilities that do simple unsafe things with safe, testable, interfaces, the easier it is to audit everything.

I've marked this as "help wanted" -- this is not an urgent project, but it is a nicely scoped, straightforward, and subdividable project, and it's a good chance to see some of the low-level parts of the software stack that wasi-common is built on. And mentoring is available :-).

Additional resources:

@jlb6740
Copy link
Collaborator

jlb6740 commented May 16, 2019

Hi .. I was hoping to be able to help on the wasi-common request and this might be a good place to start. I have an initial patch here for unlinkat: https://github.com/nix-rust/nix/compare/master...jlb6740:implement_unlinkat?expand=1. I am new to rust so could use some pointers on testing this. Once this initial one is tested I'll submit a pull request and see about implementing some of the others.

@sunfishcode
Copy link
Member Author

Hi! Thanks for starting this! The patch looks good. For testing unlinkat, take a look at test_fchmodat in test/test_stat.rs. The basic setup it does is essentially what you want here: it creates a temp directory and a file inside that directory, obtains a directory file descriptor, and then does operations using that file descriptor plus a relative path. For your test, the operation should be to do unlinkat to remove the file. For completeness, you can also create a subdirectory inside the temp directory with mkdir and then remove it with unlinkat with RemoveDir.

Also, sorry, I accidentally hit the wrong button while looking at the link above and opened a PR. I've now closed it. We can open a new PR once the testcase is ready.

@jlb6740
Copy link
Collaborator

jlb6740 commented May 21, 2019

Hi .. I revisited this patch and I think it is ready. Please have a look when you have time .. I do have a couple of questions more related to how github works. The pull request ID still needs updating but not sure the workflow here. I'll begin work on some others assuming this will be ready soon. jlb6740/nix@890de05

@sunfishcode
Copy link
Member Author

This looks good to go! I commented on the commit -- I think you can just submit the PR with "TBD" and fix it up once the PR number is assigned.

@kevinwern
Copy link

Hi! Was interested in helping out with WASI and figured this would be a good place to start. I gave mkdirat a shot. Let me know if you have any suggestions!

@kubkon
Copy link
Member

kubkon commented Jun 10, 2019

Hey, that's awesome, thanks! It'd be great if we could get all missing host function calls added to nix and especially those that we rely on internally in wasi-common. Would you be willing to scan the repo, find any calls that rely directly on libc, and try adding them to nix? For example, we're still missing posix_fadvise.

Feel free to ask any questions, and I'll be happy to help out as much as I can, and @sunfishcode can mentor and straighten me out in case I'm saying something stupid ;-)

@kevinwern
Copy link

kevinwern commented Jun 12, 2019

Definitely up to help out however I can...I remember confirming that the list @sunfishcode had at the top of this issue covered everything but I will double-check that tomorrow. Regardless, I'll try to pick off things bit by bit.

bors bot referenced this issue in nix-rust/nix Jun 16, 2019
1084: sys/stat: implement mkdirat r=asomers a=kevinwern

See: https://github.com/CraneStation/wasi-common/issues/16
https://linux.die.net/man/2/mkdirat

My first contribution to this repo. Tests were probably overkill...

Also, out of curiosity, is there any reason why `mkdir` is located in `unistd`? The documentation I read mentioned the function definition also being located in `sys/stat.h`.

Co-authored-by: Kevin Wern <kevin.m.wern@gmail.com>
@kevinwern
Copy link

kevinwern commented Jun 24, 2019

Continuing to chip away at this, also did a bit of grepping here for direct references to libc functions. I ended up with:

I need to look more into whether or not nix::dir as it is can replace the directory reading/iterating calls listed here. I think that's the case.

Any input @sunfishcode @kubkon ? I'll keep track of things here to the best of knowledge.

@programmerjake
Copy link

memcpy should be able to be replaced with std::ptr::copy_nonoverlapping

@sunfishcode
Copy link
Member Author

@kevinwern That list looks good! As a minor update, sched_yield is now in upstream nix here so we just need to update wasi-common to use it.

@kubkon
Copy link
Member

kubkon commented Jun 24, 2019

@kevinwern That list looks good! As a minor update, sched_yield is now in upstream nix here so we just need to update wasi-common to use it.

That's a good catch. One caveat here though. It is currently quite unfortunate that sched_yield landed in sched module in nix crate which is compiled only for linux-like hosts. This precludes its use on macos for example. I have now submitted a PR to remedy this nix-rust/nix#1090, and I guess when it lands, we can safely move away from using libc::sched_yield directly.

@kubkon
Copy link
Member

kubkon commented Jun 27, 2019

It might also be useful to explore #519 a little bit more, i.e., see whether a fix is needed on our side or nixs.

bors bot referenced this issue in nix-rust/nix Jul 1, 2019
1089: implement posix_fadvise r=asomers a=kevinwern

See: 
https://github.com/CraneStation/wasi-common/issues/16
http://man7.org/linux/man-pages/man2/posix_fadvise.2.html

Conditional compilation derived from corresponding libc conditions:
Fuchsia:
https://github.com/rust-lang/libc/blob/0b02c4060a750983cebcccdf1f35a5ec4cdcf516/src/fuchsia/mod.rs#L3807
https://github.com/rust-lang/libc/blob/0e702c1b4e2e56788e8b67d3efd9c242807c3d4b/src/lib.rs#L104-L109

uClibc:
https://github.com/rust-lang/libc/blob/11c762c535cb43dda3d9d87a0845c55201a905fb/src/unix/uclibc/mod.rs#L1676
https://github.com/rust-lang/libc/blob/ce7e3a7e866dd7109a971b694a2bf58bd08f101a/src/unix/mod.rs#L1141-L1143
https://github.com/rust-lang/libc/blob/0e702c1b4e2e56788e8b67d3efd9c242807c3d4b/src/lib.rs#L116-L121

Linux, android, emscripten:
https://github.com/rust-lang/libc/blob/11c762c535cb43dda3d9d87a0845c55201a905fb/src/unix/linux_like/mod.rs#L1303
https://github.com/rust-lang/libc/blob/ce7e3a7e866dd7109a971b694a2bf58bd08f101a/src/unix/mod.rs#L1147-L1151
https://github.com/rust-lang/libc/blob/0e702c1b4e2e56788e8b67d3efd9c242807c3d4b/src/lib.rs#L116-L121

FreeBSD:
https://github.com/rust-lang/libc/blob/e0ff1e68b9e34173e9c4c3217d1b0fc81a7d352d/src/unix/bsd/freebsdlike/freebsd/mod.rs#L1223
https://github.com/rust-lang/libc/blob/ce7e3a7e866dd7109a971b694a2bf58bd08f101a/src/unix/bsd/freebsdlike/mod.rs#L1307-L1309
https://github.com/rust-lang/libc/blob/ce7e3a7e866dd7109a971b694a2bf58bd08f101a/src/unix/bsd/mod.rs#L683-L685
https://github.com/rust-lang/libc/blob/ce7e3a7e866dd7109a971b694a2bf58bd08f101a/src/unix/mod.rs#L1152-L1159
https://github.com/rust-lang/libc/blob/0e702c1b4e2e56788e8b67d3efd9c242807c3d4b/src/lib.rs#L116-L121

WASI:
https://github.com/rust-lang/libc/blob/0e702c1b4e2e56788e8b67d3efd9c242807c3d4b/src/wasi.rs#L1001
https://github.com/rust-lang/libc/blob/0e702c1b4e2e56788e8b67d3efd9c242807c3d4b/src/lib.rs#L134-L139


Co-authored-by: Kevin Wern <kevin.m.wern@gmail.com>
bors bot referenced this issue in nix-rust/nix Jul 11, 2019
1089: implement posix_fadvise r=asomers a=kevinwern

See: 
https://github.com/CraneStation/wasi-common/issues/16
http://man7.org/linux/man-pages/man2/posix_fadvise.2.html

Conditional compilation derived from corresponding libc conditions:
Fuchsia:
https://github.com/rust-lang/libc/blob/0b02c4060a750983cebcccdf1f35a5ec4cdcf516/src/fuchsia/mod.rs#L3807
https://github.com/rust-lang/libc/blob/0e702c1b4e2e56788e8b67d3efd9c242807c3d4b/src/lib.rs#L104-L109

uClibc:
https://github.com/rust-lang/libc/blob/11c762c535cb43dda3d9d87a0845c55201a905fb/src/unix/uclibc/mod.rs#L1676
https://github.com/rust-lang/libc/blob/ce7e3a7e866dd7109a971b694a2bf58bd08f101a/src/unix/mod.rs#L1141-L1143
https://github.com/rust-lang/libc/blob/0e702c1b4e2e56788e8b67d3efd9c242807c3d4b/src/lib.rs#L116-L121

Linux, android, emscripten:
https://github.com/rust-lang/libc/blob/11c762c535cb43dda3d9d87a0845c55201a905fb/src/unix/linux_like/mod.rs#L1303
https://github.com/rust-lang/libc/blob/ce7e3a7e866dd7109a971b694a2bf58bd08f101a/src/unix/mod.rs#L1147-L1151
https://github.com/rust-lang/libc/blob/0e702c1b4e2e56788e8b67d3efd9c242807c3d4b/src/lib.rs#L116-L121

FreeBSD:
https://github.com/rust-lang/libc/blob/e0ff1e68b9e34173e9c4c3217d1b0fc81a7d352d/src/unix/bsd/freebsdlike/freebsd/mod.rs#L1223
https://github.com/rust-lang/libc/blob/ce7e3a7e866dd7109a971b694a2bf58bd08f101a/src/unix/bsd/freebsdlike/mod.rs#L1307-L1309
https://github.com/rust-lang/libc/blob/ce7e3a7e866dd7109a971b694a2bf58bd08f101a/src/unix/bsd/mod.rs#L683-L685
https://github.com/rust-lang/libc/blob/ce7e3a7e866dd7109a971b694a2bf58bd08f101a/src/unix/mod.rs#L1152-L1159
https://github.com/rust-lang/libc/blob/0e702c1b4e2e56788e8b67d3efd9c242807c3d4b/src/lib.rs#L116-L121

WASI:
https://github.com/rust-lang/libc/blob/0e702c1b4e2e56788e8b67d3efd9c242807c3d4b/src/wasi.rs#L1001
https://github.com/rust-lang/libc/blob/0e702c1b4e2e56788e8b67d3efd9c242807c3d4b/src/lib.rs#L134-L139


Co-authored-by: Kevin Wern <kevin.m.wern@gmail.com>
@jlb6740
Copy link
Collaborator

jlb6740 commented Jul 15, 2019

I think unlinkat is close to merging. I will give linkat and symlinkat a go if no one else has started these.

@kevinwern
Copy link

@jlb6740 noted that on the list. Go ahead!

@jlb6740
Copy link
Collaborator

jlb6740 commented Jul 29, 2019

Looks like symlinkat was done (correct me if there was more here?) unlinkat is in review, though likely needs another update or two. Will start on fdopendir and probably fdreaddir if not taken.

@dingxiangfei2009
Copy link

Not sure how we are going to use posix_fallocate, but here is the work related to it: nix-rust/nix#1105

marmistrz referenced this issue in marmistrz/wasi-common Aug 19, 2019
kubkon referenced this issue in CraneStation/wasi-common Aug 20, 2019
@kevinwern
Copy link

Sorry was away for a bit...updated the list to reflect updates from @jlb6740 and @dingxiangfei2009 .

@sunfishcode
Copy link
Member Author

@dingxiangfei2009 posix_fallocate is desirable for implementing WASI's fd_allocate, because it changes the file size with a single system call, rather than needing to do an fstat to check the size followed by an ftruncate (which is what Rust's File's set_len does).

@nomeata
Copy link

nomeata commented Oct 18, 2019

I was trying to build wasmtime on darwin and I am hitting linker errors related to _utimensat: NixOS/nixpkgs#71316. Is that related to this issue here?

@kubkon
Copy link
Member

kubkon commented Oct 18, 2019

Hey @nomeata, thanks for your comment! Hmm, that's interesting. I assume that by darwin you don't necessarily mean MacOS, right? Anyhow, it might as well be related to the issue you've linked to. Could you submit full logs here so that I could have a closer look please?

@nomeata
Copy link

nomeata commented Oct 18, 2019

(moved to https://github.com/CraneStation/wasi-common/issues/132)

@kubkon
Copy link
Member

kubkon commented Oct 18, 2019

Actually @nomeata, would you mind submitting a new issue for the problem you've experienced? I have an idea for a fix, and would love to have it properly tracked. Also, would you mind working through it with me?

@kubkon kubkon transferred this issue from CraneStation/wasi-common Nov 8, 2019
@kubkon kubkon added the wasi:impl Issues pertaining to WASI implementation in Wasmtime label Nov 8, 2019
@kubkon
Copy link
Member

kubkon commented Dec 11, 2019

Given that we've effectively decided to abandon using nix in wasi-common, and use our own in-house thin, low-level wrapper crate for used Unix syscalls (aka yanix, see #649), I'm closing this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi:impl Issues pertaining to WASI implementation in Wasmtime
Projects
None yet
Development

No branches or pull requests

8 participants
@nomeata @kubkon @sunfishcode @programmerjake @dingxiangfei2009 @kevinwern @jlb6740 and others