From db762e01872f5822cc6d29d3dc3b0124e6ea766d Mon Sep 17 00:00:00 2001 From: Janis Date: Wed, 5 Apr 2023 21:07:35 +0200 Subject: [PATCH] cleanup + switching to entry instead of find_key() --- btrfs/src/lib.rs | 2 +- btrfs/src/v2/mod.rs | 283 +---------------------------------------- btrfs/src/v2/tree.rs | 8 +- btrfs/src/v2/volume.rs | 68 +++++----- 4 files changed, 45 insertions(+), 316 deletions(-) diff --git a/btrfs/src/lib.rs b/btrfs/src/lib.rs index 9b794db..38ab3b7 100644 --- a/btrfs/src/lib.rs +++ b/btrfs/src/lib.rs @@ -36,7 +36,7 @@ pub mod std_io { impl VolumeIo for T { fn read(&mut self, dst: &mut [u8], address: u64) -> Result<(), Error> { self.seek(std::io::SeekFrom::Start(address)) - .map_err(|a| Error::ReadFailed)?; + .map_err(|_| Error::ReadFailed)?; self.read_exact(dst).map_err(|_| Error::ReadFailed) } diff --git a/btrfs/src/v2/mod.rs b/btrfs/src/v2/mod.rs index b3a40ad..a868019 100644 --- a/btrfs/src/v2/mod.rs +++ b/btrfs/src/v2/mod.rs @@ -1,11 +1,3 @@ -use core::{ - cmp::Ordering, - fmt::Debug, - ops::{Bound, RangeBounds}, -}; - -use crate::Error; - pub mod error { use thiserror::Error; @@ -63,7 +55,8 @@ pub trait Read { impl Read for std::fs::File { fn read(&self, dst: &mut [u8], address: u64) -> error::Result<()> { use std::os::unix::prelude::FileExt; - self.read_at(dst, address).map_err(|_| Error::ReadFailed)?; + self.read_at(dst, address) + .map_err(|_| error::Error::ReadFailed)?; Ok(()) } } @@ -71,275 +64,3 @@ impl Read for std::fs::File { pub mod file; pub mod tree; pub mod volume; - -pub fn cmp_start_bound( - rhs: &core::ops::Bound, - lhs: &core::ops::Bound, -) -> Option { - match rhs { - core::ops::Bound::Included(r) => match lhs { - core::ops::Bound::Included(l) => r.partial_cmp(&l), - core::ops::Bound::Excluded(l) => match r.partial_cmp(&l) { - Some(core::cmp::Ordering::Equal) => Some(core::cmp::Ordering::Less), - i => i, - }, - core::ops::Bound::Unbounded => Some(core::cmp::Ordering::Greater), - }, - core::ops::Bound::Excluded(r) => match lhs { - core::ops::Bound::Excluded(l) => r.partial_cmp(&l), - core::ops::Bound::Included(l) => match r.partial_cmp(&l) { - Some(core::cmp::Ordering::Equal) => Some(core::cmp::Ordering::Greater), - i => i, - }, - core::ops::Bound::Unbounded => Some(core::cmp::Ordering::Greater), - }, - core::ops::Bound::Unbounded => match lhs { - core::ops::Bound::Unbounded => Some(core::cmp::Ordering::Equal), - _ => Some(core::cmp::Ordering::Less), - }, - } -} - -pub fn cmp_end_bound( - rhs: &core::ops::Bound, - lhs: &core::ops::Bound, -) -> Option { - match rhs { - core::ops::Bound::Included(r) => match lhs { - core::ops::Bound::Included(l) => r.partial_cmp(&l), - core::ops::Bound::Excluded(l) => match r.partial_cmp(&l) { - Some(core::cmp::Ordering::Equal) => Some(core::cmp::Ordering::Greater), - i => i, - }, - core::ops::Bound::Unbounded => Some(core::cmp::Ordering::Less), - }, - core::ops::Bound::Excluded(r) => match lhs { - core::ops::Bound::Excluded(l) => r.partial_cmp(&l), - core::ops::Bound::Included(l) => match r.partial_cmp(&l) { - Some(core::cmp::Ordering::Equal) => Some(core::cmp::Ordering::Greater), - i => i, - }, - core::ops::Bound::Unbounded => Some(core::cmp::Ordering::Less), - }, - core::ops::Bound::Unbounded => match lhs { - core::ops::Bound::Unbounded => Some(core::cmp::Ordering::Equal), - _ => Some(core::cmp::Ordering::Greater), - }, - } -} - -#[derive(Debug, PartialEq, Eq)] -struct StartBound(Bound); -impl PartialOrd for StartBound { - fn partial_cmp(&self, other: &Self) -> Option { - cmp_start_bound(&self.0, &other.0) - } -} -impl Ord for StartBound { - fn cmp(&self, other: &Self) -> core::cmp::Ordering { - self.partial_cmp(other).unwrap() - } -} - -#[derive(Debug, PartialEq, Eq)] -struct EndBound(Bound); - -impl PartialOrd for EndBound { - fn partial_cmp(&self, other: &Self) -> Option { - cmp_end_bound(&self.0, &other.0) - } -} -impl Ord for EndBound { - fn cmp(&self, other: &Self) -> core::cmp::Ordering { - // safety: partial_cmp only returns None when returning T::partial_cmp - self.partial_cmp(other).unwrap() - } -} - -pub fn range_countains_bound>( - range: R, - bound: Bound<&T>, -) -> bool { - let start = &StartBound(bound); - let end = &EndBound(bound); - let r_start = &StartBound(range.start_bound()); - let r_end = &EndBound(range.end_bound()); - log::info!( - "start: {start:?} <=> {r_start:?}: {:?} {:?}", - start.cmp(r_start), - r_start.cmp(start) - ); - log::info!( - "end: {end:?} <=> {r_end:?}: {:?} {:?}", - end.cmp(r_end), - r_end.cmp(end) - ); - (start.cmp(r_start).is_ge() || r_start.cmp(start).is_le()) - && (end.cmp(r_end).is_ge() || r_end.cmp(end).is_le()) -} - -#[cfg(test)] -mod bound_ord_tests { - use core::{cmp::Ordering, ops::RangeBounds}; - - use super::*; - use test_log::test; - - #[test] - fn start_bound_ord() { - assert_eq!( - cmp_start_bound(&Bound::Unbounded, &Bound::Included(0)), - Some(Ordering::Less) - ); - assert_eq!( - cmp_start_bound::(&Bound::Unbounded, &Bound::Unbounded), - Some(Ordering::Equal) - ); - assert_eq!( - cmp_start_bound(&Bound::Included(0), &Bound::Included(0)), - Some(Ordering::Equal) - ); - assert_eq!( - cmp_start_bound(&Bound::Excluded(0), &Bound::Included(0)), - Some(Ordering::Greater) - ); - assert_ne!( - cmp_start_bound(&Bound::Excluded(0), &Bound::Included(1)), - Some(Ordering::Greater) - ); - // This is actually WRONG and is why we have to test both ways because I - // can't think of a way to actually determine how the 2 bounds are - // ordered without knowing the smallest discrete step size of T. - // - // In this case technically they should be equal, but for floats (which - // arent ord anyways?) this would be wrong - assert_eq!( - cmp_start_bound(&Bound::Included(1), &Bound::Excluded(0)), - Some(Ordering::Greater) - ); - - assert_eq!( - cmp_start_bound(&Bound::Included(0), &Bound::Excluded(1)), - Some(Ordering::Less) - ); - } - - #[test] - fn end_bound_ord() { - assert_eq!( - cmp_end_bound::(&Bound::Unbounded, &Bound::Unbounded), - Some(Ordering::Equal) - ); - assert_eq!( - cmp_end_bound(&Bound::Unbounded, &Bound::Included(0)), - Some(Ordering::Greater) - ); - assert_eq!( - cmp_end_bound(&Bound::Included(0), &Bound::Included(0)), - Some(Ordering::Equal) - ); - assert_eq!( - cmp_end_bound(&Bound::Excluded(0), &Bound::Included(0)), - Some(Ordering::Greater) - ); - assert_ne!( - cmp_end_bound(&Bound::Excluded(0), &Bound::Included(1)), - Some(Ordering::Greater) - ); - // This is actually WRONG and is why we have to test both ways because I - // can't think of a way to actually determine how the 2 bounds are - // ordered without knowing the smallest discrete step size of T. - // - // In this case technically they should be equal, but for floats (which - // arent ord anyways?) this would be wrong - assert_eq!( - cmp_end_bound(&Bound::Included(1), &Bound::Excluded(0)), - Some(Ordering::Greater) - ); - - assert_eq!( - cmp_end_bound(&Bound::Included(0), &Bound::Excluded(1)), - Some(Ordering::Less) - ); - } - - #[test] - fn test_bound_ord() { - let r1 = 0..4; - let r2 = 2..3; - - assert_eq!( - cmp_start_bound(&r1.start_bound(), &r2.start_bound()), - Some(core::cmp::Ordering::Less) - ); - assert_eq!( - cmp_end_bound(&r1.end_bound(), &r2.end_bound()), - Some(core::cmp::Ordering::Greater) - ); - - assert_eq!( - cmp_start_bound(&r2.start_bound(), &r1.start_bound()), - Some(core::cmp::Ordering::Greater) - ); - assert_eq!( - cmp_end_bound(&r2.end_bound(), &r1.end_bound()), - Some(core::cmp::Ordering::Less) - ); - - let r1 = 0..=8; - let r2 = 0..9; - - assert_eq!( - cmp_start_bound(&r1.start_bound(), &r2.start_bound()), - Some(core::cmp::Ordering::Equal) - ); - assert_eq!( - cmp_end_bound(&r1.end_bound(), &r2.end_bound()), - Some(core::cmp::Ordering::Less) - ); - assert_eq!( - cmp_end_bound(&r2.end_bound(), &r1.end_bound()), - Some(core::cmp::Ordering::Greater) - ); - } -} - -pub fn ranges_intersect(rhs: R1, lhs: R2) -> () -where - T: Ord, - R1: core::ops::RangeBounds, - R2: core::ops::RangeBounds, -{ - let a = rhs.start_bound(); - let b = rhs.end_bound(); - let x = lhs.start_bound(); - let y = lhs.end_bound(); - - // check that a <=> x is different than b <=> x - - // a <=> x - match a { - Bound::Included(a) => match x { - Bound::Included(x) => a.partial_cmp(x), - Bound::Excluded(x) => match a.partial_cmp(x) { - Some(Ordering::Equal) => Some(Ordering::Less), - ord => ord, - }, - Bound::Unbounded => Some(Ordering::Greater), - }, - Bound::Excluded(a) => match x { - Bound::Included(x) => match a.partial_cmp(x) { - Some(Ordering::Equal) => Some(Ordering::Greater), - ord => ord, - }, - Bound::Excluded(x) => a.partial_cmp(x), - Bound::Unbounded => Some(Ordering::Less), - }, - Bound::Unbounded => match x { - Bound::Unbounded => Some(Ordering::Equal), - _ => Some(Ordering::Less), - }, - }; - - todo!() -} diff --git a/btrfs/src/v2/tree.rs b/btrfs/src/v2/tree.rs index 0fab4f0..0d96e14 100644 --- a/btrfs/src/v2/tree.rs +++ b/btrfs/src/v2/tree.rs @@ -32,7 +32,7 @@ impl NodePtr { pub fn key_ptr(&self) -> &KeyPtr { match self { NodePtr::Unvisited(key) => key, - NodePtr::Visited { key, node } => key, + NodePtr::Visited { key, .. } => key, } } @@ -216,7 +216,11 @@ pub mod entry { self.key } - pub fn item(&self) -> Result { + pub fn item_and_value(&self) -> Result<(Item, TreeItem)> { + self.node.parse_item() + } + + pub fn value(&self) -> Result { Ok(self.node.parse_item()?.1) } } diff --git a/btrfs/src/v2/volume.rs b/btrfs/src/v2/volume.rs index b904929..f7c59b5 100644 --- a/btrfs/src/v2/volume.rs +++ b/btrfs/src/v2/volume.rs @@ -1,5 +1,5 @@ use core::mem::size_of; -use core::ops::{Range, RangeBounds}; +use core::ops::RangeBounds; use alloc::collections::btree_map::Entry; use alloc::{collections::BTreeMap, rc::Rc, vec, vec::Vec}; @@ -490,13 +490,18 @@ impl Fs { Some(crc as u64), ); - if let Some((_, value)) = self.fs_root.find_key(&key)? { - let dir_items = value.as_dir_item().expect("dir index"); - - let item = dir_items.iter().find(|item| item.name() == child).cloned(); - Ok(item) - } else { - Ok(None) + match self.fs_root.entry(&key)? { + super::tree::entry::Entry::Occupied(occupied) => { + let item = occupied + .value()? + .as_dir_item() + .expect("dir item") + .iter() + .find(|item| item.name() == child) + .cloned(); + Ok(item) + } + super::tree::entry::Entry::Vacant(_) => Ok(None), } } @@ -605,12 +610,13 @@ impl Fs { fn find_inode_ref(&self, inode_id: u64) -> Result> { let key = PartialKey::new(Some(inode_id.into()), Some(ObjectType::INodeRef), None); - if let Some((item, value)) = self.fs_root.find_key(&key)? { - let inode = value.as_inode_ref().expect("inoderef").clone(); - - Ok(Some((item, inode))) - } else { - Ok(None) + match self.fs_root.entry(&key)? { + super::tree::entry::Entry::Occupied(entry) => { + entry.item_and_value().map(|(item, value)| { + Some((item, value.as_inode_ref().expect("inode ref").clone())) + }) + } + super::tree::entry::Entry::Vacant(_) => Ok(None), } } @@ -626,22 +632,20 @@ impl Fs { Some(inoderef.item().index.get()), ); - if let Some((_, value)) = self.fs_root.find_key(&key)? { - let dir_index = value.as_dir_index().expect("dir index").clone(); - Ok(Some(dir_index)) - } else { - Ok(None) + match self.fs_root.entry(&key)? { + super::tree::entry::Entry::Occupied(entry) => entry + .item_and_value() + .map(|(_, value)| Some(value.as_dir_index().expect("dir index").clone())), + super::tree::entry::Entry::Vacant(_) => Ok(None), } } fn find_inode_item(&self, dir_item: &DirItemEntry) -> Result> { - dir_item.item().location; - if let Some((_, value)) = self.fs_root.find_key(&dir_item.item().location)? { - let inode = value.as_inode_item().expect("inode item").clone(); - - Ok(Some(inode)) - } else { - Ok(None) + match self.fs_root.entry(&dir_item.item().location)? { + super::tree::entry::Entry::Occupied(entry) => entry + .item_and_value() + .map(|(_, value)| Some(value.as_inode_item().expect("inode item").clone())), + super::tree::entry::Entry::Vacant(_) => Ok(None), } } } @@ -674,7 +678,7 @@ mod tests { let v2 = vol.into_volume2().expect("volume2"); log::info!("roots:"); - for (id, v) in v2.roots.iter() { + for (id, _) in v2.roots.iter() { log::info!("[{id:?}] "); } } @@ -686,11 +690,11 @@ mod tests { let v2 = vol.into_volume2().expect("volume2"); log::info!("roots:"); - for (id, v) in v2.roots.iter() { + for (id, _) in v2.roots.iter() { log::info!("[{id:?}] "); } log::info!("roots rev:"); - for (id, v) in v2.roots.iter().rev() { + for (id, _) in v2.roots.iter().rev() { log::info!("[{id:?}] "); } } @@ -726,7 +730,7 @@ mod tests { let vol = Volume::new(file).expect("volume"); let v2 = vol.into_volume2().expect("volume2"); - let fs = v2.default_subvolume().expect("subvol"); + _ = v2.default_subvolume().expect("subvol"); Ok(()) } @@ -829,7 +833,7 @@ mod tests { log::info!("files 1:"); let now = std::time::Instant::now(); for (_id, entry) in fs.fs_root.iter() { - if let Some(dir) = entry.as_dir_index() { + if let Some(_dir) = entry.as_dir_index() { //log::info!("{}", dir.name_as_string_lossy()); } } @@ -838,7 +842,7 @@ mod tests { log::info!("files 2:"); let now = std::time::Instant::now(); for (_id, entry) in fs.fs_root.iter() { - if let Some(dir) = entry.as_dir_index() { + if let Some(_dir) = entry.as_dir_index() { //log::info!("{}", dir.name_as_string_lossy()); } }