Unify fd_readdir impl between *nixes #613

kubkon · 2019-11-21T12:45:28Z

This commit unifies the implementation of fd_readdir between Linux
and BSD hosts. In particular, it re-uses the Dirent, Entry, and
Dir (among others) building blocks introduced recently when
fd_readdir was being implemented on Windows.

Notable changes:

on BSD, wraps readdir syscall in an Iterator of the mutex-locked
Dir struct
on BSD, removes DirStream struct from OsFile; OsFile now holds a
mutex to Dir
makes Dir iterators implementation specific (Linux has its own,
and so does BSD)

cc @marmistrz

Note: this PR depends on #612 as a by-product of me accidentally disabling the WASI tests in #600 (sorry!). This has now been sorted out.

This commit unifies the implementation of `fd_readdir` between Linux and BSD hosts. In particular, it re-uses the `Dirent`, `Entry`, and `Dir` (among others) building blocks introduced recently when `fd_readdir` was being implemented on Windows. Notable changes: * on BSD, wraps `readdir` syscall in an `Iterator` of the mutex-locked `Dir` struct * on BSD, removes `DirStream` struct from `OsFile`; `OsFile` now holds a mutex to `Dir` * makes `Dir` iterators implementation specific (Linux has its own, and so does BSD)

marmistrz · 2019-11-21T13:34:06Z

crates/wasi-common/src/sys/unix/bsd/hostcalls_impl.rs

+            //     descriptor, or to modify the state of the associated description other
+            //     than by means of closedir(), readdir(), readdir_r(), or rewinddir(),
+            //     the behaviour is undefined.
+            let fd = (*os_file).try_clone()?;


If osfile.dir already is Some, then you're making a useless syscall for dup with a throwaway result.

Could you elaborate? This leg is only executed if os_file.dir is not set, i.e., None, so I'm not sure what you mean here.

Oh, I focused on the get_or_insert too much :) It's fine then!

marmistrz · 2019-11-21T13:35:20Z

crates/wasi-common/src/sys/unix/bsd/hostcalls_impl.rs

+            //     control of the system, and if any attempt is made to close the file
+            //     descriptor, or to modify the state of the associated description other
+            //     than by means of closedir(), readdir(), readdir_r(), or rewinddir(),
+            //     the behaviour is undefined.


We seem to be mostly using the American English variant behavior, so I think it's best to choose one variant and stay consistent :)

As a British citizen I have to respectfully decline :-P

marmistrz · 2019-11-21T13:36:19Z

crates/wasi-common/src/sys/unix/bsd/hostcalls_impl.rs

+    // new items may not be returned to the caller.
+    if cookie == wasi::__WASI_DIRCOOKIE_START {
+        log::trace!("     | fd_readdir: doing rewinddir");
+        dir.lock().unwrap().rewind();


Please lock the mutex only once.

marmistrz · 2019-11-21T14:01:12Z

crates/wasi-common/src/sys/unix/bsd/hostcalls_impl.rs

+    }))
+}
+
+struct DirIter<'a>(MutexGuard<'a, Dir>);


Could we possibly have a common DirIter implementation for all *nixes?

On Linux, nothing will prevent the caller to execute concurrent fd_readdir's on the same directory stream, so I don't see why we should prevent it on BSD. In particular readdir(2) on Linux says: (emphasis added)

In cases where multiple threads must read from the same directory stream, using readdir() with external synchronization is still preferable to the use of the deprecated readdir_r(3) function

This means, that code which doesn't have do any synchronization itself for readdiring a single directory stram from multiple threads will be invalid anyway, so we can probably get away with not having mutexes here. Unfortunately, this would probably require adding some unsafe code, but we might want to do it for performance reasons.

In particular, when threads are introduced to WASI, thread-safety of various function will have to be evaluated.

I think I'd prefer to leave it behind a Mutex as-is for now. This change was not introduced in this PR, but only refactored out from the actual implementation of fd_readdir on BSD that we've had until now. I'm OK with some code duplication between Linux and BSD, especially since on BSD we still need to recoved the current location using telldir as it's not available to be read directly from the dirent entry.

Then we could possibly have a system-dependent implementation of Entry which would reuse the d_off value on Linux and call telldir on other nixes, or just call telldir everywhere (it's just a glibc call, so while it's not free, it's cheap).

But I'm fine with addressing this in a subsequent PR.

Agreed to address this in a subsequent PR. :-)

marmistrz · 2019-11-21T14:11:54Z

crates/wasi-common/src/sys/unix/linux/hostcalls_impl.rs

@@ -98,7 +97,7 @@ pub(crate) fn fd_readdir_impl(
        dir.seek(loc);
    }

-    Ok(dir.into_iter().map(|entry| {
+    Ok(DirIter(dir).map(|entry| {


into_iter seems more idiomatic to me, is there any particular reason why you switched to literal DirIter?

Since it's an internal structure, I wanted to keep DirIter private to the module. By implementing an IntoIterator for Dir we have to leak the struct outside of the module we seems like a lot of noise for little to be gained.

Fair enough!

marmistrz · 2019-11-21T14:13:39Z

crates/wasi-common/src/sys/unix/bsd/hostcalls_impl.rs

+                if errno != Errno::last() {
+                    // According to POSIX man (for Linux though!), there was an error
+                    // if the errno value has changed at some point during the sequence
+                    // of readdir calls


Linux man says something different:

To distinguish end of stream and from an error, set errno to zero before calling readdir() and then check the value of errno if NULL is returned.

POSIX says the same:

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.

Interesting, since this reference says something different:

If the end of the directory stream is reached, NULL is returned and errno is not changed. If an error occurs, NULL is returned and errno is set appropriately.

Also, please note that this bit of code was introduced in 4982878 as a fix to a subtle bug that only emerged when testing the crate in release.

The version I quoted is from

This page is part of release 5.03 of the Linux man-pages project.

and

This is POSIX.1-2008 with the 2013 Technical Corrigendum 1 applied.

respectively.

Hmm, interesting. Perhaps this is a BSD-only thing (we won't know for sure until we rewrite fd_readdir on Linux using readdir rather than readdir_r. Anyhow, as a compromise, I can remove the mention of Linux in the comment.

marmistrz · 2019-11-21T14:15:57Z

crates/wasi-common/src/sys/unix/bsd/osfile.rs

 #[derive(Debug)]
 pub(crate) struct OsFile {
    pub(crate) file: fs::File,
-    pub(crate) dir_stream: Option<Mutex<DirStream>>,
+    pub(crate) dir: Option<Mutex<Dir>>,


Please add a comment why we're storing it in OsFile (or mentioned where the reason is described in more detail)

Can you add a reference that BSD nixes require the client to do the caching?

Sure thing. It's actually taken directly from the man pages. Let me add that in.

marmistrz

LGTM!

marmistrz · 2019-11-21T15:28:11Z

crates/wasi-common/src/sys/unix/bsd/osfile.rs

 #[derive(Debug)]
 pub(crate) struct OsFile {
    pub(crate) file: fs::File,
-    pub(crate) dir_stream: Option<Mutex<DirStream>>,
+    pub(crate) dir: Option<Mutex<Dir>>,


Can you add a reference that BSD nixes require the client to do the caching?

kubkon · 2019-11-24T09:29:00Z

I'm gonna go ahead and merge this so that I don't necessarily block other avenues including #520. Feel free to reopen or submit an issue if you feel something is amiss though!

kubkon added the wasi:impl Issues pertaining to WASI implementation in Wasmtime label Nov 21, 2019

kubkon requested review from peterhuene and sunfishcode November 21, 2019 12:45

kubkon force-pushed the fd_readdir_bsd branch from 3e1481e to fc23cde Compare November 21, 2019 12:53

marmistrz suggested changes Nov 21, 2019

View reviewed changes

Lock mutex once only; explain dir in OsFile

8569897

marmistrz approved these changes Nov 21, 2019

View reviewed changes

Add more comments

941ef82

kubkon merged commit c45f709 into bytecodealliance:master Nov 24, 2019

kubkon deleted the fd_readdir_bsd branch November 24, 2019 09:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify fd_readdir impl between *nixes #613

Unify fd_readdir impl between *nixes #613

kubkon commented Nov 21, 2019 •

edited

Loading

marmistrz Nov 21, 2019

kubkon Nov 21, 2019

marmistrz Nov 21, 2019

kubkon Nov 21, 2019

marmistrz Nov 21, 2019

kubkon Nov 21, 2019

marmistrz Nov 21, 2019

kubkon Nov 21, 2019

marmistrz Nov 21, 2019

kubkon Nov 21, 2019

marmistrz Nov 21, 2019

kubkon Nov 21, 2019

marmistrz Nov 21, 2019

kubkon Nov 21, 2019

marmistrz Nov 21, 2019

marmistrz Nov 21, 2019

kubkon Nov 21, 2019

marmistrz Nov 21, 2019

kubkon Nov 21, 2019

marmistrz Nov 21, 2019

kubkon Nov 21, 2019

marmistrz Nov 21, 2019

kubkon Nov 21, 2019

marmistrz left a comment

marmistrz Nov 21, 2019

kubkon commented Nov 24, 2019

Unify fd_readdir impl between *nixes #613

Unify fd_readdir impl between *nixes #613

Conversation

kubkon commented Nov 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmistrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubkon commented Nov 24, 2019

kubkon commented Nov 21, 2019 •

edited

Loading