Begin porting yanix to WASI #2046
Conversation
Subscribe to Label Actioncc @kubkon
This issue or pull request has been labeled: "wasi"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
sunfishcode
force-pushed
the
yanix-wasi
branch
3 times, most recently
from
4d572da
to
79f4610
Compare
| @@ -25,12 +25,12 @@ impl Dir { | |||
| } | |||
|
|
|||
| unsafe fn from_fd(fd: RawFd) -> Result<Self> { | |||
| let d = libc::fdopendir(fd); | |||
| let d = libc::fdopendir(fd as libc::c_int); | |||
There was a problem hiding this comment.
Hmm, why is this cast suddenly needed here?
There was a problem hiding this comment.
WASI's RawFd is u32. libc::fdopendir takes a c_int, which is signed i32.
| if let Some(d) = ptr::NonNull::new(d) { | ||
| Ok(Self(d)) | ||
| } else { | ||
| let e = io::Error::last_os_error(); | ||
| libc::close(fd); | ||
| libc::close(fd as libc::c_int); |
|
|
||
| impl AsRawFd for Dir { | ||
| fn as_raw_fd(&self) -> RawFd { | ||
| unsafe { libc::dirfd(self.0.as_ptr()) } | ||
| unsafe { libc::dirfd(self.0.as_ptr()) as RawFd } |
There was a problem hiding this comment.
Do we need to as cast here at all?
There was a problem hiding this comment.
As above, RawFd is a u32 on WASI.
| use libc::{ | ||
| fstat as libc_fstat, fstatat as libc_fstatat, lseek as libc_lseek, openat as libc_openat, | ||
| }; | ||
| #[cfg(any(target_os = "linux", target_os = "emscripten", target_os = "l4re"))] |
There was a problem hiding this comment.
It's a microkernel with L4 ancestry. It's just here because I looked that Rust's libc crate to see which platforms needed this, and it was one of them.
| // TODO This should be verified on different BSD-flavours. | ||
| // | ||
| // According to 4.3BSD/POSIX.1-2001 man pages, there was an error | ||
| // if the errno value has changed at some point during the sequence | ||
| // of readdir calls. |
There was a problem hiding this comment.
Have you verified this is the case on different BSD flavours perhaps?
There was a problem hiding this comment.
Ah, yes. FreeBSD's readdir says:
The function returns NULL upon
reaching the end of the directory or on error. In the event of an error,
errno may be set to any of the values documented for the getdirentries(2)
system call.
The Unix convention for situatuations like this is to set errno to 0 before the call, and check if it's non-zero after the call. POSIX's description of readdir makes this explicit:
Applications wishing to check for error situations should set errno to 0 before calling readdir(). If errno is set to non-zero on return, an error occurred.
| Ok(link.into()) | ||
| // Start with a buffer big enough for the vast majority of paths. | ||
| let mut buffer = Vec::with_capacity(256); | ||
| loop { |
There was a problem hiding this comment.
Is this loop necessary for actual OSes? I thought that libc::PATH_MAX existed for a reason and the OS would never return a path that is actually longer, or am I missing something here?
On WASI, it makes sense to loop since we AFAIK we don't have the concept of a PATH_MAX since it's so platform-dependent and could actually leak what host we're running on, but I'm not sure this loop is at all necessary on actual OSes. Plus, doesn't it lead to unnecessary allocs which could have been easily handled using static buffers instead like it was prior to this change?
There was a problem hiding this comment.
For WASI, I want to avoid PATH_MAX because the actual value of PATH_MAX depends on the underlying host OS, so it's better for WASI code to avoid making size assumptions. But you're right that other platforms don't need this. I've now made this code conditional to restore stack allocation.
sunfishcode
force-pushed
the
yanix-wasi
branch
from
b7626d1
to
5edc161
Compare
|
Thanks for the review! I've now responded to all of the comments :-). |
sunfishcode
force-pushed
the
yanix-wasi
branch
from
5edc161
to
8f48ed5
Compare
Also export a `stat` from yanix, so that we can abstract over `stat` vs. `stat64`. While here, rename wasi-common's `from_nix`/`nix_from` functions to mention `yanix`.
Remove the workarounds for rust-lang/rust#74075 now that it's landed on nightly Rust.
The underlying issue is fixed in rust-lang/libc#1827 When that's available in a libc package, this can be tidied up.
This matches what Rust's libc does, and reflects that libc implementations use locking internally with `DIR`.
sunfishcode
force-pushed
the
yanix-wasi
branch
from
8f48ed5
to
e6d75e5
Compare
|
Closing, as this PR is out of date with my latest WASI porting work. |
This is a series of patches which begin porting yanix to WASI, and which contain various other fixes needed for using yanix outside of Wasmtime in general.