Merge pull request #45 from jmmv/delete

Cargo.toml

@@ -28,7 +28,7 @@ rev = "4b23d3962c1baff6483caabf1ca15068216a7506"
 [dependencies.nix]
 # TODO(jmmv): Replace this with 0.12 once released.
 git = "https://github.com/nix-rust/nix.git"
-rev = "eef3a432d57e8f830e05fede6e3099dcb689aa6b"
+rev = "8c3e43ccd4fc83583c16848a35410022f5a8efc9"
 
 [dev-dependencies]
 tempdir = "0.3"

WORKSPACE

@@ -57,7 +57,7 @@ go_repository(
 go_repository(
     name = "org_golang_x_sys",
     importpath = "golang.org/x/sys",
-    commit = "4b45465282a4624cf39876842a017334f13b8aff",
+    commit = "9b800f95dbbc54abff0acf7ee32d88ba4e328c89",
 )
 
 gazelle_dependencies()

admin/travis-build.sh

@@ -91,16 +91,10 @@ do_rust() {
     TestProfiling_FileProfiles
     TestProfiling_BadConfiguration
     TestReadOnly_Attributes
-    TestReadWrite_Remove
     TestReadWrite_InodeReassignedAfterRecreation
-    TestReadWrite_FstatOnDeletedNode
-    TestReadWrite_FtruncateOnDeletedFile
     TestReadWrite_NestedMappingsInheritDirectoryProperties
     TestReadWrite_RenameFile
     TestReadWrite_MoveFile
-    TestReadWrite_FchmodOnDeletedNode
-    TestReadWrite_FchownOnDeletedNode
-    TestReadWrite_FutimesOnDeletedNode
     TestReconfiguration_Streams
     TestReconfiguration_Steps
     TestReconfiguration_Unmap

integration/read_write_test.go

@@ -24,6 +24,8 @@ import (
 	"testing"
 	"time"
 
+	"golang.org/x/sys/unix"
+
 	"github.com/bazelbuild/sandboxfs/integration/utils"
 	"github.com/bazelbuild/sandboxfs/internal/sandbox"
 )
@@ -565,20 +567,11 @@ func TestReadWrite_Chmod(t *testing.T) {
 		}
 	})
 
-	t.Run("DanglingSymlink", func(t *testing.T) {
-		utils.MustSymlink(t, "missing", state.RootPath("dangling-symlink"))
-
-		path := state.MountPath("dangling-symlink")
-		if err := os.Chmod(path, 0555); err == nil {
-			t.Errorf("Want chmod to fail on dangling link, got success")
-		}
-	})
-
-	t.Run("GoodSymlink", func(t *testing.T) {
+	t.Run("Symlink", func(t *testing.T) {
 		utils.MustWriteFile(t, state.RootPath("target"), 0644, "")
-		utils.MustSymlink(t, "target", state.RootPath("good-symlink"))
+		utils.MustSymlink(t, "target", state.RootPath("symlink"))
 
-		path := state.MountPath("good-symlink")
+		path := state.MountPath("symlink")
 		linkFileInfo, err := os.Lstat(path)
 		if err != nil {
 			t.Fatalf("Failed to stat %s: %v", path, err)
@@ -588,11 +581,11 @@ func TestReadWrite_Chmod(t *testing.T) {
 			t.Fatalf("Failed to chmod %s: %v", path, err)
 		}
 
-		if err := checkPerm("good-symlink", linkFileInfo.Mode()&os.ModePerm); err != nil {
+		if err := checkPerm("symlink", linkFileInfo.Mode()&os.ModePerm); err != nil {
 			t.Error(err)
 		}
 		if err := checkPerm("target", 0200); err != nil {
-			t.Error(err)
+			t.Errorf("Mode of symlink target was modified but shouldn't have been: %v", err)
 		}
 	})
 }
@@ -664,9 +657,8 @@ func TestReadWrite_Chown(t *testing.T) {
 
 	utils.MustMkdirAll(t, state.RootPath("dir"), 0755)
 	utils.MustWriteFile(t, state.RootPath("file"), 0644, "new content")
-	utils.MustSymlink(t, "missing", state.RootPath("dangling-symlink"))
 	utils.MustWriteFile(t, state.RootPath("target"), 0644, "")
-	utils.MustSymlink(t, "target", state.RootPath("good-symlink"))
+	utils.MustSymlink(t, "target", state.RootPath("symlink"))
 
 	targetFileInfo, err := os.Lstat(state.RootPath("target"))
 	if err != nil {
@@ -683,8 +675,7 @@ func TestReadWrite_Chown(t *testing.T) {
 	}{
 		{"Dir", "dir", 1, 2},
 		{"File", "file", 3, 4},
-		{"DanglingSymlink", "dangling-symlink", 5, 6},
-		{"GoodSymlink", "good-symlink", 7, 8},
+		{"Symlink", "symlink", 7, 8},
 	}
 	for _, d := range testData {
 		t.Run(d.name, func(t *testing.T) {
@@ -821,35 +812,22 @@ func TestReadWrite_Chtimes(t *testing.T) {
 		}
 	})
 
-	t.Run("DanglingSymlink", func(t *testing.T) {
-		utils.MustSymlink(t, "missing", state.RootPath("dangling-symlink"))
-
-		if _, err := chtimes("dangling-symlink", time.Unix(0, 0), time.Unix(0, 0)); err == nil {
-			t.Errorf("Want chtimes to fail on dangling link, got success")
-		}
-	})
-
-	t.Run("GoodSymlink", func(t *testing.T) {
+	t.Run("Symlink", func(t *testing.T) {
 		utils.MustWriteFile(t, state.RootPath("target"), 0644, "")
-		utils.MustSymlink(t, "target", state.RootPath("good-symlink"))
-		path := state.MountPath("good-symlink")
+		utils.MustSymlink(t, "target", state.RootPath("symlink"))
+		path := state.MountPath("symlink")
 
-		linkFileInfo, err := os.Lstat(path)
+		atimeTimespec, err := unix.TimeToTimespec(someAtime)
 		if err != nil {
-			t.Fatalf("Failed to stat %s: %v", path, err)
+			t.Fatalf("Failed to convert %v to a timespec: %v", someAtime, err)
 		}
-		linkStat := linkFileInfo.Sys().(*syscall.Stat_t)
-
-		wantMinCtime, err := chtimes(path, someAtime, someMtime)
+		mtimeTimespec, err := unix.TimeToTimespec(someMtime)
 		if err != nil {
-			t.Fatal(err)
+			t.Fatalf("Failed to convert %v to a timespec: %v", someMtime, err)
 		}
 
-		if err := checkTimes("good-symlink", time.Unix(0, 0), linkFileInfo.ModTime(), sandbox.Ctime(linkStat)); err != nil {
-			t.Error(err)
-		}
-		if err := checkTimes("target", someAtime, someMtime, wantMinCtime); err != nil {
-			t.Error(err)
+		if err = unix.UtimesNanoAt(unix.AT_FDCWD, path, []unix.Timespec{atimeTimespec, mtimeTimespec}, unix.AT_SYMLINK_NOFOLLOW); err == nil || err != syscall.EOPNOTSUPP {
+			t.Fatalf("Expected EOPNOTSUPP changing the times of a symlink; got %v", err)
 		}
 	})
 }

internal/sandbox/node.go

@@ -342,6 +342,13 @@ func (n *BaseNode) setattrTimes(req *fuse.SetattrRequest) error {
 	}
 
 	if underlyingPath, isMapped := n.UnderlyingPath(); isMapped && !n.deleted {
+		if n.attr.Mode&os.ModeType == os.ModeSymlink {
+			// TODO(https://github.com/bazelbuild/sandboxfs/issues/46): Should use
+			// futimensat to support changing the times of a symlink if requested to
+			// do so.
+			return fuse.Errno(syscall.EOPNOTSUPP)
+		}
+
 		if err := os.Chtimes(underlyingPath, atime, mtime); err != nil {
 			return err
 		}

src/lib.rs

@@ -12,6 +12,13 @@
 // License for the specific language governing permissions and limitations
 // under the License.
 
+// For portability reasons, we need to be able to cast integer values to system-level opaque
+// types such as "mode_t".  Because we don't know the size of those integers on the platform we
+// are building for, sometimes the casts do widen the values but other times they are no-ops.
+#![cfg_attr(feature = "cargo-clippy", allow(identity_conversion))]
+
+// We construct complex structures in multiple places, and allowing for redundant field names
+// increases readability.
 #![cfg_attr(feature = "cargo-clippy", allow(redundant_field_names))]
 
 #[macro_use] extern crate failure;
@@ -482,9 +489,17 @@ impl fuse::Filesystem for SandboxFS {
         panic!("Required RW operation not yet implemented");
     }
 
-    fn rmdir(&mut self, _req: &fuse::Request, _parent: u64, _name: &OsStr,
-        _reply: fuse::ReplyEmpty) {
-        panic!("Required RW operation not yet implemented");
+    fn rmdir(&mut self, _req: &fuse::Request, parent: u64, name: &OsStr, reply: fuse::ReplyEmpty) {
+        let dir_node = self.find_node(parent);
+        if !dir_node.writable() {
+            reply.error(Errno::EPERM as i32);
+            return;
+        }
+
+        match dir_node.rmdir(name) {
+            Ok(_) => reply.ok(),
+            Err(e) => reply.error(e.errno_as_i32()),
+        }
     }
 
     fn setattr(&mut self, _req: &fuse::Request, inode: u64, mode: Option<u32>, uid: Option<u32>,
@@ -538,9 +553,17 @@ impl fuse::Filesystem for SandboxFS {
         }
     }
 
-    fn unlink(&mut self, _req: &fuse::Request, _parent: u64, _name: &OsStr,
-        _reply: fuse::ReplyEmpty) {
-        panic!("Required RW operation not yet implemented");
+    fn unlink(&mut self, _req: &fuse::Request, parent: u64, name: &OsStr, reply: fuse::ReplyEmpty) {
+        let dir_node = self.find_node(parent);
+        if !dir_node.writable() {
+            reply.error(Errno::EPERM as i32);
+            return;
+        }
+
+        match dir_node.unlink(name) {
+            Ok(_) => reply.ok(),
+            Err(e) => reply.error(e.errno_as_i32()),
+        }
     }
 
     fn write(&mut self, _req: &fuse::Request, _inode: u64, fh: u64, offset: i64, data: &[u8],

src/nodes/conv.rs

@@ -60,6 +60,18 @@ pub fn timespec_to_timeval(spec: Timespec) -> sys::time::TimeVal {
     TimeVal::seconds(spec.sec) + TimeVal::nanoseconds(spec.nsec.into())
 }
 
+/// Converts a `sys::time::TimeVal` object into a `time::Timespec`.
+// TODO(jmmv): Consider upstreaming this function as a TimeVal method.
+pub fn timeval_to_timespec(val: sys::time::TimeVal) -> Timespec {
+    let usec = if val.tv_usec() > sys::time::suseconds_t::from(std::i32::MAX) {
+        warn!("Cannot represent too-long usec quantity {} in timespec; using 0", val.tv_usec());
+        0
+    } else {
+        val.tv_usec() as i32
+    };
+    Timespec::new(val.tv_sec() as sys::time::time_t, usec)
+}
+
 /// Converts a file type as returned by the file system to a FUSE file type.
 ///
 /// `path` is the file from which the file type was originally extracted and is only for debugging
@@ -206,6 +218,14 @@ mod tests {
         assert_eq!(45, val.tv_usec());
     }
 
+    #[test]
+    fn test_timeval_to_timespec() {
+        let val = sys::time::TimeVal::seconds(654) + sys::time::TimeVal::nanoseconds(123_456);
+        let spec = timeval_to_timespec(val);
+        assert_eq!(654, spec.sec);
+        assert_eq!(123, spec.nsec);
+    }
+
     #[test]
     fn test_system_time_to_timespec_ok() {
         let sys_time = SystemTime::UNIX_EPOCH + Duration::new(12345, 6789);

src/nodes/dir.rs

@@ -254,12 +254,12 @@ impl Dir {
         if let Some(path) = &state.underlying_path {
             let fs_attr = fs::symlink_metadata(path)?;
             if !fs_attr.is_dir() {
-                warn!("Path {:?} backing a directory node is no longer a directory; got {:?}",
-                    path, fs_attr.file_type());
+                warn!("Path {} backing a directory node is no longer a directory; got {:?}",
+                    path.display(), fs_attr.file_type());
                 return Err(KernelError::from_errno(errno::Errno::EIO));
             }
             state.attr = conv::attr_fs_to_fuse(path, inode, &fs_attr);
-        };
+        }
 
         Ok(state.attr)
     }
@@ -348,6 +348,22 @@ impl Dir {
             }
         }
     }
+
+    /// Common implementation for the `rmdir` and `unlink` operations.
+    ///
+    /// The behavior of these operations differs only in the syscall we invoke to delete the
+    /// underlying entry, which is passed in as the `remove` parameter.
+    fn remove_any<R>(&self, name: &OsStr, remove: R) -> NodeResult<()>
+        where R: Fn(&PathBuf) -> io::Result<()> {
+        let mut state = self.state.lock().unwrap();
+        let path = Dir::get_writable_path(&mut state, name)?;
+
+        remove(&path)?;
+
+        state.children[name].node.delete();
+        state.children.remove(name);
+        Ok(())
+    }
 }
 
 impl Node for Dir {
@@ -363,6 +379,14 @@ impl Node for Dir {
         fuse::FileType::Directory
     }
 
+    fn delete(&self) {
+        let mut state = self.state.lock().unwrap();
+        assert!(
+            state.underlying_path.is_some(),
+            "Delete already called or trying to delete an explicit mapping");
+        state.underlying_path = None;
+    }
+
     fn map(&self, components: &[Component], underlying_path: &Path, writable: bool,
         ids: &IdGenerator, cache: &Cache) -> Result<(), Error> {
         let (name, remainder) = split_components(components);
@@ -494,12 +518,16 @@ impl Node for Dir {
         }))
     }
 
+    fn rmdir(&self, name: &OsStr) -> NodeResult<()> {
+        // TODO(jmmv): Figure out how to remove the redundant closure.
+        #[cfg_attr(feature = "cargo-clippy", allow(redundant_closure))]
+        self.remove_any(name, |p| fs::remove_dir(p))
+    }
+
     fn setattr(&self, delta: &AttrDelta) -> NodeResult<fuse::FileAttr> {
         let mut state = self.state.lock().unwrap();
-        if let Some(path) = &state.underlying_path {
-            setattr(path, &state.attr, delta)?;
-        }
-        Dir::getattr_locked(self.inode, &mut state)
+        state.attr = setattr(state.underlying_path.as_ref(), &state.attr, delta)?;
+        Ok(state.attr)
     }
 
     fn symlink(&self, name: &OsStr, link: &Path, ids: &IdGenerator, cache: &Cache)
@@ -511,4 +539,10 @@ impl Node for Dir {
         Dir::post_create_lookup(self.writable, &mut state, &path, name,
             fuse::FileType::Symlink, ids, cache)
     }
+
+    fn unlink(&self, name: &OsStr) -> NodeResult<()> {
+        // TODO(jmmv): Figure out how to remove the redundant closure.
+        #[cfg_attr(feature = "cargo-clippy", allow(redundant_closure))]
+        self.remove_any(name, |p| fs::remove_file(p))
+    }
 }