Prepare to support setattr on deleted files

In order to implement unlink and rmdir, which will come later, we must
prepare to deal with the case of applying updates to a delete node via
still-open handles.

To do this, avoid doing a getattr immediately after a setattr (because
it may not be possible once the underlying file is gone), and instead
compute what the updated attributes should be.  This is not completely
accurate because of ctime, but it's good enough for now.

Also, make the input path to setattr optional so that we can ask this
function to only update the in-memory properties.

src/nodes/conv.rs

@@ -60,6 +60,12 @@ 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 {
+    Timespec::new(val.tv_sec(), val.tv_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 +212,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

@@ -496,10 +496,14 @@ impl Node for Dir {
 
     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)
+
+        // setattr only gets called on writable nodes, which must have an underlying path.  This
+        // assertion will go away when we have the ability to delete nodes as those may still
+        // exist in memory but have no corresponding underlying path.
+        debug_assert!(state.underlying_path.is_some());
+
+        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)

src/nodes/file.rs

@@ -135,7 +135,7 @@ impl Node for File {
 
     fn setattr(&self, delta: &AttrDelta) -> NodeResult<fuse::FileAttr> {
         let mut state = self.state.lock().unwrap();
-        setattr(&state.underlying_path, &state.attr, delta)?;
-        File::getattr_locked(self.inode, &mut state)
+        state.attr = setattr(Some(&state.underlying_path), &state.attr, delta)?;
+        Ok(state.attr)
     }
 }

src/nodes/mod.rs

@@ -88,57 +88,135 @@ pub struct AttrDelta {
 /// Generic result type for of all node operations.
 pub type NodeResult<T> = Result<T, KernelError>;
 
-/// Updates the metadata of a file given a delta of attributes.
-///
-/// This is a helper function to implement `Node::setattr` for the various node types.  The caller
-/// is responsible for obtaining the updated metadata of the underlying file to return it to the
-/// kernel, as computing the updated metadata here would be inaccurate.  The reason is that we don't
-/// track ctimes ourselves so any modifications to the file cause the backing ctime to be updated
-/// and we need to obtain it.
-/// TODO(https://github.com/bazelbuild/sandboxfs/issues/43): Compute the modified fuse::FileAttr and
-/// return it once we track ctimes.
-///
-/// This tries to apply as many properties as possible in case of errors.  When errors occur,
-/// returns the first that was encountered.
-pub fn setattr(path: &Path, attr: &fuse::FileAttr, delta: &AttrDelta) -> Result<(), nix::Error> {
-    let mut result = Ok(());
+/// Applies an operation to a path if present, or otherwise returns Ok.
+fn try_path<O: Fn(&PathBuf) -> nix::Result<()>>(path: Option<&PathBuf>, op: O) -> nix::Result<()> {
+    match path {
+        Some(path) => op(path),
+        None => Ok(()),
+    }
+}
 
-    if let Some(mode) = delta.mode {
-        result = result.and(
-            sys::stat::fchmodat(None, path, mode, sys::stat::FchmodatFlags::NoFollowSymlink));
+/// Helper function for `setattr` to apply only the mode changes.
+fn setattr_mode(attr: &mut fuse::FileAttr, path: Option<&PathBuf>, mode: Option<sys::stat::Mode>)
+    -> Result<(), nix::Error> {
+    if mode.is_none() {
+        return Ok(())
     }
+    let mode = mode.unwrap();
 
-    if delta.uid.is_some() || delta.gid.is_some() {
-        result = result.and(
-            unistd::fchownat(None, path, delta.uid, delta.gid,
-                unistd::FchownatFlags::NoFollowSymlink));
+    let result = try_path(path, |p|
+        sys::stat::fchmodat(None, p, mode, sys::stat::FchmodatFlags::NoFollowSymlink));
+    if result.is_ok() {
+        attr.perm = mode.bits();
     }
+    result
+}
 
-    if delta.atime.is_some() || delta.mtime.is_some() {
-        let atime = delta.atime.unwrap_or_else(|| conv::timespec_to_timeval(attr.atime));
-        let mtime = delta.mtime.unwrap_or_else(|| conv::timespec_to_timeval(attr.mtime));
-        if attr.kind == fuse::FileType::Symlink {
-            // TODO(jmmv): Should use futimensat to avoid following symlinks but this function is
-            // not available on macOS El Capitan, which we currently require for CI builds.
-            warn!("Asked to update atime/mtime for symlink {} but following symlink instead",
-                path.display());
-        }
-        result = result.and(sys::stat::utimes(path, &atime, &mtime));
+/// Helper function for `setattr` to apply only the UID and GID changes.
+fn setattr_owners(attr: &mut fuse::FileAttr, path: Option<&PathBuf>, uid: Option<unistd::Uid>,
+    gid: Option<unistd::Gid>) -> Result<(), nix::Error> {
+    if uid.is_none() && gid.is_none() {
+        return Ok(())
     }
 
-    if let Some(size) = delta.size {
+    let result = try_path(path, |p|
+        unistd::fchownat(None, p, uid, gid, unistd::FchownatFlags::NoFollowSymlink));
+    if result.is_ok() {
+        attr.uid = uid.map_or_else(|| attr.uid, u32::from);
+        attr.gid = uid.map_or_else(|| attr.gid, u32::from);
+    }
+    result
+}
+
+/// Helper function for `setattr` to apply only the atime and mtime changes.
+fn setattr_times(attr: &mut fuse::FileAttr, path: Option<&PathBuf>,
+    atime: Option<sys::time::TimeVal>, mtime: Option<sys::time::TimeVal>)
+    -> Result<(), nix::Error> {
+    if atime.is_none() && mtime.is_none() {
+        return Ok(());
+    }
+
+    let atime = atime.unwrap_or_else(|| conv::timespec_to_timeval(attr.atime));
+    let mtime = mtime.unwrap_or_else(|| conv::timespec_to_timeval(attr.mtime));
+    if attr.kind == fuse::FileType::Symlink {
+        // TODO(jmmv): Should use futimensat to avoid following symlinks but this function is
+        // not available on macOS El Capitan, which we currently require for CI builds.
+        warn!("Asked to update atime/mtime for symlink {:?} but following symlink instead", path);
+    }
+    let result = try_path(path, |p| sys::stat::utimes(p, &atime, &mtime));
+    if result.is_ok() {
+        attr.atime = conv::timeval_to_timespec(atime);
+        attr.mtime = conv::timeval_to_timespec(mtime);
+    }
+    result
+}
+
+/// Helper function for `setattr` to apply only the size changes.
+fn setattr_size(attr: &mut fuse::FileAttr, path: Option<&PathBuf>, size: Option<u64>)
+    -> Result<(), nix::Error> {
+    if size.is_none() {
+        return Ok(());
+    }
+    let size = size.unwrap();
+
+    let result = if size > ::std::i64::MAX as u64 {
+        warn!("truncate request got size {}, which is too large (exceeds i64's MAX)", size);
+        Err(nix::Error::invalid_argument())
+    } else {
+        try_path(path, |p| unistd::truncate(p, size as i64))
+    };
+    if result.is_ok() {
+        attr.size = size;
+    }
+    result
+}
+
+/// Updates the metadata of a file given a delta of attributes.
+///
+/// This is a helper function to implement `Node::setattr` for the various node types.
+///
+/// This tries to apply as many properties as possible in case of errors.  When errors occur,
+/// returns the first that was encountered.
+pub fn setattr(path: Option<&PathBuf>, attr: &fuse::FileAttr, delta: &AttrDelta)
+    -> Result<fuse::FileAttr, nix::Error> {
+    // Compute the potential new ctime for these updates.  We want to avoid picking a ctime that is
+    // larger than what the operations below can result in (so as to prevent a future getattr from
+    // moving the ctime back) which is tricky because we don't know the time resolution of the
+    // underlying file system.  Some, like HFS+, only have 1-second resolution... so pick that in
+    // the worst case.
+    //
+    // This is not perfectly accurate because we don't actually know what ctime the underlying file
+    // has gotten and thus a future getattr on it will cause the ctime to change.  But as long as we
+    // avoid going back on time, we can afford to do this -- which is a requirement for supporting
+    // setting attributes on deleted files.
+    //
+    // TODO(https://github.com/bazelbuild/sandboxfs/issues/43): Revisit this when we track
+    // ctimes purely on our own.
+    let updated_ctime = {
+        let mut now = time::now().to_timespec();
+        now.nsec = 0;
+        if attr.ctime > now {
+            attr.ctime
+        } else {
+            now
+        }
+    };
+
+    let mut new_attr = *attr;
+    // Intentionally using and() instead of and_then() to try to apply all changes but to only
+    // keep the first error result.
+    let result = Ok(())
+        .and(setattr_mode(&mut new_attr, path, delta.mode))
+        .and(setattr_owners(&mut new_attr, path, delta.uid, delta.gid))
+        .and(setattr_times(&mut new_attr, path, delta.atime, delta.mtime))
         // Updating the size only makes sense on files, but handling it here is much simpler than
         // doing so on a node type basis.  Plus, who knows, if the kernel asked us to change the
         // size of anything other than a file, maybe we have to obey and try to do it.
-        if size > ::std::i64::MAX as u64 {
-            warn!("truncate request got size {}, which is too large (exceeds i64's MAX)", size);
-            result = result.and(Err(nix::Error::invalid_argument()));
-        } else {
-            result = result.and(unistd::truncate(path, size as i64));
-        }
+        .and(setattr_size(&mut new_attr, path, delta.size));
+    if *attr != new_attr {
+        new_attr.ctime = updated_ctime;
     }
-
-    result
+    result.and(Ok(new_attr))
 }
 
 /// Abstract representation of an open file handle.

src/nodes/symlink.rs

@@ -99,7 +99,7 @@ impl Node for Symlink {
 
     fn setattr(&self, delta: &AttrDelta) -> NodeResult<fuse::FileAttr> {
         let mut state = self.state.lock().unwrap();
-        setattr(&state.underlying_path, &state.attr, delta)?;
-        Symlink::getattr_locked(self.inode, &mut state)
+        state.attr = setattr(Some(&state.underlying_path), &state.attr, delta)?;
+        Ok(state.attr)
     }
 }