Applied suggestions from @theduke

wasmerio · Mar 16, 2023 · 6fb20e4 · 6fb20e4
1 parent 6cbaa0a
commit 6fb20e4
Show file tree

Hide file tree

Showing 5 changed files with 73 additions and 34 deletions.
diff --git a/lib/vfs/src/lib.rs b/lib/vfs/src/lib.rs
@@ -30,7 +30,7 @@ pub mod union_fs;
 pub mod zero_file;
 // tty_file -> see wasmer_wasi::tty_file
 mod filesystems;
-pub mod ops;
+pub(crate) mod ops;
 mod overlay_fs;
 pub mod pipe;
 #[cfg(feature = "static-fs")]

diff --git a/lib/vfs/src/mem_fs/filesystem.rs b/lib/vfs/src/mem_fs/filesystem.rs
@@ -12,9 +12,8 @@ use std::sync::{Arc, RwLock};
 
 /// The in-memory file system!
 ///
-/// It's a thin wrapper around [`FileSystemInner`]. This `FileSystem`
-/// type can be cloned, it's a light copy of the `FileSystemInner`
-/// (which is behind a `Arc` + `RwLock`.
+/// This `FileSystem` type can be cloned, it's a light copy of the
+/// `FileSystemInner` (which is behind a `Arc` + `RwLock`).
 #[derive(Clone, Default)]
 pub struct FileSystem {
     pub(super) inner: Arc<RwLock<FileSystemInner>>,
@@ -955,7 +954,7 @@ impl DirectoryMustBeEmpty {
 
 #[cfg(test)]
 mod test_filesystem {
-    use crate::{mem_fs::*, DirEntry, FileSystem as FS, FileSystemExt, FileType, FsError};
+    use crate::{mem_fs::*, ops, DirEntry, FileSystem as FS, FileType, FsError};
 
     macro_rules! path {
         ($path:expr) => {
@@ -1707,9 +1706,9 @@ mod test_filesystem {
     #[ignore = "Not yet supported. See https://github.com/wasmerio/wasmer/issues/3678"]
     fn mount_to_overlapping_directories() {
         let top_level = FileSystem::default();
-        top_level.touch("/file.txt").unwrap();
+        ops::touch(&top_level, "/file.txt").unwrap();
         let nested = FileSystem::default();
-        nested.touch("/another-file.txt").unwrap();
+        ops::touch(&nested, "/another-file.txt").unwrap();
         let top_level: Arc<dyn crate::FileSystem + Send + Sync> = Arc::new(top_level);
         let nested: Arc<dyn crate::FileSystem + Send + Sync> = Arc::new(nested);
 
@@ -1719,9 +1718,9 @@ mod test_filesystem {
         fs.mount("/top-level/nested".into(), &nested, "/".into())
             .unwrap();
 
-        assert!(fs.is_dir("/top-level"));
-        assert!(fs.is_file("/top-level/file.txt"));
-        assert!(fs.is_dir("/top-level/nested"));
-        assert!(fs.is_file("/top-level/nested/another-file.txt"));
+        assert!(ops::is_dir(&fs, "/top-level"));
+        assert!(ops::is_file(&fs, "/top-level/file.txt"));
+        assert!(ops::is_dir(&fs, "/top-level/nested"));
+        assert!(ops::is_file(&fs, "/top-level/nested/another-file.txt"));
     }
 }
diff --git a/lib/vfs/src/ops.rs b/lib/vfs/src/ops.rs
@@ -1,4 +1,5 @@
 //! Common [`FileSystem`] operations.
+#![allow(dead_code)] // Most of these helpers are used during testing
 
 use std::{collections::VecDeque, path::Path};
 

diff --git a/lib/vfs/src/overlay_fs.rs b/lib/vfs/src/overlay_fs.rs
@@ -120,7 +120,14 @@ where
                     }
                     had_at_least_one_success = true;
                 }
-                Err(e) if should_continue(e) => continue,
+                Err(e)
+                    if {
+                        let e = e;
+                        matches!(e, FsError::EntryNotFound)
+                    } =>
+                {
+                    continue
+                }
                 Err(e) => return Err(e),
             }
         }
@@ -141,7 +148,11 @@ where
 
     fn create_dir(&self, path: &Path) -> Result<(), FsError> {
         match self.primary.create_dir(path) {
-            Err(e) if should_continue(e) => {}
+            Err(e)
+                if {
+                    let e = e;
+                    matches!(e, FsError::EntryNotFound)
+                } => {}
             other => return other,
         }
 
@@ -150,7 +161,11 @@ where
 
     fn remove_dir(&self, path: &Path) -> Result<(), FsError> {
         match self.primary.remove_dir(path) {
-            Err(e) if should_continue(e) => {}
+            Err(e)
+                if {
+                    let e = e;
+                    matches!(e, FsError::EntryNotFound)
+                } => {}
             other => return other,
         }
 
@@ -159,7 +174,11 @@ where
 
     fn rename(&self, from: &Path, to: &Path) -> Result<(), FsError> {
         match self.primary.rename(from, to) {
-            Err(e) if should_continue(e) => {}
+            Err(e)
+                if {
+                    let e = e;
+                    matches!(e, FsError::EntryNotFound)
+                } => {}
             other => return other,
         }
 
@@ -169,13 +188,24 @@ where
     fn metadata(&self, path: &Path) -> Result<Metadata, FsError> {
         match self.primary.metadata(path) {
             Ok(meta) => return Ok(meta),
-            Err(e) if should_continue(e) => {}
+            Err(e)
+                if {
+                    let e = e;
+                    matches!(e, FsError::EntryNotFound)
+                } => {}
             Err(e) => return Err(e),
         }
 
         for fs in self.secondaries.filesystems() {
             match fs.metadata(path) {
-                Err(e) if should_continue(e) => continue,
+                Err(e)
+                    if {
+                        let e = e;
+                        matches!(e, FsError::EntryNotFound)
+                    } =>
+                {
+                    continue
+                }
                 other => return other,
             }
         }
@@ -185,7 +215,11 @@ where
 
     fn remove_file(&self, path: &Path) -> Result<(), FsError> {
         match self.primary.remove_file(path) {
-            Err(e) if should_continue(e) => {}
+            Err(e)
+                if {
+                    let e = e;
+                    matches!(e, FsError::EntryNotFound)
+                } => {}
             other => return other,
         }
 
@@ -213,7 +247,11 @@ where
             .options(conf.clone())
             .open(path)
         {
-            Err(e) if should_continue(e) => {}
+            Err(e)
+                if {
+                    let e = e;
+                    matches!(e, FsError::EntryNotFound)
+                } => {}
             other => return other,
         }
 
@@ -246,7 +284,14 @@ where
 
         for fs in self.secondaries.filesystems() {
             match fs.new_open_options().options(conf.clone()).open(path) {
-                Err(e) if should_continue(e) => continue,
+                Err(e)
+                    if {
+                        let e = e;
+                        matches!(e, FsError::EntryNotFound)
+                    } =>
+                {
+                    continue
+                }
                 other => return other,
             }
         }
@@ -304,18 +349,13 @@ where
             }
         }
 
-        f.debug_struct("FileSystem")
+        f.debug_struct("OverlayFileSystem")
             .field("primary", &self.primary)
             .field("secondaries", &IterFilesystems(&self.secondaries))
             .finish()
     }
 }
 
-/// Is it okay to use a fallback filesystem to deal with this particular error?
-fn should_continue(e: FsError) -> bool {
-    matches!(e, FsError::EntryNotFound)
-}
-
 #[cfg(test)]
 mod tests {
     use std::{path::PathBuf, sync::Arc};

diff --git a/lib/vfs/src/union_fs.rs b/lib/vfs/src/union_fs.rs
@@ -465,8 +465,7 @@ mod tests {
     use tokio::io::AsyncWriteExt;
 
     use crate::{
-        host_fs::FileSystem, mem_fs, FileSystem as FileSystemTrait, FileSystemExt, FsError,
-        UnionFileSystem,
+        host_fs::FileSystem, mem_fs, ops, FileSystem as FileSystemTrait, FsError, UnionFileSystem,
     };
 
     fn gen_filesystem() -> UnionFileSystem {
@@ -1078,9 +1077,9 @@ mod tests {
     #[ignore = "Not yet supported. See https://github.com/wasmerio/wasmer/issues/3678"]
     fn mount_to_overlapping_directories() {
         let top_level = mem_fs::FileSystem::default();
-        top_level.touch("/file.txt").unwrap();
+        ops::touch(&top_level, "/file.txt").unwrap();
         let nested = mem_fs::FileSystem::default();
-        nested.touch("/another-file.txt").unwrap();
+        ops::touch(&nested, "/another-file.txt").unwrap();
 
         let mut fs = UnionFileSystem::default();
         fs.mount(
@@ -1098,9 +1097,9 @@ mod tests {
             Some("/top-level/nested"),
         );
 
-        assert!(fs.is_dir("/top-level"));
-        assert!(fs.is_file("/top-level/file.txt"));
-        assert!(fs.is_dir("/top-level/nested"));
-        assert!(fs.is_file("/top-level/nested/another-file.txt"));
+        assert!(ops::is_dir(&fs, "/top-level"));
+        assert!(ops::is_file(&fs, "/top-level/file.txt"));
+        assert!(ops::is_dir(&fs, "/top-level/nested"));
+        assert!(ops::is_file(&fs, "/top-level/nested/another-file.txt"));
     }
 }