More improvements

Still need to traverse the free list, as this reports the blocks on the
free list as missing

fwdctl/src/check.rs

@@ -4,7 +4,8 @@
 use clap::Args;
 use log::warn;
 use std::collections::BTreeMap;
-use std::io::Error;
+use std::io::{Error, ErrorKind};
+use std::ops::Bound;
 use std::str;
 use std::sync::Arc;
 
@@ -19,7 +20,7 @@ pub struct Options {
         long,
         required = false,
         value_name = "DB_NAME",
-        default_value_t = String::from("firewood"),
+        default_value_t = String::from("firewood.db"),
         help = "Name of the database"
     )]
     pub db: String,
@@ -46,8 +47,21 @@ pub(super) async fn run(opts: &Options) -> Result<(), api::Error> {
 
     visitor(rev.clone(), addr, &mut allocated)?;
 
+    let mut expected = 2048;
     for (addr, size) in allocated.iter() {
-        println!("{:?} {}", addr, size);
+        match addr.get().cmp(&expected) {
+            std::cmp::Ordering::Less => {
+                warn!(
+                    "Node at {:?} is before the expected address {}",
+                    addr, expected
+                );
+            }
+            std::cmp::Ordering::Greater => {
+                warn!("{} bytes missing at {}", addr.get() - expected, expected);
+            }
+            std::cmp::Ordering::Equal => {}
+        }
+        expected = addr.get() + rev.size_from_area_index(*size);
     }
 
     Ok(())
@@ -58,28 +72,58 @@ fn visitor<T: ReadableStorage>(
     addr: LinearAddress,
     allocated: &mut BTreeMap<LinearAddress, u8>,
 ) -> Result<(), Error> {
+    // find the node before this one, check if it overlaps
+    if let Some((found_addr, found_size)) = allocated
+        .range((Bound::Unbounded, Bound::Included(addr)))
+        .next_back()
+    {
+        match found_addr
+            .get()
+            .checked_add(rev.size_from_area_index(*found_size))
+        {
+            None => warn!("Node at {:?} overflows a u64", found_addr),
+            Some(end) => {
+                if end > addr.get() {
+                    warn!(
+                        "Node at {:?} overlaps with another node at {:?} (size: {})",
+                        addr, found_addr, found_size
+                    );
+                    return Err(Error::new(ErrorKind::Other, "Overlapping nodes"));
+                }
+            }
+        }
+    }
+    if addr.get() > rev.header().size() {
+        warn!(
+            "Node at {:?} starts past the database high water mark",
+            addr
+        );
+        return Err(Error::new(ErrorKind::Other, "Node overflows database"));
+    }
+
     let (node, size) = rev.uncached_read_node_and_size(addr)?;
-    if let Some(duplicate) = allocated.insert(addr, size) {
-        warn!("Duplicate allocation at {:?} (size: {})", addr, duplicate);
+    if addr.get() + rev.size_from_area_index(size) > rev.header().size() {
+        warn!(
+            "Node at {:?} extends past the database high water mark",
+            addr
+        );
+        return Err(Error::new(ErrorKind::Other, "Node overflows database"));
     }
 
-    println!("{:?} {:?}", addr, node);
-
-    match node.as_ref() {
-        Node::Branch(branch) => {
-            for child in branch.children.iter() {
-                match child {
-                    None => {}
-                    Some(child) => match child {
-                        storage::Child::Node(_) => unreachable!(),
-                        storage::Child::AddressWithHash(addr, _hash) => {
-                            visitor(rev.clone(), *addr, allocated)?;
-                        }
-                    },
-                }
+    allocated.insert(addr, size);
+
+    if let Node::Branch(branch) = node.as_ref() {
+        for child in branch.children.iter() {
+            match child {
+                None => {}
+                Some(child) => match child {
+                    storage::Child::Node(_) => unreachable!(),
+                    storage::Child::AddressWithHash(addr, _hash) => {
+                        visitor(rev.clone(), *addr, allocated)?;
+                    }
+                },
             }
         }
-        Node::Leaf(_leaf) => {}
     }
     Ok(())
 }

fwdctl/src/graph.rs

@@ -18,7 +18,7 @@ pub struct Options {
 }
 
 pub(super) async fn run(opts: &Options) -> Result<(), api::Error> {
-    log::debug!("dump database {:?}", opts);
+    log::debug!("graph database {:?}", opts);
     let cfg = DbConfig::builder().truncate(false);
 
     let db = Db::new(opts.db.clone(), cfg.build()).await?;

fwdctl/src/main.rs

@@ -50,8 +50,6 @@ enum Commands {
     Check(check::Options),
     /// Produce a dot file of the database
     Graph(graph::Options),
-    /// Check a database
-    Check(check::Options),
 }
 
 #[tokio::main]

storage/src/nodestore.rs

@@ -245,23 +245,24 @@ impl<T: ReadInMemoryNode, S: ReadableStorage> NodeStore<T, S> {
     pub fn uncached_read_node_and_size(
         &self,
         addr: LinearAddress,
-    ) -> Result<(Arc<Node>, u8), Error> {
+    ) -> Result<(SharedNode, u8), Error> {
         let mut area_stream = self.storage.stream_from(addr.get())?;
         let mut size = [0u8];
         area_stream.read_exact(&mut size)?;
-        let area: Area<Node, FreeArea> = DefaultOptions::new()
-            .deserialize_from(area_stream)
-            .map_err(|e| Error::new(ErrorKind::InvalidData, e))?;
-
-        let node = match area {
-            Area::Node(node) => Ok(node.into()),
-            Area::Free(_) => Err(Error::new(
-                ErrorKind::InvalidData,
-                "Attempted to read a freed area",
-            )),
-        }?;
+        self.storage.stream_from(addr.get() + 1)?;
+        let node: SharedNode = Node::from_reader(area_stream)?.into();
         Ok((node, size[0]))
     }
+
+    /// Get a reference to the header of this nodestore
+    pub fn header(&self) -> &NodeStoreHeader {
+        &self.header
+    }
+
+    /// Get the size of an area index (used by the checker)
+    pub fn size_from_area_index(&self, index: AreaIndex) -> u64 {
+        AREA_SIZES[index as usize]
+    }
 }
 
 impl<S: ReadableStorage> NodeStore<Committed, S> {
@@ -347,20 +348,17 @@ impl Parentable for Arc<ImmutableProposal> {
 impl<S> NodeStore<Arc<ImmutableProposal>, S> {
     /// When an immutable proposal commits, we need to reparent any proposal that
     /// has the committed proposal as it's parent
-    pub fn commit_reparent(&self, other: &Arc<NodeStore<Arc<ImmutableProposal>, S>>) -> bool {
+    pub fn commit_reparent(&self, other: &Arc<NodeStore<Arc<ImmutableProposal>, S>>) {
         match *other.kind.parent.load() {
             NodeStoreParent::Proposed(ref parent) => {
                 if Arc::ptr_eq(&self.kind, parent) {
                     other
                         .kind
                         .parent
                         .store(NodeStoreParent::Committed(self.kind.root_hash()).into());
-                    true
-                } else {
-                    false
                 }
             }
-            NodeStoreParent::Committed(_) => false,
+            NodeStoreParent::Committed(_) => {}
         }
     }
 }
@@ -624,7 +622,7 @@ pub type FreeLists = [Option<LinearAddress>; NUM_AREA_SIZES];
 /// The [NodeStoreHeader] is at the start of the ReadableStorage.
 #[derive(Copy, Debug, PartialEq, Eq, Serialize, Deserialize, Clone, NoUninit, AnyBitPattern)]
 #[repr(C)]
-struct NodeStoreHeader {
+pub struct NodeStoreHeader {
     /// Identifies the version of firewood used to create this [NodeStore].
     version: Version,
     /// always "1"; verifies endianness
@@ -656,6 +654,11 @@ impl NodeStoreHeader {
             free_lists: Default::default(),
         }
     }
+
+    // return the size of this nodestore
+    pub fn size(&self) -> u64 {
+        self.size
+    }
 }
 
 /// A [FreeArea] is stored at the start of the area that contained a node that