Remove new_from_path(). Rename new_from_file() to new(). Added more t…

…ests.
anza-xyz · Mar 13, 2024 · 0c298f5 · 0c298f5
1 parent bdae394
commit 0c298f5
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 29 deletions.
diff --git a/accounts-db/src/tiered_storage/file.rs b/accounts-db/src/tiered_storage/file.rs
@@ -10,7 +10,7 @@ use {
 };
 
 /// The ending 8 bytes of a valid tiered account storage file.
-pub const FOOTER_MAGIC_NUMBER: u64 = 0x502A2AB5; // SOLALABS -> SOLANA LABS
+pub const FILE_MAGIC_NUMBER: u64 = 0xA72A2AB5; // ANZALABS
 
 #[derive(Debug, PartialEq, Eq, Clone, Copy, Pod, Zeroable)]
 #[repr(C)]
@@ -21,7 +21,7 @@ const _: () = assert!(std::mem::size_of::<TieredStorageMagicNumber>() == 8);
 
 impl Default for TieredStorageMagicNumber {
     fn default() -> Self {
-        Self(FOOTER_MAGIC_NUMBER)
+        Self(FILE_MAGIC_NUMBER)
     }
 }
 
@@ -136,19 +136,70 @@ impl TieredStorageFile {
 
 #[cfg(test)]
 mod tests {
-    use {super::TieredStorageFile, tempfile::TempDir};
+    use {
+        crate::tiered_storage::{
+            error::TieredStorageError,
+            file::{TieredStorageFile, FILE_MAGIC_NUMBER},
+        },
+        std::{fs::OpenOptions, path::Path},
+        tempfile::TempDir,
+    };
+
+    fn generate_test_file_with_number(path: impl AsRef<Path>, number: u64) {
+        // Generate a new temp path that is guaranteed to NOT already have a file.
+        let file = TieredStorageFile::new_writable(&path).unwrap();
+        file.write_pod(&number).unwrap();
+    }
 
     #[test]
-    #[should_panic(expected = "MagicNumberMismatch")]
     fn test_magic_number() {
-        // Generate a new temp path that is guaranteed to NOT already have a file.
         let temp_dir = TempDir::new().unwrap();
-        let path = temp_dir.path().join("test_magic_number");
-        {
-            let file = TieredStorageFile::new_writable(&path).unwrap();
-            let unmagic_number: u64 = 0x12345678;
-            file.write_pod(&unmagic_number).unwrap();
-        }
+        let path = temp_dir.path().join("test_check_magic_number");
+        generate_test_file_with_number(&path, FILE_MAGIC_NUMBER);
+        let file = TieredStorageFile(
+            OpenOptions::new()
+                .read(true)
+                .create(false)
+                .open(&path)
+                .unwrap(),
+        );
+        assert!(matches!(file.check_magic_number(), Ok(())));
+    }
+
+    #[test]
+    fn test_check_magic_number_mismatch() {
+        let temp_dir = TempDir::new().unwrap();
+        let path = temp_dir.path().join("test_check_magic_number_mismatch");
+        generate_test_file_with_number(&path, !FILE_MAGIC_NUMBER);
+        let file = TieredStorageFile(
+            OpenOptions::new()
+                .read(true)
+                .create(false)
+                .open(&path)
+                .unwrap(),
+        );
+        assert!(matches!(
+            file.check_magic_number(),
+            Err(TieredStorageError::MagicNumberMismatch(_, _))
+        ));
+    }
+
+    #[test]
+    fn test_new_readonly() {
+        let temp_dir = TempDir::new().unwrap();
+        let path = temp_dir.path().join("test_new_readonly");
+        generate_test_file_with_number(&path, FILE_MAGIC_NUMBER);
         TieredStorageFile::new_readonly(&path).unwrap();
     }
+
+    #[test]
+    fn test_new_readonly_mismatch() {
+        let temp_dir = TempDir::new().unwrap();
+        let path = temp_dir.path().join("test_new_readonly_mismatch");
+        generate_test_file_with_number(&path, !FILE_MAGIC_NUMBER);
+        assert!(matches!(
+            TieredStorageFile::new_readonly(&path),
+            Err(TieredStorageError::MagicNumberMismatch(_, _))
+        ));
+    }
 }
diff --git a/accounts-db/src/tiered_storage/footer.rs b/accounts-db/src/tiered_storage/footer.rs
@@ -117,7 +117,7 @@ pub struct TieredStorageFooter {
     /// The size of the footer including the magic number.
     pub footer_size: u64,
     // This field is persisted in the storage but not in this struct.
-    // The number should match FOOTER_MAGIC_NUMBER.
+    // The number should match FILE_MAGIC_NUMBER.
     // pub magic_number: u64,
 }
 

diff --git a/accounts-db/src/tiered_storage/hot.rs b/accounts-db/src/tiered_storage/hot.rs
@@ -346,13 +346,7 @@ pub struct HotStorageReader {
 }
 
 impl HotStorageReader {
-    /// Constructs a HotStorageReader from the specified path.
-    pub fn new_from_path(path: impl AsRef<Path>) -> TieredStorageResult<Self> {
-        let file = TieredStorageFile::new_readonly(&path)?;
-        Self::new_from_file(file)
-    }
-
-    pub fn new_from_file(file: TieredStorageFile) -> TieredStorageResult<Self> {
+    pub fn new(file: TieredStorageFile) -> TieredStorageResult<Self> {
         let mmap = unsafe { MmapOptions::new().map(&file.0)? };
         // Here we are copying the footer, as accessing any data in a
         // TieredStorage instance requires accessing its Footer.
@@ -903,7 +897,8 @@ pub mod tests {
         // Reopen the same storage, and expect the persisted footer is
         // the same as what we have written.
         {
-            let hot_storage = HotStorageReader::new_from_path(&path).unwrap();
+            let file = TieredStorageFile::new_readonly(&path).unwrap();
+            let hot_storage = HotStorageReader::new(file).unwrap();
             assert_eq!(expected_footer, *hot_storage.footer());
         }
     }
@@ -949,7 +944,8 @@ pub mod tests {
             footer.write_footer_block(&file).unwrap();
         }
 
-        let hot_storage = HotStorageReader::new_from_path(&path).unwrap();
+        let file = TieredStorageFile::new_readonly(&path).unwrap();
+        let hot_storage = HotStorageReader::new(file).unwrap();
 
         for (offset, expected_meta) in account_offsets.iter().zip(hot_account_metas.iter()) {
             let meta = hot_storage.get_account_meta_from_offset(*offset).unwrap();
@@ -979,7 +975,8 @@ pub mod tests {
             footer.write_footer_block(&file).unwrap();
         }
 
-        let hot_storage = HotStorageReader::new_from_path(&path).unwrap();
+        let file = TieredStorageFile::new_readonly(&path).unwrap();
+        let hot_storage = HotStorageReader::new(file).unwrap();
         let offset = HotAccountOffset::new(footer.index_block_offset as usize).unwrap();
         // Read from index_block_offset, which offset doesn't belong to
         // account blocks.  Expect assert failure here
@@ -1030,7 +1027,8 @@ pub mod tests {
             footer.write_footer_block(&file).unwrap();
         }
 
-        let hot_storage = HotStorageReader::new_from_path(&path).unwrap();
+        let file = TieredStorageFile::new_readonly(&path).unwrap();
+        let hot_storage = HotStorageReader::new(file).unwrap();
         for (i, index_writer_entry) in index_writer_entries.iter().enumerate() {
             let account_offset = hot_storage
                 .get_account_offset(IndexOffset(i as u32))
@@ -1079,7 +1077,8 @@ pub mod tests {
             footer.write_footer_block(&file).unwrap();
         }
 
-        let hot_storage = HotStorageReader::new_from_path(&path).unwrap();
+        let file = TieredStorageFile::new_readonly(&path).unwrap();
+        let hot_storage = HotStorageReader::new(file).unwrap();
         for (i, address) in addresses.iter().enumerate() {
             assert_eq!(
                 hot_storage
@@ -1153,7 +1152,8 @@ pub mod tests {
             footer.write_footer_block(&file).unwrap();
         }
 
-        let hot_storage = HotStorageReader::new_from_path(&path).unwrap();
+        let file = TieredStorageFile::new_readonly(&path).unwrap();
+        let hot_storage = HotStorageReader::new(file).unwrap();
 
         // First, verify whether we can find the expected owners.
         let mut owner_candidates = owner_addresses.clone();
@@ -1285,7 +1285,8 @@ pub mod tests {
             footer.write_footer_block(&file).unwrap();
         }
 
-        let hot_storage = HotStorageReader::new_from_path(&path).unwrap();
+        let file = TieredStorageFile::new_readonly(&path).unwrap();
+        let hot_storage = HotStorageReader::new(file).unwrap();
 
         for i in 0..NUM_ACCOUNTS {
             let (stored_meta, next) = hot_storage
@@ -1366,10 +1367,10 @@ pub mod tests {
             writer.write_accounts(&storable_accounts, 0).unwrap()
         };
 
-        let hot_storage = HotStorageReader::new_from_path(&path).unwrap();
+        let file = TieredStorageFile::new_readonly(&path).unwrap();
+        let hot_storage = HotStorageReader::new(file).unwrap();
 
         let num_accounts = account_data_sizes.len();
-
         for i in 0..num_accounts {
             let (stored_meta, next) = hot_storage
                 .get_account(IndexOffset(i as u32))

diff --git a/accounts-db/src/tiered_storage/readable.rs b/accounts-db/src/tiered_storage/readable.rs
@@ -26,7 +26,7 @@ impl TieredStorageReader {
         let file = TieredStorageFile::new_readonly(&path)?;
         let footer = TieredStorageFooter::new_from_footer_block(&file)?;
         match footer.account_meta_format {
-            AccountMetaFormat::Hot => Ok(Self::Hot(HotStorageReader::new_from_file(file)?)),
+            AccountMetaFormat::Hot => Ok(Self::Hot(HotStorageReader::new(file)?)),
         }
     }