image: replace Directory.entries Vec with BTreeMap

allisonkarlitskaya · cgwalters · commit 66f543f2eedf · 2025-04-08T10:51:20.000-04:00
Instead of trying to maintain our own sorted array of directory entries, just use a BTreeMap. There might be some very minor performance implications here one way or another, but this removes a lot of pointless code. Closes #92 Signed-off-by: Allison Karlitskaya <allison.karlitskaya@redhat.com>
diff --git a/src/dumpfile.rs b/src/dumpfile.rs
@@ -13,7 +13,7 @@ use rustix::fs::FileType;
 
 use crate::{
     fsverity::Sha256HashValue,
-    image::{DirEnt, Directory, FileSystem, Inode, Leaf, LeafContent, RegularFile, Stat},
+    image::{Directory, FileSystem, Inode, Leaf, LeafContent, RegularFile, Stat},
 };
 
 fn write_empty(writer: &mut impl fmt::Write) -> fmt::Result {
@@ -226,9 +226,9 @@ impl<'a, W: Write> DumpfileWriter<'a, W> {
     fn write_dir(&mut self, path: &mut PathBuf, dir: &Directory) -> Result<()> {
         // nlink is 2 + number of subdirectories
         // this is also true for the root dir since '..' is another self-ref
-        let nlink = dir.entries.iter().fold(2, |count, ent| {
+        let nlink = dir.entries.values().fold(2, |count, inode| {
             count + {
-                match ent.inode {
+                match inode {
                     Inode::Directory(..) => 1,
                     _ => 0,
                 }
@@ -239,8 +239,8 @@ impl<'a, W: Write> DumpfileWriter<'a, W> {
             write_directory(fmt, path, &dir.stat, nlink)
         })?;
 
-        for DirEnt { name, inode } in dir.entries.iter() {
-            path.push(name);
+        for (name, inode) in &dir.entries {
+            path.push(name.as_ref());
 
             match inode {
                 Inode::Directory(ref dir) => {
diff --git a/src/erofs/writer.rs b/src/erofs/writer.rs
@@ -485,13 +485,13 @@ impl<'a> InodeCollector<'a> {
 
         let mut entries = vec![];
 
-        for entry in &dir.entries {
-            let child = match &entry.inode {
+        for (name, inode) in &dir.entries {
+            let child = match inode {
                 image::Inode::Directory(dir) => self.collect_dir(dir, me),
                 image::Inode::Leaf(leaf) => self.collect_leaf(leaf),
             };
             entries.push(DirEnt {
-                name: entry.name.as_bytes(),
+                name: name.as_bytes(),
                 inode: child,
                 file_type: self.inodes[child].file_type(),
             });
diff --git a/src/fs.rs b/src/fs.rs
@@ -25,7 +25,7 @@ use zerocopy::IntoBytes;
 
 use crate::{
     fsverity::{digest::FsVerityHasher, Sha256HashValue},
-    image::{DirEnt, Directory, FileSystem, Inode, Leaf, LeafContent, RegularFile, Stat},
+    image::{Directory, FileSystem, Inode, Leaf, LeafContent, RegularFile, Stat},
     repository::Repository,
     selabel::selabel,
     util::proc_self_fd,
@@ -112,7 +112,7 @@ fn write_leaf(leaf: &Leaf, dirfd: &OwnedFd, name: &OsStr, repo: &Repository) ->
 }
 
 fn write_directory_contents(dir: &Directory, fd: &OwnedFd, repo: &Repository) -> Result<()> {
-    for DirEnt { name, inode } in &dir.entries {
+    for (name, inode) in &dir.entries {
         match inode {
             Inode::Directory(ref dir) => write_directory(dir, fd, name, repo),
             Inode::Leaf(ref leaf) => write_leaf(leaf, fd, name, repo),
diff --git a/src/image.rs b/src/image.rs
@@ -1,6 +1,5 @@
 use std::{
     cell::RefCell,
-    cmp::{Ord, Ordering},
     collections::BTreeMap,
     ffi::{OsStr, OsString},
     path::Path,
@@ -45,7 +44,7 @@ pub struct Leaf {
 #[derive(Debug)]
 pub struct Directory {
     pub stat: Stat,
-    pub entries: Vec<DirEnt>,
+    pub entries: BTreeMap<Box<OsStr>, Inode>,
 }
 
 #[derive(Debug)]
@@ -54,12 +53,6 @@ pub enum Inode {
     Leaf(Rc<Leaf>),
 }
 
-#[derive(Debug)]
-pub struct DirEnt {
-    pub name: OsString,
-    pub inode: Inode,
-}
-
 impl Inode {
     pub fn stat(&self) -> &Stat {
         match self {
@@ -73,99 +66,46 @@ impl Directory {
     pub fn new(stat: Stat) -> Self {
         Self {
             stat,
-            entries: vec![],
-        }
-    }
-
-    pub fn find_entry(&self, name: &OsStr) -> Result<usize, usize> {
-        // OCI layer tarballs are typically sorted, with the entries for a particular directory
-        // written out immediately after that directory was created.  That means that it's very
-        // likely that the thing we're looking for is either the last entry or the insertion point
-        // immediately following it.  Fast-path those cases by essentially unrolling the first
-        // iteration of the binary search.
-        if let Some(last_entry) = self.entries.last() {
-            match name.cmp(&last_entry.name) {
-                Ordering::Equal => Ok(self.entries.len() - 1), // the last item, indeed
-                Ordering::Greater => Err(self.entries.len()),  // need to append
-                Ordering::Less => self.entries.binary_search_by_key(&name, |e| &e.name),
-            }
-        } else {
-            Err(0)
+            entries: BTreeMap::new(),
         }
     }
 
     pub fn recurse(&mut self, name: impl AsRef<OsStr>) -> Result<&mut Directory> {
-        match self.find_entry(name.as_ref()) {
-            Ok(idx) => match &mut self.entries[idx].inode {
-                Inode::Directory(ref mut subdir) => Ok(subdir),
-                _ => bail!("Parent directory is not a directory"),
-            },
-            _ => bail!("Unable to find parent directory {:?}", name.as_ref()),
+        match self.entries.get_mut(name.as_ref()) {
+            Some(Inode::Directory(subdir)) => Ok(subdir),
+            Some(_) => bail!("Parent directory is not a directory"),
+            None => bail!("Unable to find parent directory {:?}", name.as_ref()),
         }
     }
 
     pub fn mkdir(&mut self, name: &OsStr, stat: Stat) {
-        match self.find_entry(name) {
-            Ok(idx) => match self.entries[idx].inode {
-                // Entry already exists, is a dir
-                Inode::Directory(ref mut dir) => {
-                    // update the stat, but don't drop the entries
-                    dir.stat = stat;
-                }
-                // Entry already exists, is not a dir
-                Inode::Leaf(..) => {
-                    todo!("Trying to replace non-dir with dir!");
-                }
-            },
+        match self.entries.get_mut(name) {
+            // Entry already exists, is a dir.  update the stat, but don't drop the entries
+            Some(Inode::Directory(dir)) => dir.stat = stat,
+            // Entry already exists, is not a dir
+            Some(Inode::Leaf(..)) => todo!("Trying to replace non-dir with dir!"),
             // Entry doesn't exist yet
-            Err(idx) => {
-                self.entries.insert(
-                    idx,
-                    DirEnt {
-                        name: OsString::from(name),
-                        inode: Inode::Directory(Box::new(Directory::new(stat))),
-                    },
-                );
+            None => {
+                self.entries
+                    .insert(name.into(), Inode::Directory(Directory::new(stat).into()));
             }
         }
     }
 
     pub fn insert(&mut self, name: &OsStr, inode: Inode) {
-        match self.find_entry(name) {
-            Ok(idx) => {
-                // found existing item
-                self.entries[idx].inode = inode;
-            }
-            Err(idx) => {
-                // need to add new item
-                self.entries.insert(
-                    idx,
-                    DirEnt {
-                        name: OsString::from(name),
-                        inode,
-                    },
-                );
-            }
-        }
+        self.entries.insert(name.into(), inode);
     }
 
     pub fn get_for_link(&self, name: &OsStr) -> Result<Rc<Leaf>> {
-        match self.find_entry(name) {
-            Ok(idx) => match self.entries[idx].inode {
-                Inode::Leaf(ref leaf) => Ok(Rc::clone(leaf)),
-                Inode::Directory(..) => bail!("Cannot hardlink to directory"),
-            },
-            _ => bail!("Attempt to hardlink to non-existent file"),
+        match self.entries.get(name) {
+            Some(Inode::Leaf(leaf)) => Ok(Rc::clone(leaf)),
+            Some(Inode::Directory(..)) => bail!("Cannot hardlink to directory"),
+            None => bail!("Attempt to hardlink to non-existent file"),
         }
     }
 
     pub fn remove(&mut self, name: &OsStr) {
-        match self.find_entry(name) {
-            Ok(idx) => {
-                self.entries.remove(idx);
-            }
-            _ => { /* not an error to remove an already-missing file */ }
-        }
+        self.entries.remove(name);
     }
 
     pub fn remove_all(&mut self) {
@@ -174,7 +114,7 @@ impl Directory {
 
     pub fn newest_file(&self) -> i64 {
         let mut newest = self.stat.st_mtim_sec;
-        for DirEnt { inode, .. } in &self.entries {
+        for inode in self.entries.values() {
             let mtime = match inode {
                 Inode::Leaf(ref leaf) => leaf.stat.st_mtim_sec,
                 Inode::Directory(ref dir) => dir.newest_file(),
diff --git a/src/selabel.rs b/src/selabel.rs
@@ -11,7 +11,7 @@ use anyhow::{bail, ensure, Context, Result};
 use regex_automata::{hybrid::dfa, util::syntax, Anchored, Input};
 
 use crate::{
-    image::{DirEnt, Directory, FileSystem, Inode, Leaf, LeafContent, RegularFile, Stat},
+    image::{Directory, FileSystem, Inode, Leaf, LeafContent, RegularFile, Stat},
     repository::Repository,
 };
 
@@ -110,11 +110,11 @@ pub fn openat<'a>(
     filename: impl AsRef<OsStr>,
     repo: &Repository,
 ) -> Result<Option<Box<dyn Read + 'a>>> {
-    let Ok(idx) = dir.find_entry(filename.as_ref()) else {
+    let Some(inode) = dir.entries.get(filename.as_ref()) else {
         return Ok(None);
     };
 
-    match &dir.entries[idx].inode {
+    match inode {
         Inode::Leaf(leaf) => match &leaf.content {
             LeafContent::Regular(RegularFile::Inline(data)) => Ok(Some(Box::new(&**data))),
             LeafContent::Regular(RegularFile::External(id, ..)) => {
@@ -231,8 +231,8 @@ fn relabel_inode(inode: &Inode, path: &mut PathBuf, policy: &mut Policy) {
 fn relabel_dir(dir: &Directory, path: &mut PathBuf, policy: &mut Policy) {
     relabel(&dir.stat, path, b'd', policy);
 
-    for DirEnt { name, inode } in dir.entries.iter() {
-        path.push(name);
+    for (name, inode) in &dir.entries {
+        path.push(name.as_ref());
         match policy.check_aliased(path.as_os_str()) {
             Some(original) => relabel_inode(inode, &mut PathBuf::from(original), policy),
             None => relabel_inode(inode, path, policy),
