tree: change approach to mtime of root directory

Instead of hardcoding a bunch of weird values for various parameters in
the root directory of the filesystem, handle the presence of an explicit
'stat' value more explicitly by keeping a separate boolean.

This fixes one of our testcases: in case of an empty filesystem the
sentinel value was slipping through which means we'd end up with an
unusual mtime on our root directory of -1, which showed up in the test
snapshots as 18446744073709551615 (0xffffffffffffffff).

Signed-off-by: Allison Karlitskaya <[email protected]>

Author: allisonkarlitskaya

src/fs.rs

@@ -1,6 +1,5 @@
 use std::{
     cell::RefCell,
-    cmp::max,
     collections::{BTreeMap, HashMap},
     ffi::{CStr, OsStr},
     fs::File,
@@ -145,7 +144,6 @@ pub fn write_to_path<ObjectID: FsVerityHashValue>(
 pub struct FilesystemReader<'repo, ObjectID: FsVerityHashValue> {
     repo: Option<&'repo Repository<ObjectID>>,
     inodes: HashMap<(u64, u64), Rc<Leaf<ObjectID>>>,
-    root_mtime: Option<i64>,
 }
 
 impl<ObjectID: FsVerityHashValue> FilesystemReader<'_, ObjectID> {
@@ -269,6 +267,7 @@ impl<ObjectID: FsVerityHashValue> FilesystemReader<'_, ObjectID> {
         &mut self,
         dirfd: impl AsFd,
         name: &OsStr,
+        stat_self: bool,
     ) -> Result<Directory<ObjectID>> {
         let fd = openat(
             dirfd,
@@ -277,8 +276,12 @@ impl<ObjectID: FsVerityHashValue> FilesystemReader<'_, ObjectID> {
             Mode::empty(),
         )?;
 
-        let (_, stat) = Self::stat(&fd, FileType::Directory)?;
-        let mut directory = Directory::new(stat);
+        let mut directory = if stat_self {
+            let (_, stat) = Self::stat(&fd, FileType::Directory)?;
+            Directory::new(stat)
+        } else {
+            Directory::default()
+        };
 
         for item in Dir::read_from(&fd)? {
             let entry = item?;
@@ -289,7 +292,6 @@ impl<ObjectID: FsVerityHashValue> FilesystemReader<'_, ObjectID> {
             }
 
             let inode = self.read_inode(&fd, name, entry.file_type())?;
-            self.root_mtime = max(self.root_mtime, Some(inode.stat().st_mtim_sec));
             directory.insert(name, inode);
         }
 
@@ -303,7 +305,7 @@ impl<ObjectID: FsVerityHashValue> FilesystemReader<'_, ObjectID> {
         ifmt: FileType,
     ) -> Result<Inode<ObjectID>> {
         if ifmt == FileType::Directory {
-            let dir = self.read_directory(dirfd, name)?;
+            let dir = self.read_directory(dirfd, name, true)?;
             Ok(Inode::Directory(Box::new(dir)))
         } else {
             let leaf = self.read_leaf(dirfd, name, ifmt)?;
@@ -315,19 +317,20 @@ impl<ObjectID: FsVerityHashValue> FilesystemReader<'_, ObjectID> {
 pub fn read_from_path<ObjectID: FsVerityHashValue>(
     path: &Path,
     repo: Option<&Repository<ObjectID>>,
+    stat_root: bool,
 ) -> Result<FileSystem<ObjectID>> {
     let mut reader = FilesystemReader {
         repo,
         inodes: HashMap::new(),
-        root_mtime: None,
     };
+
+    let root = reader.read_directory(CWD, path.as_os_str(), stat_root)?;
+
     let mut fs = FileSystem {
-        root: reader.read_directory(CWD, path.as_os_str())?,
+        root,
+        have_root_stat: stat_root,
     };
 
-    // A filesystem with no files ends up in the 1970s...
-    fs.root.stat.st_mtim_sec = reader.root_mtime.unwrap_or(0);
-
     // We can only relabel if we have the repo because we need to read the config and policy files
     if let Some(repo) = repo {
         selabel(&mut fs, repo)?;
@@ -340,7 +343,7 @@ pub fn create_image<ObjectID: FsVerityHashValue>(
     path: &Path,
     repo: Option<&Repository<ObjectID>>,
 ) -> Result<ObjectID> {
-    let fs = read_from_path(path, repo)?;
+    let fs = read_from_path(path, repo, false)?;
     let image = crate::erofs::writer::mkfs_erofs(&fs);
     if let Some(repo) = repo {
         Ok(repo.write_image(None, &image)?)
@@ -350,7 +353,7 @@ pub fn create_image<ObjectID: FsVerityHashValue>(
 }
 
 pub fn create_dumpfile<ObjectID: FsVerityHashValue>(path: &Path) -> Result<()> {
-    let fs = read_from_path::<ObjectID>(path, None)?;
+    let fs = read_from_path::<ObjectID>(path, None, false)?;
     super::dumpfile::write_dumpfile(&mut std::io::stdout(), &fs)
 }
 

src/oci/image.rs

@@ -53,7 +53,7 @@ pub fn compose_filesystem<ObjectID: FsVerityHashValue>(
     repo: &Repository<ObjectID>,
     layers: &[String],
 ) -> Result<FileSystem<ObjectID>> {
-    let mut filesystem = FileSystem::<ObjectID>::new();
+    let mut filesystem = FileSystem::default();
 
     for layer in layers {
         let mut split_stream = repo.open_stream(layer, None)?;
@@ -63,7 +63,7 @@ pub fn compose_filesystem<ObjectID: FsVerityHashValue>(
     }
 
     selabel(&mut filesystem, repo)?;
-    filesystem.done();
+    filesystem.ensure_root_stat();
 
     Ok(filesystem)
 }
@@ -81,7 +81,7 @@ pub fn create_image<ObjectID: FsVerityHashValue>(
     name: Option<&str>,
     verity: Option<&ObjectID>,
 ) -> Result<ObjectID> {
-    let mut filesystem = FileSystem::new();
+    let mut filesystem = FileSystem::default();
 
     let mut config_stream = repo.open_stream(config, verity)?;
     let config = ImageConfiguration::from_reader(&mut config_stream)?;
@@ -97,7 +97,7 @@ pub fn create_image<ObjectID: FsVerityHashValue>(
     }
 
     selabel(&mut filesystem, repo)?;
-    filesystem.done();
+    filesystem.ensure_root_stat();
 
     let erofs = mkfs_erofs(&filesystem);
     repo.write_image(name, &erofs)
@@ -155,7 +155,7 @@ mod test {
 
     #[test]
     fn test_process_entry() -> Result<()> {
-        let mut fs = FileSystem::<Sha256HashValue>::new();
+        let mut fs = FileSystem::<Sha256HashValue>::default();
 
         // both with and without leading slash should be supported
         process_entry(&mut fs, dir_entry("/a"))?;

src/oci/mod.rs

@@ -391,7 +391,7 @@ pub fn prepare_boot<ObjectID: FsVerityHashValue>(
         .with_context(|| "Attachments layer {layer} is not connected to image {name}")?;
 
     // read the layer into a FileSystem object
-    let mut filesystem = crate::tree::FileSystem::new();
+    let mut filesystem = crate::tree::FileSystem::default();
     let mut split_stream = repo.open_stream(&hex::encode(layer_sha256), Some(layer_verity))?;
     while let Some(entry) = tar::get_entry(&mut split_stream)? {
         image::process_entry(&mut filesystem, entry)?;

src/tree.rs

@@ -76,6 +76,22 @@ impl<ObjectID: FsVerityHashValue> Inode<ObjectID> {
     }
 }
 
+// For some reason #[derive(Default)] doesn't work, so let's DIY
+impl<ObjectID: FsVerityHashValue> Default for Directory<ObjectID> {
+    fn default() -> Self {
+        Self {
+            stat: Stat {
+                st_uid: 0,
+                st_gid: 0,
+                st_mode: 0o555,
+                st_mtim_sec: 0,
+                xattrs: Default::default(),
+            },
+            entries: BTreeMap::default(),
+        }
+    }
+}
+
 impl<ObjectID: FsVerityHashValue> Directory<ObjectID> {
     pub fn new(stat: Stat) -> Self {
         Self {
@@ -97,7 +113,7 @@ impl<ObjectID: FsVerityHashValue> Directory<ObjectID> {
     ///
     /// ```
     /// use composefs::{tree::FileSystem, fsverity::Sha256HashValue};
-    /// let fs = FileSystem::<Sha256HashValue>::new();
+    /// let fs = FileSystem::<Sha256HashValue>::default();
     ///
     /// // populate the fs...
     ///
@@ -358,7 +374,7 @@ impl<ObjectID: FsVerityHashValue> Directory<ObjectID> {
         self.entries.clear();
     }
 
-    fn newest_file(&self) -> i64 {
+    pub fn newest_file(&self) -> i64 {
         let mut newest = self.stat.st_mtim_sec;
         for inode in self.entries.values() {
             let mtime = match inode {
@@ -376,43 +392,28 @@ impl<ObjectID: FsVerityHashValue> Directory<ObjectID> {
 #[derive(Debug)]
 pub struct FileSystem<ObjectID: FsVerityHashValue> {
     pub root: Directory<ObjectID>,
+    pub have_root_stat: bool,
 }
 
 impl<ObjectID: FsVerityHashValue> Default for FileSystem<ObjectID> {
     fn default() -> Self {
-        Self::new()
+        Self {
+            root: Directory::default(),
+            have_root_stat: false,
+        }
     }
 }
 
 impl<ObjectID: FsVerityHashValue> FileSystem<ObjectID> {
-    pub fn new() -> Self {
-        Self {
-            root: Directory::new(Stat {
-                st_mode: u32::MAX, // assigned later
-                st_uid: u32::MAX,  // assigned later
-                st_gid: u32::MAX,  // assigned later
-                st_mtim_sec: -1,   // assigned later
-                xattrs: RefCell::new(BTreeMap::new()),
-            }),
-        }
+    pub fn set_root_stat(&mut self, stat: Stat) {
+        self.have_root_stat = true;
+        self.root.stat = stat;
     }
 
-    pub fn done(&mut self) {
-        // We need to look at the root entry and deal with the "assign later" fields
-        let stat = &mut self.root.stat;
-
-        if stat.st_mode == u32::MAX {
-            stat.st_mode = 0o555;
-        }
-        if stat.st_uid == u32::MAX {
-            stat.st_uid = 0;
-        }
-        if stat.st_gid == u32::MAX {
-            stat.st_gid = 0;
-        }
-        if stat.st_mtim_sec == -1 {
-            // write this in full to avoid annoying the borrow checker
+    pub fn ensure_root_stat(&mut self) {
+        if !self.have_root_stat {
             self.root.stat.st_mtim_sec = self.root.newest_file();
+            self.have_root_stat = true;
         }
     }
 }

tests/mkfs.rs

@@ -18,7 +18,7 @@ use composefs::{
 };
 
 fn debug_fs(mut fs: FileSystem<impl FsVerityHashValue>) -> String {
-    fs.done();
+    fs.ensure_root_stat();
     let image = mkfs_erofs(&fs);
     let mut output = vec![];
     debug_img(&mut output, &image).unwrap();
@@ -29,7 +29,7 @@ fn empty(_fs: &mut FileSystem<impl FsVerityHashValue>) {}
 
 #[test]
 fn test_empty() {
-    let mut fs = FileSystem::<Sha256HashValue>::new();
+    let mut fs = FileSystem::<Sha256HashValue>::default();
     empty(&mut fs);
     insta::assert_snapshot!(debug_fs(fs));
 }
@@ -82,16 +82,16 @@ fn simple(fs: &mut FileSystem<Sha256HashValue>) {
 
 #[test]
 fn test_simple() {
-    let mut fs = FileSystem::<Sha256HashValue>::new();
+    let mut fs = FileSystem::<Sha256HashValue>::default();
     simple(&mut fs);
     insta::assert_snapshot!(debug_fs(fs));
 }
 
 fn foreach_case(f: fn(&FileSystem<Sha256HashValue>)) {
     for case in [empty, simple] {
-        let mut fs = FileSystem::new();
+        let mut fs = FileSystem::default();
         case(&mut fs);
-        fs.done();
+        fs.ensure_root_stat();
         f(&fs);
     }
 }

tests/snapshots/mkfs__empty.snap

@@ -26,7 +26,6 @@ snapshot_kind: text
       +4     mode: 0040555 (directory)
       +8     size: U64(27)
      +14     ino: U32(36)
-     +20     mtime: U64(18446744073709551615)
      +2c     nlink: U32(2)
      +40 --- inline directory entries ---
       +0     inode_offset: U64(36)