From cc0b065f8a19720f420736f52d0e0d693507b950 Mon Sep 17 00:00:00 2001 From: mulhern Date: Tue, 18 Dec 2018 11:50:41 -0500 Subject: [PATCH 01/24] Run clippy on tests and non-default features Also, use flag for warning instead of environment variable. This part is only to imitate recommended way. Signed-off-by: mulhern --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index c99a1ad4e4..7e395fdfd8 100644 --- a/Makefile +++ b/Makefile @@ -59,7 +59,7 @@ stratisd.8.gz: stratisd.8 gzip --stdout docs/stratisd.8 > docs/stratisd.8.gz clippy: - RUSTFLAGS='-D warnings' cargo clippy + cargo clippy --all-targets --all-features -- -D warnings uml-graphs: ${HOME}/.cargo/bin/cargo-script PATH=${HOME}/.cargo/bin:${PATH} cargo script scripts/uml_graphs.rs From c347d4815d56b84716b173bda6adcbf4161b76c4 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 10:57:39 -0500 Subject: [PATCH 02/24] Use clippy scope for allowed lints Signed-off-by: mulhern --- src/bin/stratisd.rs | 3 +-- src/engine/mod.rs | 2 +- src/engine/strat_engine/backstore/blockdevmgr.rs | 2 +- src/engine/strat_engine/backstore/metadata.rs | 2 +- src/engine/strat_engine/backstore/mod.rs | 2 +- src/engine/strat_engine/backstore/setup.rs | 4 ++-- src/engine/strat_engine/thinpool/filesystem.rs | 2 +- src/engine/strat_engine/thinpool/mod.rs | 2 +- src/engine/strat_engine/thinpool/thinpool.rs | 4 ++-- src/lib.rs | 3 +-- src/stratis/buff_log.rs | 2 +- src/stratis/mod.rs | 2 +- 12 files changed, 14 insertions(+), 16 deletions(-) diff --git a/src/bin/stratisd.rs b/src/bin/stratisd.rs index 833e56e773..49ad25d9ac 100644 --- a/src/bin/stratisd.rs +++ b/src/bin/stratisd.rs @@ -2,8 +2,7 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at http://mozilla.org/MPL/2.0/. -#![cfg_attr(not(feature = "clippy"), allow(unknown_lints))] -#![allow(doc_markdown)] +#![allow(clippy::doc_markdown)] extern crate devicemapper; extern crate libstratis; diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 032b75d54d..79ebd448ab 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -30,7 +30,7 @@ pub use self::types::RenameAction; mod macros; mod devlinks; -#[allow(module_inception)] +#[allow(clippy::module_inception)] mod engine; mod event; mod sim_engine; diff --git a/src/engine/strat_engine/backstore/blockdevmgr.rs b/src/engine/strat_engine/backstore/blockdevmgr.rs index 37889b0211..febf9b15f8 100644 --- a/src/engine/strat_engine/backstore/blockdevmgr.rs +++ b/src/engine/strat_engine/backstore/blockdevmgr.rs @@ -411,7 +411,7 @@ fn initialize( /// Filter devices for admission to pool based on dev_infos. /// If there is an error finding out the info, return that error. /// Also, return an error if a device is not appropriate for this pool. - #[allow(type_complexity)] + #[allow(clippy::type_complexity)] fn filter_devs<'a, I>( dev_infos: I, pool_uuid: PoolUuid, diff --git a/src/engine/strat_engine/backstore/metadata.rs b/src/engine/strat_engine/backstore/metadata.rs index f15ecb3ffb..ad5a23acc3 100644 --- a/src/engine/strat_engine/backstore/metadata.rs +++ b/src/engine/strat_engine/backstore/metadata.rs @@ -776,7 +776,7 @@ mod mda { // This comparison seems absurd when compiled in an environment // where usize is u64, which is usual. It is not absurd when // compiled in an environment where usize is u32. - #![allow(absurd_extreme_comparisons)] + #![allow(clippy::absurd_extreme_comparisons)] assert!(*self.used <= std::usize::MAX as u64); let mut data_buf = vec![0u8; *self.used as usize]; diff --git a/src/engine/strat_engine/backstore/mod.rs b/src/engine/strat_engine/backstore/mod.rs index bf46e37790..9ee8c8c161 100644 --- a/src/engine/strat_engine/backstore/mod.rs +++ b/src/engine/strat_engine/backstore/mod.rs @@ -2,7 +2,7 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this // file, You can obtain one at http://mozilla.org/MPL/2.0/. -#[allow(module_inception)] +#[allow(clippy::module_inception)] mod backstore; mod blockdev; mod blockdevmgr; diff --git a/src/engine/strat_engine/backstore/setup.rs b/src/engine/strat_engine/backstore/setup.rs index 17ec16c58f..2f7a676927 100644 --- a/src/engine/strat_engine/backstore/setup.rs +++ b/src/engine/strat_engine/backstore/setup.rs @@ -50,7 +50,7 @@ pub fn find_all() -> StratisResult>> /// Get the most recent metadata from a set of Devices for a given pool UUID. /// Returns None if no metadata found for this pool. -#[allow(implicit_hasher)] +#[allow(clippy::implicit_hasher)] pub fn get_metadata( pool_uuid: PoolUuid, devnodes: &HashMap, @@ -121,7 +121,7 @@ pub fn get_metadata( /// are the devs that support the cache tier. /// Precondition: Every device in devnodes has already been determined to /// belong to the pool with the specified pool uuid. -#[allow(implicit_hasher)] +#[allow(clippy::implicit_hasher)] pub fn get_blockdevs( pool_uuid: PoolUuid, backstore_save: &BackstoreSave, diff --git a/src/engine/strat_engine/thinpool/filesystem.rs b/src/engine/strat_engine/thinpool/filesystem.rs index 1862189afd..39b22e387a 100644 --- a/src/engine/strat_engine/thinpool/filesystem.rs +++ b/src/engine/strat_engine/thinpool/filesystem.rs @@ -136,7 +136,7 @@ impl StratFilesystem { /// Mounting a filesystem with a duplicate UUID would require special handling, /// so snapshot_fs_uuid is used to update the new snapshot filesystem so it has /// a unique UUID. - #[allow(too_many_arguments)] + #[allow(clippy::too_many_arguments)] pub fn snapshot( &self, thin_pool: &ThinPoolDev, diff --git a/src/engine/strat_engine/thinpool/mod.rs b/src/engine/strat_engine/thinpool/mod.rs index e1c46be83d..1faf15f3e6 100644 --- a/src/engine/strat_engine/thinpool/mod.rs +++ b/src/engine/strat_engine/thinpool/mod.rs @@ -5,7 +5,7 @@ mod filesystem; mod mdv; mod thinids; -#[allow(module_inception)] +#[allow(clippy::module_inception)] mod thinpool; pub use self::thinpool::{ThinPool, ThinPoolSizeParams, DATA_BLOCK_SIZE}; diff --git a/src/engine/strat_engine/thinpool/thinpool.rs b/src/engine/strat_engine/thinpool/thinpool.rs index 34f539aeb7..e2d7156ccb 100644 --- a/src/engine/strat_engine/thinpool/thinpool.rs +++ b/src/engine/strat_engine/thinpool/thinpool.rs @@ -1146,7 +1146,7 @@ impl Recordable for ThinPool { /// run thin_repair, using the spare segments, to try to repair the metadata /// dev. Return the metadata device, the metadata segments, and the /// spare segments. -#[allow(type_complexity)] +#[allow(clippy::type_complexity)] fn setup_metadev( pool_uuid: PoolUuid, thinpool_name: &DmName, @@ -1154,7 +1154,7 @@ fn setup_metadev( meta_segments: Vec<(Sectors, Sectors)>, spare_segments: Vec<(Sectors, Sectors)>, ) -> StratisResult<(LinearDev, Vec<(Sectors, Sectors)>, Vec<(Sectors, Sectors)>)> { - #![allow(collapsible_if)] + #![allow(clippy::collapsible_if)] let (dm_name, dm_uuid) = format_flex_ids(pool_uuid, FlexRole::ThinMeta); let mut meta_dev = LinearDev::setup( get_dm(), diff --git a/src/lib.rs b/src/lib.rs index acf63d9855..b2e78f6666 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,5 +1,4 @@ -#![cfg_attr(not(feature = "clippy"), allow(unknown_lints))] -#![allow(doc_markdown)] +#![allow(clippy::doc_markdown)] extern crate devicemapper; #[macro_use] diff --git a/src/stratis/buff_log.rs b/src/stratis/buff_log.rs index 8224298fc7..b8e9054dfb 100644 --- a/src/stratis/buff_log.rs +++ b/src/stratis/buff_log.rs @@ -170,7 +170,7 @@ impl OwnedRecord { } } -#[allow(type_complexity)] +#[allow(clippy::type_complexity)] #[derive(Debug)] struct BuffLogger { pass_through: bool, diff --git a/src/stratis/mod.rs b/src/stratis/mod.rs index baafb8d43c..e65c3ed44e 100644 --- a/src/stratis/mod.rs +++ b/src/stratis/mod.rs @@ -7,5 +7,5 @@ pub use self::stratis::VERSION; pub mod buff_log; mod errors; -#[allow(module_inception)] +#[allow(clippy::module_inception)] mod stratis; From 4b4217de1711734aa9f6a7a0cddc75a7abf7c5f2 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 11:08:49 -0500 Subject: [PATCH 03/24] Remove explicit unit return types clippy likes it better if we leave it out. Signed-off-by: mulhern --- src/bin/stratisd.rs | 2 +- src/engine/strat_engine/backstore/backstore.rs | 8 ++++---- src/engine/strat_engine/backstore/blockdevmgr.rs | 10 +++++----- src/engine/strat_engine/backstore/cache_tier.rs | 2 +- src/engine/strat_engine/backstore/data_tier.rs | 2 +- src/engine/strat_engine/pool.rs | 4 ++-- src/engine/strat_engine/tests/loopbacked.rs | 2 +- src/engine/strat_engine/tests/real.rs | 4 ++-- src/engine/strat_engine/thinpool/thinpool.rs | 4 ++-- src/engine/structures.rs | 2 +- 10 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/bin/stratisd.rs b/src/bin/stratisd.rs index 49ad25d9ac..01592feaba 100644 --- a/src/bin/stratisd.rs +++ b/src/bin/stratisd.rs @@ -61,7 +61,7 @@ const DEFAULT_STATE_DUMP_MINUTES: i64 = 10; const DEFAULT_LOG_HOLD_MINUTES: i64 = 30; /// If writing a program error to stderr fails, panic. -fn print_err(err: &StratisError) -> () { +fn print_err(err: &StratisError) { eprintln!("{}", err); } diff --git a/src/engine/strat_engine/backstore/backstore.rs b/src/engine/strat_engine/backstore/backstore.rs index 0b26e24df3..ff46863a76 100644 --- a/src/engine/strat_engine/backstore/backstore.rs +++ b/src/engine/strat_engine/backstore/backstore.rs @@ -590,7 +590,7 @@ mod tests { /// * backstore's data tier allocated is equal to the size of the cap device /// * backstore's next index is always less than the size of the cap /// device - fn invariant(backstore: &Backstore) -> () { + fn invariant(backstore: &Backstore) { assert!( (backstore.cache_tier.is_none() && backstore.cache.is_none()) || (backstore.cache_tier.is_some() @@ -614,7 +614,7 @@ mod tests { /// Nonetheless, because nothing is written or read, cache usage ought /// to be 0. Adding some more cachedevs exercises different code path /// from adding initial cachedevs. - fn test_add_cache_devs(paths: &[&Path]) -> () { + fn test_add_cache_devs(paths: &[&Path]) { assert!(paths.len() > 3); let meta_size = Sectors(IEC::Mi); @@ -713,7 +713,7 @@ mod tests { /// bigger than the reqested amount. /// Request an impossibly large amount. /// Verify that the backstore is now all used up. - fn test_request(paths: &[&Path]) -> () { + fn test_request(paths: &[&Path]) { assert!(paths.len() > 0); let pool_uuid = Uuid::new_v4(); @@ -772,7 +772,7 @@ mod tests { /// Setup the same backstore again. /// Verify blockdev metadata again. /// Destroy all. - fn test_setup(paths: &[&Path]) -> () { + fn test_setup(paths: &[&Path]) { assert!(paths.len() > 1); let (paths1, paths2) = paths.split_at(paths.len() / 2); diff --git a/src/engine/strat_engine/backstore/blockdevmgr.rs b/src/engine/strat_engine/backstore/blockdevmgr.rs index febf9b15f8..5d1ae0bb02 100644 --- a/src/engine/strat_engine/backstore/blockdevmgr.rs +++ b/src/engine/strat_engine/backstore/blockdevmgr.rs @@ -541,7 +541,7 @@ mod tests { /// size() - metadata_size() = avail_space(). /// After 2 Sectors have been allocated, that amount must also be included /// in balance. - fn test_blockdevmgr_used(paths: &[&Path]) -> () { + fn test_blockdevmgr_used(paths: &[&Path]) { let mut mgr = BlockDevMgr::initialize(Uuid::new_v4(), paths, MIN_MDA_SECTORS).unwrap(); assert_eq!(mgr.avail_space() + mgr.metadata_size(), mgr.size()); @@ -579,7 +579,7 @@ mod tests { /// Verify that it is impossible to initialize a set of disks of which /// even one of them has a signature. Choose the dirty disk randomly. - fn test_fail_single_signature(paths: &[&Path]) -> () { + fn test_fail_single_signature(paths: &[&Path]) { let index = rand::random::() as usize % paths.len(); cmd::create_ext3_fs(paths[index]).unwrap(); @@ -643,7 +643,7 @@ mod tests { /// 1. Initialize devices with pool uuid. /// 2. Initializing again with different uuid must fail. /// 3. Adding the devices must succeed, because they already belong. - fn test_initialization_add_stratis(paths: &[&Path]) -> () { + fn test_initialization_add_stratis(paths: &[&Path]) { assert!(paths.len() > 1); let (paths1, paths2) = paths.split_at(paths.len() / 2); @@ -699,7 +699,7 @@ mod tests { /// 5. Run find_all() again and verify that both sets of devices are found. /// 6. Verify that get_metadata() return an error. initialize() only /// initializes block devices, it does not write metadata. - fn test_initialize(paths: &[&Path]) -> () { + fn test_initialize(paths: &[&Path]) { assert!(paths.len() > 1); let (paths1, paths2) = paths.split_at(paths.len() / 2); @@ -752,7 +752,7 @@ mod tests { /// Test that initialing devices claims all and that destroying /// them releases all. - fn test_ownership(paths: &[&Path]) -> () { + fn test_ownership(paths: &[&Path]) { let pool_uuid = Uuid::new_v4(); let mut bd_mgr = BlockDevMgr::initialize(pool_uuid, paths, MIN_MDA_SECTORS).unwrap(); diff --git a/src/engine/strat_engine/backstore/cache_tier.rs b/src/engine/strat_engine/backstore/cache_tier.rs index 1be49523c1..20d7e73d22 100644 --- a/src/engine/strat_engine/backstore/cache_tier.rs +++ b/src/engine/strat_engine/backstore/cache_tier.rs @@ -242,7 +242,7 @@ mod tests { /// Do basic testing of the cache. Make a new cache and test some /// expected properties, then add some additional blockdevs and test /// some more properties. - fn cache_test_add(paths: &[&Path]) -> () { + fn cache_test_add(paths: &[&Path]) { assert!(paths.len() > 1); let (paths1, paths2) = paths.split_at(paths.len() / 2); diff --git a/src/engine/strat_engine/backstore/data_tier.rs b/src/engine/strat_engine/backstore/data_tier.rs index e565dc7f55..cf2f44b352 100644 --- a/src/engine/strat_engine/backstore/data_tier.rs +++ b/src/engine/strat_engine/backstore/data_tier.rs @@ -182,7 +182,7 @@ mod tests { /// Put the data tier through some paces. Make it, alloc a small amount, /// add some more blockdevs, allocate enough that the newly added blockdevs /// must be allocated from for success. - fn test_add_and_alloc(paths: &[&Path]) -> () { + fn test_add_and_alloc(paths: &[&Path]) { assert!(paths.len() > 1); let (paths1, paths2) = paths.split_at(paths.len() / 2); diff --git a/src/engine/strat_engine/pool.rs b/src/engine/strat_engine/pool.rs index a020c96aec..62cd2419e9 100644 --- a/src/engine/strat_engine/pool.rs +++ b/src/engine/strat_engine/pool.rs @@ -494,7 +494,7 @@ mod tests { use super::*; - fn invariant(pool: &StratPool, pool_name: &str) -> () { + fn invariant(pool: &StratPool, pool_name: &str) { check_metadata(&pool.record(&Name::new(pool_name.into()))).unwrap(); } @@ -564,7 +564,7 @@ mod tests { /// Verify that a pool with no devices does not have the minimum amount of /// space required. - fn test_empty_pool(paths: &[&Path]) -> () { + fn test_empty_pool(paths: &[&Path]) { assert_eq!(paths.len(), 0); assert!(StratPool::initialize("stratis_test_pool", paths, Redundancy::NONE).is_err()); } diff --git a/src/engine/strat_engine/tests/loopbacked.rs b/src/engine/strat_engine/tests/loopbacked.rs index fd683a6080..f5318ecccf 100644 --- a/src/engine/strat_engine/tests/loopbacked.rs +++ b/src/engine/strat_engine/tests/loopbacked.rs @@ -90,7 +90,7 @@ fn get_devices(count: usize, size: Option, dir: &tempfile::TempDir) -> } /// Run the designated tests according to the specification. -pub fn test_with_spec(limits: DeviceLimits, test: F) -> () +pub fn test_with_spec(limits: DeviceLimits, test: F) where F: Fn(&[&Path]) -> () + panic::RefUnwindSafe, { diff --git a/src/engine/strat_engine/tests/real.rs b/src/engine/strat_engine/tests/real.rs index 56b199a7cd..3f8b09a3e3 100644 --- a/src/engine/strat_engine/tests/real.rs +++ b/src/engine/strat_engine/tests/real.rs @@ -43,7 +43,7 @@ impl RealTestDev { } /// Teardown a real test dev - fn teardown(self) -> () { + fn teardown(self) { wipe_sectors(&self.as_path(), Sectors(0), Bytes(IEC::Mi).sectors()).unwrap(); if let Some(mut ld) = self.dev.right() { ld.teardown(get_dm()).unwrap(); @@ -188,7 +188,7 @@ fn make_linear_test_dev(devnode: &Path, start: Sectors, length: Sectors) -> Line /// Run test on real devices, using given constraints. Constraints may result /// in multiple invocations of the test, with differing numbers of block /// devices. -pub fn test_with_spec(limits: DeviceLimits, test: F) -> () +pub fn test_with_spec(limits: DeviceLimits, test: F) where F: Fn(&[&Path]) -> () + panic::RefUnwindSafe, { diff --git a/src/engine/strat_engine/thinpool/thinpool.rs b/src/engine/strat_engine/thinpool/thinpool.rs index e2d7156ccb..3e673e6b0c 100644 --- a/src/engine/strat_engine/thinpool/thinpool.rs +++ b/src/engine/strat_engine/thinpool/thinpool.rs @@ -1608,7 +1608,7 @@ mod tests { /// Verify that destroy_filesystems actually deallocates the space /// from the thinpool, by attempting to reinstantiate it using the /// same thin id and verifying that it fails. - fn test_thindev_destroy(paths: &[&Path]) -> () { + fn test_thindev_destroy(paths: &[&Path]) { let pool_uuid = Uuid::new_v4(); devlinks::cleanup_devlinks(Vec::new().into_iter()); let mut backstore = Backstore::initialize(pool_uuid, paths, MIN_MDA_SECTORS).unwrap(); @@ -1660,7 +1660,7 @@ mod tests { /// dip below the FILESYSTEM_LOWATER mark. Verify that the space has been /// expanded by calling filesystem.check() then looking at the total space /// compared to the original size. - fn test_xfs_expand(paths: &[&Path]) -> () { + fn test_xfs_expand(paths: &[&Path]) { let pool_uuid = Uuid::new_v4(); devlinks::cleanup_devlinks(Vec::new().into_iter()); let mut backstore = Backstore::initialize(pool_uuid, paths, MIN_MDA_SECTORS).unwrap(); diff --git a/src/engine/structures.rs b/src/engine/structures.rs index 01706cbfe6..ecaeb2b5d6 100644 --- a/src/engine/structures.rs +++ b/src/engine/structures.rs @@ -281,7 +281,7 @@ mod tests { // A global invariant checker for the table. // Verifies proper relationship between internal data structures. - fn table_invariant(table: &Table) -> () { + fn table_invariant(table: &Table) { for (uuid, &(ref name, _)) in &table.items { assert_eq!(*uuid, *table.name_to_uuid.get(name).unwrap()) } From f28610365ebeaf075d20b8a361ffbe38970602f2 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 11:11:11 -0500 Subject: [PATCH 04/24] Use _'s to make hex value more readable Because clippy likes the look and they are a bit more readable, too. Signed-off-by: mulhern --- src/engine/strat_engine/backstore/metadata.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/strat_engine/backstore/metadata.rs b/src/engine/strat_engine/backstore/metadata.rs index ad5a23acc3..a21ea69fae 100644 --- a/src/engine/strat_engine/backstore/metadata.rs +++ b/src/engine/strat_engine/backstore/metadata.rs @@ -848,7 +848,7 @@ mod mda { use super::*; // 82102984128000 in decimal, approx 17 million years - const UTC_TIMESTAMP_SECS_BOUND: i64 = 0x7779beb9f00; + const UTC_TIMESTAMP_SECS_BOUND: i64 = 0x777_9beb_9f00; const UTC_TIMESTAMP_NSECS_BOUND: u32 = 2_000_000_000u32; #[test] From dcfb5d75c79cb25aa91ecab6f39b2349e904a2ad Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 11:13:50 -0500 Subject: [PATCH 05/24] Use cloned() instead of map() On the principle of least strength. Also, clippy prefers it. Signed-off-by: mulhern --- src/engine/sim_engine/engine.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/sim_engine/engine.rs b/src/engine/sim_engine/engine.rs index 84a80304ec..393dd68923 100644 --- a/src/engine/sim_engine/engine.rs +++ b/src/engine/sim_engine/engine.rs @@ -44,7 +44,7 @@ impl Engine for SimEngine { } let device_set: HashSet<_, RandomState> = HashSet::from_iter(blockdev_paths); - let devices = device_set.into_iter().map(|x| *x).collect::>(); + let devices = device_set.into_iter().cloned().collect::>(); let (pool_uuid, pool) = SimPool::new(&Rc::clone(&self.rdm), &devices, redundancy); From ec4dcacf53b14b51dc5f754b85419320dd84f38b Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 11:12:54 -0500 Subject: [PATCH 06/24] Allow a non-Self return value from new It's quite possible that new shouldn't have been used in these cases. Perhaps we can change some of these names later. Signed-off-by: mulhern --- src/engine/sim_engine/blockdev.rs | 1 + src/engine/sim_engine/pool.rs | 1 + src/engine/strat_engine/backstore/blockdev.rs | 1 + src/engine/strat_engine/backstore/cache_tier.rs | 1 + src/engine/strat_engine/backstore/range_alloc.rs | 1 + src/engine/strat_engine/thinpool/thinpool.rs | 1 + 6 files changed, 6 insertions(+) diff --git a/src/engine/sim_engine/blockdev.rs b/src/engine/sim_engine/blockdev.rs index 94280fca72..49ff881032 100644 --- a/src/engine/sim_engine/blockdev.rs +++ b/src/engine/sim_engine/blockdev.rs @@ -63,6 +63,7 @@ impl BlockDev for SimDev { impl SimDev { /// Generates a new device from any devnode. + #[allow(clippy::new_ret_no_self)] pub fn new(rdm: Rc>, devnode: &Path) -> (Uuid, SimDev) { ( Uuid::new_v4(), diff --git a/src/engine/sim_engine/pool.rs b/src/engine/sim_engine/pool.rs index f96e55fdba..18f89fcc99 100644 --- a/src/engine/sim_engine/pool.rs +++ b/src/engine/sim_engine/pool.rs @@ -41,6 +41,7 @@ pub struct SimPool { } impl SimPool { + #[allow(clippy::new_ret_no_self)] pub fn new( rdm: &Rc>, paths: &[&Path], diff --git a/src/engine/strat_engine/backstore/blockdev.rs b/src/engine/strat_engine/backstore/blockdev.rs index 56238555b8..a2d61926e5 100644 --- a/src/engine/strat_engine/backstore/blockdev.rs +++ b/src/engine/strat_engine/backstore/blockdev.rs @@ -49,6 +49,7 @@ impl StratBlockDev { /// on the device is simply invisible to the blockdev. Consequently, it /// is invisible to the engine, and is not part of the total size value /// reported on the D-Bus. + #[allow(clippy::new_ret_no_self)] pub fn new( dev: Device, devnode: PathBuf, diff --git a/src/engine/strat_engine/backstore/cache_tier.rs b/src/engine/strat_engine/backstore/cache_tier.rs index 20d7e73d22..fed6306b02 100644 --- a/src/engine/strat_engine/backstore/cache_tier.rs +++ b/src/engine/strat_engine/backstore/cache_tier.rs @@ -147,6 +147,7 @@ impl CacheTier { /// sub-device too big. /// /// WARNING: metadata changing event + #[allow(clippy::new_ret_no_self)] pub fn new(mut block_mgr: BlockDevMgr) -> StratisResult { let avail_space = block_mgr.avail_space(); diff --git a/src/engine/strat_engine/backstore/range_alloc.rs b/src/engine/strat_engine/backstore/range_alloc.rs index 4e1d7ba029..c4f054462f 100644 --- a/src/engine/strat_engine/backstore/range_alloc.rs +++ b/src/engine/strat_engine/backstore/range_alloc.rs @@ -19,6 +19,7 @@ pub struct RangeAllocator { impl RangeAllocator { /// Create a new RangeAllocator with the specified (offset, length) /// ranges marked as used. + #[allow(clippy::new_ret_no_self)] pub fn new( limit: Sectors, initial_used: &[(Sectors, Sectors)], diff --git a/src/engine/strat_engine/thinpool/thinpool.rs b/src/engine/strat_engine/thinpool/thinpool.rs index 3e673e6b0c..5b04c737db 100644 --- a/src/engine/strat_engine/thinpool/thinpool.rs +++ b/src/engine/strat_engine/thinpool/thinpool.rs @@ -228,6 +228,7 @@ pub struct ThinPool { impl ThinPool { /// Make a new thin pool. + #[allow(clippy::new_ret_no_self)] pub fn new( pool_uuid: PoolUuid, thin_pool_size: &ThinPoolSizeParams, From ff2fe25f82b642f9bb03ccad3bdffef1e090d25e Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 11:33:53 -0500 Subject: [PATCH 07/24] Simplify a boolean expression Signed-off-by: mulhern --- src/engine/sim_engine/randomization.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/engine/sim_engine/randomization.rs b/src/engine/sim_engine/randomization.rs index 9f62f8bc49..4f5e7b41f0 100644 --- a/src/engine/sim_engine/randomization.rs +++ b/src/engine/sim_engine/randomization.rs @@ -61,8 +61,8 @@ mod tests { .set_probability(denominator) .throw_die(); prop_assert!(denominator > 1 - || (denominator != 0 && result == false) - || (denominator == 0 && result == true)); + || (denominator != 0 && !result) + || (denominator == 0 && result)); } } } From 1a98cb608179fdb842d82e72883dc1c502b83144 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 11:35:15 -0500 Subject: [PATCH 08/24] Use empty check instead of length check According to the principle of least strength. Also, clippy prefers it. Signed-off-by: mulhern --- src/engine/strat_engine/backstore/backstore.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/strat_engine/backstore/backstore.rs b/src/engine/strat_engine/backstore/backstore.rs index ff46863a76..a9c39f22e6 100644 --- a/src/engine/strat_engine/backstore/backstore.rs +++ b/src/engine/strat_engine/backstore/backstore.rs @@ -714,7 +714,7 @@ mod tests { /// Request an impossibly large amount. /// Verify that the backstore is now all used up. fn test_request(paths: &[&Path]) { - assert!(paths.len() > 0); + assert!(!paths.is_empty()); let pool_uuid = Uuid::new_v4(); let mut backstore = Backstore::initialize(pool_uuid, paths, MIN_MDA_SECTORS).unwrap(); From e171a685300a2fc4722693e98320f62b4837837e Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 11:36:56 -0500 Subject: [PATCH 09/24] Use [] operator to get item in map when the result is unwrapped clippy thinks this is more concise. Signed-off-by: mulhern --- src/engine/strat_engine/backstore/backstore.rs | 4 ++-- src/engine/strat_engine/pool.rs | 10 +++++----- src/engine/structures.rs | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/engine/strat_engine/backstore/backstore.rs b/src/engine/strat_engine/backstore/backstore.rs index a9c39f22e6..b99fac3c1c 100644 --- a/src/engine/strat_engine/backstore/backstore.rs +++ b/src/engine/strat_engine/backstore/backstore.rs @@ -798,7 +798,7 @@ mod tests { cmd::udev_settle().unwrap(); let map = find_all().unwrap(); - let map = map.get(&pool_uuid).unwrap(); + let map = &map[&pool_uuid]; let mut backstore = Backstore::setup(pool_uuid, &backstore_save, &map, None).unwrap(); invariant(&backstore); @@ -810,7 +810,7 @@ mod tests { cmd::udev_settle().unwrap(); let map = find_all().unwrap(); - let map = map.get(&pool_uuid).unwrap(); + let map = &map[&pool_uuid]; let mut backstore = Backstore::setup(pool_uuid, &backstore_save, &map, None).unwrap(); invariant(&backstore); diff --git a/src/engine/strat_engine/pool.rs b/src/engine/strat_engine/pool.rs index 62cd2419e9..97ff38d21b 100644 --- a/src/engine/strat_engine/pool.rs +++ b/src/engine/strat_engine/pool.rs @@ -525,8 +525,8 @@ mod tests { cmd::udev_settle().unwrap(); let pools = find_all().unwrap(); assert_eq!(pools.len(), 2); - let devnodes1 = pools.get(&uuid1).unwrap(); - let devnodes2 = pools.get(&uuid2).unwrap(); + let devnodes1 = &pools[&uuid1]; + let devnodes2 = &pools[&uuid2]; let pool_save1 = get_metadata(uuid1, devnodes1).unwrap().unwrap(); let pool_save2 = get_metadata(uuid2, devnodes2).unwrap().unwrap(); assert_eq!(pool_save1, metadata1); @@ -538,8 +538,8 @@ mod tests { cmd::udev_settle().unwrap(); let pools = find_all().unwrap(); assert_eq!(pools.len(), 2); - let devnodes1 = pools.get(&uuid1).unwrap(); - let devnodes2 = pools.get(&uuid2).unwrap(); + let devnodes1 = &pools[&uuid1]; + let devnodes2 = &pools[&uuid2]; let pool_save1 = get_metadata(uuid1, devnodes1).unwrap().unwrap(); let pool_save2 = get_metadata(uuid2, devnodes2).unwrap().unwrap(); assert_eq!(pool_save1, metadata1); @@ -654,7 +654,7 @@ mod tests { cmd::udev_settle().unwrap(); let pools = find_all().unwrap(); assert_eq!(pools.len(), 1); - let devices = pools.get(&uuid).unwrap(); + let devices = &pools[&uuid]; let (name, pool) = StratPool::setup( uuid, &devices, diff --git a/src/engine/structures.rs b/src/engine/structures.rs index ecaeb2b5d6..52b3463621 100644 --- a/src/engine/structures.rs +++ b/src/engine/structures.rs @@ -283,11 +283,11 @@ mod tests { // Verifies proper relationship between internal data structures. fn table_invariant(table: &Table) { for (uuid, &(ref name, _)) in &table.items { - assert_eq!(*uuid, *table.name_to_uuid.get(name).unwrap()) + assert_eq!(uuid, &table.name_to_uuid[name]) } for (name, uuid) in &table.name_to_uuid { - assert_eq!(*name, table.items.get(uuid).unwrap().0); + assert_eq!(name, &table.items[uuid].0); } // No extra garbage From b4ad126c7924515a101fb1aabc9f972391050c86 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 11:39:51 -0500 Subject: [PATCH 10/24] Do not use & in pattern matching rustc does not require it any more and clippy doesn't like it. Signed-off-by: mulhern --- src/engine/strat_engine/backstore/blockdevmgr.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/engine/strat_engine/backstore/blockdevmgr.rs b/src/engine/strat_engine/backstore/blockdevmgr.rs index 5d1ae0bb02..a5c2175ba6 100644 --- a/src/engine/strat_engine/backstore/blockdevmgr.rs +++ b/src/engine/strat_engine/backstore/blockdevmgr.rs @@ -528,8 +528,8 @@ mod tests { true } else { match left { - &DevOwnership::Theirs(_) => match right { - &DevOwnership::Theirs(_) => true, + DevOwnership::Theirs(_) => match right { + DevOwnership::Theirs(_) => true, _ => false, }, _ => false, From 22df9547339e2912893073270d374fadc279073e Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 11:43:15 -0500 Subject: [PATCH 11/24] Use u64::from for a safe cast The compiler will now fail if the type cast from is unsafe to cast to a u64. Signed-off-by: mulhern --- src/engine/strat_engine/backstore/metadata.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/engine/strat_engine/backstore/metadata.rs b/src/engine/strat_engine/backstore/metadata.rs index a21ea69fae..36f04fc990 100644 --- a/src/engine/strat_engine/backstore/metadata.rs +++ b/src/engine/strat_engine/backstore/metadata.rs @@ -892,7 +892,7 @@ mod mda { // 4 is NUM_MDA_REGIONS which is not imported from super. let region_size = - (MIN_MDA_SECTORS / 4usize).bytes() + Bytes(region_size_ext as u64); + (MIN_MDA_SECTORS / 4usize).bytes() + Bytes(u64::from(region_size_ext)); let header = MDAHeader { last_updated: Utc.timestamp(sec, nsec), @@ -977,7 +977,7 @@ mod tests { fn random_static_header(blkdev_size: u64, mda_size_factor: u32) -> StaticHeader { let pool_uuid = Uuid::new_v4(); let dev_uuid = Uuid::new_v4(); - let mda_size = MIN_MDA_SECTORS + Sectors((mda_size_factor * 4) as u64); + let mda_size = MIN_MDA_SECTORS + Sectors(u64::from(mda_size_factor * 4)); let blkdev_size = (Bytes(IEC::Mi) + Sectors(blkdev_size).bytes()).sectors(); StaticHeader::new( pool_uuid, From 9710b89b67355632504804135a711a459bbafdbd Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 12:06:33 -0500 Subject: [PATCH 12/24] Use & on expression instead of "ref" on identifier clippy prefers it and it is more usual. Signed-off-by: mulhern --- src/engine/structures.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/engine/structures.rs b/src/engine/structures.rs index 52b3463621..aa0627abef 100644 --- a/src/engine/structures.rs +++ b/src/engine/structures.rs @@ -370,7 +370,7 @@ mod tests { // It has displaced the old thing. assert!(displaced.is_some()); - let ref displaced_item = displaced.unwrap(); + let displaced_item = &displaced.unwrap(); assert_eq!(&*displaced_item[0].0, name); assert_eq!(displaced_item[0].1, uuid); @@ -411,7 +411,7 @@ mod tests { // The items displaced consist exactly of the first item. assert!(displaced.is_some()); - let ref displaced_item = displaced.unwrap(); + let displaced_item = &displaced.unwrap(); assert_eq!(&*displaced_item[0].0, name); assert_eq!(displaced_item[0].1, uuid); assert_eq!(displaced_item[0].2.stuff, thing_key); @@ -455,7 +455,7 @@ mod tests { // The items displaced consist exactly of the first item. assert!(displaced.is_some()); - let ref displaced_item = displaced.unwrap(); + let displaced_item = &displaced.unwrap(); assert_eq!(&*displaced_item[0].0, name); assert_eq!(displaced_item[0].1, uuid); assert_eq!(displaced_item[0].2.stuff, thing_key); @@ -518,7 +518,7 @@ mod tests { // The items displaced consist of two items. assert!(displaced.is_some()); - let ref displaced_items = displaced.unwrap(); + let displaced_items = &displaced.unwrap(); assert_eq!(displaced_items.len(), 2); // The first displaced item has the name of the just inserted item. From d719f3df57d77f8c2e19248d3bcdfe0c81958e9e Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 12:07:59 -0500 Subject: [PATCH 13/24] Do not clone a copy value It's just extra business. Signed-off-by: mulhern --- src/engine/structures.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/structures.rs b/src/engine/structures.rs index aa0627abef..b4ceb86585 100644 --- a/src/engine/structures.rs +++ b/src/engine/structures.rs @@ -298,7 +298,7 @@ mod tests { pub fn new(name: &str, uuid: Uuid) -> TestThing { TestThing { name: name.to_owned(), - uuid: uuid.clone(), + uuid, stuff: rand::random::(), } } From dea302bc037a44a8674a8ba4338059f21ec4a23e Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 12:08:51 -0500 Subject: [PATCH 14/24] Remove an identify conversion Signed-off-by: mulhern --- src/engine/strat_engine/tests/util.rs | 1 - src/engine/strat_engine/throttle.rs | 1 - 2 files changed, 2 deletions(-) diff --git a/src/engine/strat_engine/tests/util.rs b/src/engine/strat_engine/tests/util.rs index 2582244858..f176a13632 100644 --- a/src/engine/strat_engine/tests/util.rs +++ b/src/engine/strat_engine/tests/util.rs @@ -89,7 +89,6 @@ fn stratis_filesystems_unmount() -> Result<()> { let parser = libmount::mountinfo::Parser::new(mount_data.as_bytes()); for mount_point in parser - .into_iter() .filter_map(|x| x.ok()) .filter_map(|m| m.mount_point.into_owned().into_string().ok()) .filter(|mp| mp.contains("stratis")) diff --git a/src/engine/strat_engine/throttle.rs b/src/engine/strat_engine/throttle.rs index d5fca210e1..84b5c67b4d 100644 --- a/src/engine/strat_engine/throttle.rs +++ b/src/engine/strat_engine/throttle.rs @@ -65,7 +65,6 @@ mod tests { fn get_write_throttled_devices() -> StratisResult> { // Find all cgroup subdirectories let mut cg_dirs = fs::read_dir(CGROUP_PATH)? - .into_iter() .filter_map(|entry| entry.ok()) .map(|entry| entry.path()) .filter(|path| path.is_dir()) From 32994d3bbb12eed3256d0cef49220911229333b2 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 12:10:10 -0500 Subject: [PATCH 15/24] Avoid using function expression as argument to unwrap_or Use unwrap_or_else instead. Signed-off-by: mulhern --- src/engine/strat_engine/tests/loopbacked.rs | 2 +- src/engine/strat_engine/tests/real.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/engine/strat_engine/tests/loopbacked.rs b/src/engine/strat_engine/tests/loopbacked.rs index f5318ecccf..adf0aafda8 100644 --- a/src/engine/strat_engine/tests/loopbacked.rs +++ b/src/engine/strat_engine/tests/loopbacked.rs @@ -41,7 +41,7 @@ impl LoopTestDev { /// Create its backing store of specified size. The file is sparse but /// will appear to be zeroed. pub fn new(lc: &LoopControl, path: &Path, size: Option) -> LoopTestDev { - let size = size.unwrap_or(Bytes(IEC::Gi).sectors()); + let size = size.unwrap_or_else(|| Bytes(IEC::Gi).sectors()); let f = OpenOptions::new() .read(true) diff --git a/src/engine/strat_engine/tests/real.rs b/src/engine/strat_engine/tests/real.rs index 3f8b09a3e3..d54a9a9b59 100644 --- a/src/engine/strat_engine/tests/real.rs +++ b/src/engine/strat_engine/tests/real.rs @@ -87,7 +87,7 @@ fn get_device_runs<'a>( } }; - let min_size = min_size.unwrap_or(Bytes(IEC::Gi).sectors()); + let min_size = min_size.unwrap_or_else(|| Bytes(IEC::Gi).sectors()); assert!(max_size.is_none() || Some(min_size) <= max_size); From 093c197de02e65981133f86dea5f9fc6a7c3e165 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 12:13:51 -0500 Subject: [PATCH 16/24] Remove a multiplication by the multiplicative identity clippy doesn't like it. Signed-off-by: mulhern --- src/engine/strat_engine/backstore/metadata.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/strat_engine/backstore/metadata.rs b/src/engine/strat_engine/backstore/metadata.rs index 36f04fc990..44aaf23c9d 100644 --- a/src/engine/strat_engine/backstore/metadata.rs +++ b/src/engine/strat_engine/backstore/metadata.rs @@ -1171,7 +1171,7 @@ mod tests { if let Some(index) = primary { // Corrupt primary copy - corrupt_byte(&mut buf, (1 * SECTOR_SIZE + index) as u64).unwrap(); + corrupt_byte(&mut buf, (SECTOR_SIZE + index) as u64).unwrap(); } if let Some(index) = secondary { From eaace9b567cc4042189e1b3418207e382205f3e8 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 12:30:07 -0500 Subject: [PATCH 17/24] Pass DeviceLimits as reference Otherwise, clippy will think that DeviceLimits type definition ought to be Copy. We don't really want it to be Copy, because there is not guarantee that it will stay so simple. Also, our purpose in passing is certainly not to have it consumed. Signed-off-by: mulhern --- .../strat_engine/backstore/backstore.rs | 18 ++++---- .../strat_engine/backstore/blockdevmgr.rs | 36 ++++++++------- .../strat_engine/backstore/cache_tier.rs | 6 +-- .../strat_engine/backstore/data_tier.rs | 6 +-- src/engine/strat_engine/backstore/device.rs | 8 ++-- src/engine/strat_engine/engine.rs | 11 +++-- src/engine/strat_engine/pool.rs | 16 +++---- src/engine/strat_engine/tests/loopbacked.rs | 10 ++--- src/engine/strat_engine/tests/real.rs | 12 ++--- src/engine/strat_engine/thinpool/thinpool.rs | 45 +++++++++++-------- src/engine/strat_engine/throttle.rs | 4 +- 11 files changed, 95 insertions(+), 77 deletions(-) diff --git a/src/engine/strat_engine/backstore/backstore.rs b/src/engine/strat_engine/backstore/backstore.rs index b99fac3c1c..b176b5d398 100644 --- a/src/engine/strat_engine/backstore/backstore.rs +++ b/src/engine/strat_engine/backstore/backstore.rs @@ -687,7 +687,7 @@ mod tests { #[test] pub fn loop_test_add_cache_devs() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(4, 5, None), + &loopbacked::DeviceLimits::Range(4, 5, None), test_add_cache_devs, ); } @@ -695,7 +695,7 @@ mod tests { #[test] pub fn real_test_add_cache_devs() { real::test_with_spec( - real::DeviceLimits::AtLeast(4, None, None), + &real::DeviceLimits::AtLeast(4, None, None), test_add_cache_devs, ); } @@ -703,7 +703,7 @@ mod tests { #[test] pub fn travis_test_add_cache_devs() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(4, 5, None), + &loopbacked::DeviceLimits::Range(4, 5, None), test_add_cache_devs, ); } @@ -752,17 +752,17 @@ mod tests { #[test] pub fn loop_test_request() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(1, 3, None), test_request); + loopbacked::test_with_spec(&loopbacked::DeviceLimits::Range(1, 3, None), test_request); } #[test] pub fn real_test_request() { - real::test_with_spec(real::DeviceLimits::AtLeast(1, None, None), test_request); + real::test_with_spec(&real::DeviceLimits::AtLeast(1, None, None), test_request); } #[test] pub fn travis_test_request() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(1, 3, None), test_request); + loopbacked::test_with_spec(&loopbacked::DeviceLimits::Range(1, 3, None), test_request); } /// Create a backstore with a cache. @@ -823,16 +823,16 @@ mod tests { #[test] pub fn loop_test_setup() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(2, 3, None), test_setup); + loopbacked::test_with_spec(&loopbacked::DeviceLimits::Range(2, 3, None), test_setup); } #[test] pub fn real_test_setup() { - real::test_with_spec(real::DeviceLimits::AtLeast(2, None, None), test_setup); + real::test_with_spec(&real::DeviceLimits::AtLeast(2, None, None), test_setup); } #[test] pub fn travis_test_setup() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(2, 3, None), test_setup); + loopbacked::test_with_spec(&loopbacked::DeviceLimits::Range(2, 3, None), test_setup); } } diff --git a/src/engine/strat_engine/backstore/blockdevmgr.rs b/src/engine/strat_engine/backstore/blockdevmgr.rs index a5c2175ba6..e8a9edc53f 100644 --- a/src/engine/strat_engine/backstore/blockdevmgr.rs +++ b/src/engine/strat_engine/backstore/blockdevmgr.rs @@ -556,7 +556,7 @@ mod tests { #[test] pub fn loop_test_blockdevmgr_used() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(1, 3, None), + &loopbacked::DeviceLimits::Range(1, 3, None), test_blockdevmgr_used, ); } @@ -564,7 +564,7 @@ mod tests { #[test] pub fn real_test_blockdevmgr_used() { real::test_with_spec( - real::DeviceLimits::AtLeast(1, None, None), + &real::DeviceLimits::AtLeast(1, None, None), test_blockdevmgr_used, ); } @@ -572,7 +572,7 @@ mod tests { #[test] pub fn travis_test_blockdevmgr_used() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(1, 3, None), + &loopbacked::DeviceLimits::Range(1, 3, None), test_blockdevmgr_used, ); } @@ -617,7 +617,7 @@ mod tests { #[test] pub fn loop_test_fail_single_signature() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(1, 3, None), + &loopbacked::DeviceLimits::Range(1, 3, None), test_fail_single_signature, ); } @@ -625,7 +625,7 @@ mod tests { #[test] pub fn real_test_fail_single_signature() { real::test_with_spec( - real::DeviceLimits::AtLeast(1, None, None), + &real::DeviceLimits::AtLeast(1, None, None), test_fail_single_signature, ); } @@ -633,7 +633,7 @@ mod tests { #[test] pub fn travis_test_fail_single_signature() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(1, 3, None), + &loopbacked::DeviceLimits::Range(1, 3, None), test_fail_single_signature, ); } @@ -668,7 +668,7 @@ mod tests { #[test] pub fn loop_test_initialization_stratis() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(2, 3, None), + &loopbacked::DeviceLimits::Range(2, 3, None), test_initialization_add_stratis, ); } @@ -676,7 +676,7 @@ mod tests { #[test] pub fn real_test_initialization_stratis() { real::test_with_spec( - real::DeviceLimits::AtLeast(2, None, None), + &real::DeviceLimits::AtLeast(2, None, None), test_initialization_add_stratis, ); } @@ -684,7 +684,7 @@ mod tests { #[test] pub fn travis_test_initialization_stratis() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(2, 3, None), + &loopbacked::DeviceLimits::Range(2, 3, None), test_initialization_add_stratis, ); } @@ -737,17 +737,23 @@ mod tests { #[test] pub fn loop_test_initialize() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(2, 3, None), test_initialize); + loopbacked::test_with_spec( + &loopbacked::DeviceLimits::Range(2, 3, None), + test_initialize, + ); } #[test] pub fn real_test_initialize() { - real::test_with_spec(real::DeviceLimits::AtLeast(2, None, None), test_initialize); + real::test_with_spec(&real::DeviceLimits::AtLeast(2, None, None), test_initialize); } #[test] pub fn travis_test_initialize() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(2, 3, None), test_initialize); + loopbacked::test_with_spec( + &loopbacked::DeviceLimits::Range(2, 3, None), + test_initialize, + ); } /// Test that initialing devices claims all and that destroying @@ -779,16 +785,16 @@ mod tests { #[test] pub fn loop_test_ownership() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(1, 3, None), test_ownership); + loopbacked::test_with_spec(&loopbacked::DeviceLimits::Range(1, 3, None), test_ownership); } #[test] pub fn real_test_ownership() { - real::test_with_spec(real::DeviceLimits::AtLeast(1, None, None), test_ownership); + real::test_with_spec(&real::DeviceLimits::AtLeast(1, None, None), test_ownership); } #[test] pub fn travis_test_ownership() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(1, 3, None), test_ownership); + loopbacked::test_with_spec(&loopbacked::DeviceLimits::Range(1, 3, None), test_ownership); } } diff --git a/src/engine/strat_engine/backstore/cache_tier.rs b/src/engine/strat_engine/backstore/cache_tier.rs index fed6306b02..859da22663 100644 --- a/src/engine/strat_engine/backstore/cache_tier.rs +++ b/src/engine/strat_engine/backstore/cache_tier.rs @@ -296,16 +296,16 @@ mod tests { #[test] pub fn loop_cache_test_add() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(2, 3, None), cache_test_add); + loopbacked::test_with_spec(&loopbacked::DeviceLimits::Range(2, 3, None), cache_test_add); } #[test] pub fn real_cache_test_add() { - real::test_with_spec(real::DeviceLimits::AtLeast(2, None, None), cache_test_add); + real::test_with_spec(&real::DeviceLimits::AtLeast(2, None, None), cache_test_add); } #[test] pub fn travis_cache_test_add() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(2, 3, None), cache_test_add); + loopbacked::test_with_spec(&loopbacked::DeviceLimits::Range(2, 3, None), cache_test_add); } } diff --git a/src/engine/strat_engine/backstore/data_tier.rs b/src/engine/strat_engine/backstore/data_tier.rs index cf2f44b352..1c01ee95fa 100644 --- a/src/engine/strat_engine/backstore/data_tier.rs +++ b/src/engine/strat_engine/backstore/data_tier.rs @@ -231,7 +231,7 @@ mod tests { #[test] pub fn loop_test_add_and_alloc() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(2, 3, None), + &loopbacked::DeviceLimits::Range(2, 3, None), test_add_and_alloc, ); } @@ -239,7 +239,7 @@ mod tests { #[test] pub fn real_test_add_and_alloc() { real::test_with_spec( - real::DeviceLimits::AtLeast(2, None, None), + &real::DeviceLimits::AtLeast(2, None, None), test_add_and_alloc, ); } @@ -247,7 +247,7 @@ mod tests { #[test] pub fn travis_test_add_and_alloc() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(2, 3, None), + &loopbacked::DeviceLimits::Range(2, 3, None), test_add_and_alloc, ); } diff --git a/src/engine/strat_engine/backstore/device.rs b/src/engine/strat_engine/backstore/device.rs index 33f5f319da..2b64c14037 100644 --- a/src/engine/strat_engine/backstore/device.rs +++ b/src/engine/strat_engine/backstore/device.rs @@ -180,7 +180,7 @@ mod test { #[test] pub fn loop_test_device_other_ownership() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(1, 3, None), + &loopbacked::DeviceLimits::Range(1, 3, None), test_other_ownership, ); } @@ -188,18 +188,18 @@ mod test { #[test] pub fn real_test_device_other_ownership() { real::test_with_spec( - real::DeviceLimits::AtLeast(1, None, None), + &real::DeviceLimits::AtLeast(1, None, None), test_other_ownership, ); } #[test] pub fn loop_test_device_empty() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(1, 3, None), test_empty); + loopbacked::test_with_spec(&loopbacked::DeviceLimits::Range(1, 3, None), test_empty); } #[test] pub fn real_test_device_empty() { - real::test_with_spec(real::DeviceLimits::AtLeast(1, None, None), test_empty); + real::test_with_spec(&real::DeviceLimits::AtLeast(1, None, None), test_empty); } } diff --git a/src/engine/strat_engine/engine.rs b/src/engine/strat_engine/engine.rs index 1ac0e7283d..50fab2cb05 100644 --- a/src/engine/strat_engine/engine.rs +++ b/src/engine/strat_engine/engine.rs @@ -399,14 +399,17 @@ mod test { #[test] pub fn loop_test_pool_rename() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(1, 3, None), + &loopbacked::DeviceLimits::Range(1, 3, None), test_pool_rename, ); } #[test] pub fn real_test_pool_rename() { - real::test_with_spec(real::DeviceLimits::AtLeast(1, None, None), test_pool_rename); + real::test_with_spec( + &real::DeviceLimits::AtLeast(1, None, None), + test_pool_rename, + ); } /// Test engine setup. @@ -455,11 +458,11 @@ mod test { #[test] pub fn loop_test_setup() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(2, 3, None), test_setup); + loopbacked::test_with_spec(&loopbacked::DeviceLimits::Range(2, 3, None), test_setup); } #[test] pub fn real_test_setup() { - real::test_with_spec(real::DeviceLimits::AtLeast(2, None, None), test_setup); + real::test_with_spec(&real::DeviceLimits::AtLeast(2, None, None), test_setup); } } diff --git a/src/engine/strat_engine/pool.rs b/src/engine/strat_engine/pool.rs index 97ff38d21b..2de83d4780 100644 --- a/src/engine/strat_engine/pool.rs +++ b/src/engine/strat_engine/pool.rs @@ -549,7 +549,7 @@ mod tests { #[test] pub fn loop_test_basic_metadata() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(2, 3, None), + &loopbacked::DeviceLimits::Range(2, 3, None), test_basic_metadata, ); } @@ -557,7 +557,7 @@ mod tests { #[test] pub fn real_test_basic_metadata() { real::test_with_spec( - real::DeviceLimits::AtLeast(2, None, None), + &real::DeviceLimits::AtLeast(2, None, None), test_basic_metadata, ); } @@ -571,12 +571,12 @@ mod tests { #[test] pub fn loop_test_empty_pool() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Exactly(0, None), test_empty_pool); + loopbacked::test_with_spec(&loopbacked::DeviceLimits::Exactly(0, None), test_empty_pool); } #[test] pub fn real_test_empty_pool() { - real::test_with_spec(real::DeviceLimits::Exactly(0, None, None), test_empty_pool); + real::test_with_spec(&real::DeviceLimits::Exactly(0, None, None), test_empty_pool); } /// Test that adding a cachedev causes metadata to be updated. @@ -688,7 +688,7 @@ mod tests { #[test] pub fn loop_test_add_cachedevs() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(2, 3, None), + &loopbacked::DeviceLimits::Range(2, 3, None), test_add_cachedevs, ); } @@ -696,7 +696,7 @@ mod tests { #[test] pub fn real_test_add_cachedevs() { real::test_with_spec( - real::DeviceLimits::AtLeast(2, None, None), + &real::DeviceLimits::AtLeast(2, None, None), test_add_cachedevs, ); } @@ -759,7 +759,7 @@ mod tests { #[test] pub fn loop_test_add_datadevs() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(2, 3, Some((4u64 * Bytes(IEC::Gi)).sectors())), + &loopbacked::DeviceLimits::Range(2, 3, Some((4u64 * Bytes(IEC::Gi)).sectors())), test_add_datadevs, ); } @@ -767,7 +767,7 @@ mod tests { #[test] pub fn real_test_add_datadevs() { real::test_with_spec( - real::DeviceLimits::AtLeast( + &real::DeviceLimits::AtLeast( 2, Some((2u64 * Bytes(IEC::Gi)).sectors()), Some((4u64 * Bytes(IEC::Gi)).sectors()), diff --git a/src/engine/strat_engine/tests/loopbacked.rs b/src/engine/strat_engine/tests/loopbacked.rs index adf0aafda8..555eeb1aa6 100644 --- a/src/engine/strat_engine/tests/loopbacked.rs +++ b/src/engine/strat_engine/tests/loopbacked.rs @@ -67,12 +67,12 @@ impl Drop for LoopTestDev { } /// Get a list of counts of devices to use for tests. -fn get_device_counts(limits: DeviceLimits) -> Vec<(usize, Option)> { +fn get_device_counts(limits: &DeviceLimits) -> Vec<(usize, Option)> { match limits { - DeviceLimits::Exactly(num, size) => vec![(num, size)], + DeviceLimits::Exactly(num, size) => vec![(*num, *size)], DeviceLimits::Range(lower, upper, size) => { assert!(lower < upper); - vec![(lower, size), (upper, size)] + vec![(*lower, *size), (*upper, *size)] } } } @@ -90,11 +90,11 @@ fn get_devices(count: usize, size: Option, dir: &tempfile::TempDir) -> } /// Run the designated tests according to the specification. -pub fn test_with_spec(limits: DeviceLimits, test: F) +pub fn test_with_spec(limits: &DeviceLimits, test: F) where F: Fn(&[&Path]) -> () + panic::RefUnwindSafe, { - let counts = get_device_counts(limits); + let counts = get_device_counts(&limits); init_logger(); diff --git a/src/engine/strat_engine/tests/real.rs b/src/engine/strat_engine/tests/real.rs index d54a9a9b59..785506bd1b 100644 --- a/src/engine/strat_engine/tests/real.rs +++ b/src/engine/strat_engine/tests/real.rs @@ -75,15 +75,15 @@ pub enum DeviceLimits { /// Get a list of lists of devices to use for tests. /// May return an empty list if the request is not satisfiable. fn get_device_runs<'a>( - limits: DeviceLimits, + limits: &DeviceLimits, dev_sizes: &[(&'a Path, Sectors)], ) -> Vec)>> { let (lower, maybe_upper, min_size, max_size) = match limits { - DeviceLimits::Exactly(num, min_size, max_size) => (num, Some(num), min_size, max_size), - DeviceLimits::AtLeast(num, min_size, max_size) => (num, None, min_size, max_size), + DeviceLimits::Exactly(num, min_size, max_size) => (*num, Some(*num), *min_size, *max_size), + DeviceLimits::AtLeast(num, min_size, max_size) => (*num, None, *min_size, *max_size), DeviceLimits::Range(lower, upper, min_size, max_size) => { assert!(lower < upper); - (lower, Some(upper), min_size, max_size) + (*lower, Some(*upper), *min_size, *max_size) } }; @@ -188,7 +188,7 @@ fn make_linear_test_dev(devnode: &Path, start: Sectors, length: Sectors) -> Line /// Run test on real devices, using given constraints. Constraints may result /// in multiple invocations of the test, with differing numbers of block /// devices. -pub fn test_with_spec(limits: DeviceLimits, test: F) +pub fn test_with_spec(limits: &DeviceLimits, test: F) where F: Fn(&[&Path]) -> () + panic::RefUnwindSafe, { @@ -218,7 +218,7 @@ where }) .collect(); - let runs = get_device_runs(limits, &dev_sizes); + let runs = get_device_runs(&limits, &dev_sizes); assert!(!runs.is_empty()); diff --git a/src/engine/strat_engine/thinpool/thinpool.rs b/src/engine/strat_engine/thinpool/thinpool.rs index 5b04c737db..3ec736c719 100644 --- a/src/engine/strat_engine/thinpool/thinpool.rs +++ b/src/engine/strat_engine/thinpool/thinpool.rs @@ -1260,7 +1260,7 @@ mod tests { #[test] pub fn loop_test_greedy_allocation() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(2, 3, None), + &loopbacked::DeviceLimits::Range(2, 3, None), test_greedy_allocation, ); } @@ -1268,7 +1268,7 @@ mod tests { #[test] pub fn real_test_greedy_allocation() { real::test_with_spec( - real::DeviceLimits::AtLeast(2, None, None), + &real::DeviceLimits::AtLeast(2, None, None), test_greedy_allocation, ); } @@ -1368,7 +1368,7 @@ mod tests { #[test] pub fn loop_test_full_pool() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Exactly(2, Some(Bytes(IEC::Gi).sectors())), + &loopbacked::DeviceLimits::Exactly(2, Some(Bytes(IEC::Gi).sectors())), test_full_pool, ); } @@ -1376,7 +1376,7 @@ mod tests { #[test] pub fn real_test_full_pool() { real::test_with_spec( - real::DeviceLimits::Exactly( + &real::DeviceLimits::Exactly( 2, Some(Bytes(IEC::Gi).sectors()), Some(Bytes(IEC::Gi * 4).sectors()), @@ -1480,7 +1480,7 @@ mod tests { #[test] pub fn loop_test_filesystem_snapshot() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(2, 3, None), + &loopbacked::DeviceLimits::Range(2, 3, None), test_filesystem_snapshot, ); } @@ -1488,7 +1488,7 @@ mod tests { #[test] pub fn real_test_filesystem_snapshot() { real::test_with_spec( - real::DeviceLimits::AtLeast(2, None, None), + &real::DeviceLimits::AtLeast(2, None, None), test_filesystem_snapshot, ); } @@ -1530,7 +1530,7 @@ mod tests { #[test] pub fn loop_test_filesystem_rename() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(1, 3, None), + &loopbacked::DeviceLimits::Range(1, 3, None), test_filesystem_rename, ); } @@ -1538,7 +1538,7 @@ mod tests { #[test] pub fn real_test_filesystem_rename() { real::test_with_spec( - real::DeviceLimits::AtLeast(1, None, None), + &real::DeviceLimits::AtLeast(1, None, None), test_filesystem_rename, ); } @@ -1599,12 +1599,15 @@ mod tests { #[test] pub fn loop_test_pool_setup() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(1, 3, None), test_pool_setup); + loopbacked::test_with_spec( + &loopbacked::DeviceLimits::Range(1, 3, None), + test_pool_setup, + ); } #[test] pub fn real_test_pool_setup() { - real::test_with_spec(real::DeviceLimits::AtLeast(1, None, None), test_pool_setup); + real::test_with_spec(&real::DeviceLimits::AtLeast(1, None, None), test_pool_setup); } /// Verify that destroy_filesystems actually deallocates the space /// from the thinpool, by attempting to reinstantiate it using the @@ -1643,7 +1646,7 @@ mod tests { pub fn loop_test_thindev_destroy() { // This test requires more than 1 GiB. loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(2, 3, None), + &loopbacked::DeviceLimits::Range(2, 3, None), test_thindev_destroy, ); } @@ -1651,7 +1654,7 @@ mod tests { #[test] pub fn real_test_thindev_destroy() { real::test_with_spec( - real::DeviceLimits::AtLeast(1, None, None), + &real::DeviceLimits::AtLeast(1, None, None), test_thindev_destroy, ); } @@ -1726,12 +1729,15 @@ mod tests { #[test] pub fn loop_test_xfs_expand() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(1, 3, None), test_xfs_expand); + loopbacked::test_with_spec( + &loopbacked::DeviceLimits::Range(1, 3, None), + test_xfs_expand, + ); } #[test] pub fn real_test_xfs_expand() { - real::test_with_spec(real::DeviceLimits::AtLeast(1, None, None), test_xfs_expand); + real::test_with_spec(&real::DeviceLimits::AtLeast(1, None, None), test_xfs_expand); } /// Just suspend and resume the device and make sure it doesn't crash. @@ -1763,7 +1769,7 @@ mod tests { #[test] pub fn loop_test_suspend_resume() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Range(1, 3, None), + &loopbacked::DeviceLimits::Range(1, 3, None), test_suspend_resume, ); } @@ -1771,7 +1777,7 @@ mod tests { #[test] pub fn real_test_suspend_resume() { real::test_with_spec( - real::DeviceLimits::AtLeast(1, None, None), + &real::DeviceLimits::AtLeast(1, None, None), test_suspend_resume, ); } @@ -1872,11 +1878,14 @@ mod tests { #[test] pub fn loop_test_set_device() { - loopbacked::test_with_spec(loopbacked::DeviceLimits::Range(2, 3, None), test_set_device); + loopbacked::test_with_spec( + &loopbacked::DeviceLimits::Range(2, 3, None), + test_set_device, + ); } #[test] pub fn real_test_set_device() { - real::test_with_spec(real::DeviceLimits::AtLeast(2, None, None), test_set_device); + real::test_with_spec(&real::DeviceLimits::AtLeast(2, None, None), test_set_device); } } diff --git a/src/engine/strat_engine/throttle.rs b/src/engine/strat_engine/throttle.rs index 84b5c67b4d..cead8ed075 100644 --- a/src/engine/strat_engine/throttle.rs +++ b/src/engine/strat_engine/throttle.rs @@ -130,7 +130,7 @@ mod tests { #[test] pub fn loop_test_write_throttling() { loopbacked::test_with_spec( - loopbacked::DeviceLimits::Exactly(1, Some(Bytes(IEC::Mi * 50).sectors())), + &loopbacked::DeviceLimits::Exactly(1, Some(Bytes(IEC::Mi * 50).sectors())), test_write_throttling, ); } @@ -138,7 +138,7 @@ mod tests { #[test] pub fn real_test_write_throttling() { real::test_with_spec( - real::DeviceLimits::Exactly(1, Some(Bytes(IEC::Mi * 50).sectors()), None), + &real::DeviceLimits::Exactly(1, Some(Bytes(IEC::Mi * 50).sectors()), None), test_write_throttling, ); } From 037d0dcbd79e3c46f26677efa8063d831192a694 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 12:33:15 -0500 Subject: [PATCH 18/24] Omit redundant closure Do eta conversion Signed-off-by: mulhern --- src/engine/strat_engine/tests/logger.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/engine/strat_engine/tests/logger.rs b/src/engine/strat_engine/tests/logger.rs index dd5542a571..745a0ab392 100644 --- a/src/engine/strat_engine/tests/logger.rs +++ b/src/engine/strat_engine/tests/logger.rs @@ -11,5 +11,5 @@ static LOGGER_INIT: Once = ONCE_INIT; /// Initialize the logger once. More than one init() attempt returns /// errors. pub fn init_logger() { - LOGGER_INIT.call_once(|| env_logger::init()); + LOGGER_INIT.call_once(env_logger::init); } From d9fcfbb3cfba07bc988b16f077190586ab72537b Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 12:59:43 -0500 Subject: [PATCH 19/24] Allow type complexity for a test function The complex types are not really worth naming, given the function's use. Signed-off-by: mulhern --- src/engine/strat_engine/tests/real.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/engine/strat_engine/tests/real.rs b/src/engine/strat_engine/tests/real.rs index 785506bd1b..4d542130af 100644 --- a/src/engine/strat_engine/tests/real.rs +++ b/src/engine/strat_engine/tests/real.rs @@ -74,6 +74,7 @@ pub enum DeviceLimits { /// Get a list of lists of devices to use for tests. /// May return an empty list if the request is not satisfiable. +#[allow(clippy::type_complexity)] fn get_device_runs<'a>( limits: &DeviceLimits, dev_sizes: &[(&'a Path, Sectors)], From 3e49ac29defc5cf2d4503d0b6450265075f8732b Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 13:03:58 -0500 Subject: [PATCH 20/24] Allow a test function to contain a lot of assertions I think it is doing the correct thing. Signed-off-by: mulhern --- src/engine/strat_engine/names.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/engine/strat_engine/names.rs b/src/engine/strat_engine/names.rs index dd07615192..bb0fcc1fda 100644 --- a/src/engine/strat_engine/names.rs +++ b/src/engine/strat_engine/names.rs @@ -240,6 +240,7 @@ mod tests { use engine::strat_engine::names::validate_name; #[test] + #[allow(clippy::cyclomatic_complexity)] pub fn test_validate_name() { assert!(validate_name(&'\u{0}'.to_string()).is_err()); assert!(validate_name("./some").is_err()); From df81431a3543c60b803e10f6f9294293679b21b8 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 13:23:42 -0500 Subject: [PATCH 21/24] Get rid of a complicated assertion clippy doesn't like it. Also, using assert_matches allows the test, if it fails to be a lot more informative. Get rid of an unused method. Signed-off-by: mulhern --- .../strat_engine/backstore/blockdevmgr.rs | 31 ++++--------------- 1 file changed, 6 insertions(+), 25 deletions(-) diff --git a/src/engine/strat_engine/backstore/blockdevmgr.rs b/src/engine/strat_engine/backstore/blockdevmgr.rs index e8a9edc53f..b6a7f29a26 100644 --- a/src/engine/strat_engine/backstore/blockdevmgr.rs +++ b/src/engine/strat_engine/backstore/blockdevmgr.rs @@ -520,23 +520,6 @@ mod tests { use super::*; - /// Returns true if two usages have the same type. This is needed because - /// if Usage::Theirs(String::from("foo") != Usage::Theirs(String::from("bar"). - /// TODO: See if there is a better way to solve this. - fn usage_equal(left: &DevOwnership, right: &DevOwnership) -> bool { - if left == right { - true - } else { - match left { - DevOwnership::Theirs(_) => match right { - DevOwnership::Theirs(_) => true, - _ => false, - }, - _ => false, - } - } - } - /// Verify that initially, /// size() - metadata_size() = avail_space(). /// After 2 Sectors have been allocated, that amount must also be included @@ -587,15 +570,13 @@ mod tests { let pool_uuid = Uuid::new_v4(); assert!(BlockDevMgr::initialize(pool_uuid, paths, MIN_MDA_SECTORS).is_err()); - assert!(paths.iter().enumerate().all(|(i, path)| { - let tmp = if i == index { - DevOwnership::Theirs(String::from("")) + for (i, path) in paths.iter().enumerate() { + if i == index { + assert_matches!(identify(path).unwrap(), DevOwnership::Theirs(_)); } else { - DevOwnership::Unowned - }; - - usage_equal(&identify(path).unwrap(), &tmp) - })); + assert_matches!(identify(path).unwrap(), DevOwnership::Unowned); + } + } // Clear out the beginning of the device and make sure we succeed now. wipe_sectors(paths[index], Sectors(0), MIN_MDA_SECTORS).unwrap(); From ceda9287579c08460ed11343818e570878106c54 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 13:28:12 -0500 Subject: [PATCH 22/24] Get rid of a complicated closure in an assert clippy doesn't like it and using assert_eq! will yield a better error message. Signed-off-by: mulhern --- .../strat_engine/backstore/blockdevmgr.rs | 55 +++++++++++-------- 1 file changed, 32 insertions(+), 23 deletions(-) diff --git a/src/engine/strat_engine/backstore/blockdevmgr.rs b/src/engine/strat_engine/backstore/blockdevmgr.rs index b6a7f29a26..885fb5c715 100644 --- a/src/engine/strat_engine/backstore/blockdevmgr.rs +++ b/src/engine/strat_engine/backstore/blockdevmgr.rs @@ -585,14 +585,17 @@ mod tests { assert!(BlockDevMgr::initialize(pool_uuid, paths, MIN_MDA_SECTORS).is_ok()); cmd::udev_settle().unwrap(); - assert!(paths.iter().all(|path| { - let (t_pool_uuid, _) = StaticHeader::device_identifiers( - &mut OpenOptions::new().read(true).open(path).unwrap(), - ) - .unwrap() - .unwrap(); - pool_uuid == t_pool_uuid - })); + for path in paths { + assert_eq!( + pool_uuid, + StaticHeader::device_identifiers( + &mut OpenOptions::new().read(true).open(path).unwrap(), + ) + .unwrap() + .unwrap() + .0 + ); + } } #[test] @@ -745,23 +748,29 @@ mod tests { cmd::udev_settle().unwrap(); - assert!(paths.iter().all(|path| { - let (t_pool_uuid, _) = StaticHeader::device_identifiers( - &mut OpenOptions::new().read(true).open(path).unwrap(), - ) - .unwrap() - .unwrap(); - pool_uuid == t_pool_uuid - })); + for path in paths { + assert_eq!( + pool_uuid, + StaticHeader::device_identifiers( + &mut OpenOptions::new().read(true).open(path).unwrap(), + ) + .unwrap() + .unwrap() + .0 + ); + } bd_mgr.destroy_all().unwrap(); - assert!(paths.iter().all(|path| { - let id = StaticHeader::device_identifiers( - &mut OpenOptions::new().read(true).open(path).unwrap(), - ) - .unwrap(); - id.is_none() - })); + + for path in paths { + assert_eq!( + StaticHeader::device_identifiers( + &mut OpenOptions::new().read(true).open(path).unwrap(), + ) + .unwrap(), + None + ); + } } #[test] From 878e5cb0b8327f40d03d71e49947a7c633e6117c Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 13:44:22 -0500 Subject: [PATCH 23/24] Use read_exact or write_all instead of read or write In all cases, it seems to be what is actually intended. Signed-off-by: mulhern --- src/engine/strat_engine/backstore/metadata.rs | 4 ++-- src/engine/strat_engine/pool.rs | 6 +++--- src/engine/strat_engine/thinpool/thinpool.rs | 6 +++--- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/engine/strat_engine/backstore/metadata.rs b/src/engine/strat_engine/backstore/metadata.rs index 44aaf23c9d..7050193cac 100644 --- a/src/engine/strat_engine/backstore/metadata.rs +++ b/src/engine/strat_engine/backstore/metadata.rs @@ -964,10 +964,10 @@ mod tests { { let mut byte_to_corrupt = [0; 1]; f.seek(SeekFrom::Start(position))?; - f.read(&mut byte_to_corrupt)?; + f.read_exact(&mut byte_to_corrupt)?; byte_to_corrupt[0] = !byte_to_corrupt[0]; f.seek(SeekFrom::Start(position))?; - f.write(&byte_to_corrupt)?; + f.write_all(&byte_to_corrupt)?; f.sync_all()?; Ok(()) } diff --git a/src/engine/strat_engine/pool.rs b/src/engine/strat_engine/pool.rs index 2de83d4780..afadc4a2f9 100644 --- a/src/engine/strat_engine/pool.rs +++ b/src/engine/strat_engine/pool.rs @@ -625,7 +625,7 @@ mod tests { .write(true) .open(&new_file) .unwrap() - .write(bytestring) + .write_all(bytestring) .unwrap(); } @@ -642,7 +642,7 @@ mod tests { .read(true) .open(&new_file) .unwrap() - .read(&mut buf) + .read_exact(&mut buf) .unwrap(); } assert_eq!(&buf, bytestring); @@ -678,7 +678,7 @@ mod tests { .read(true) .open(&new_file) .unwrap() - .read(&mut buf) + .read_exact(&mut buf) .unwrap(); } assert_eq!(&buf, bytestring); diff --git a/src/engine/strat_engine/thinpool/thinpool.rs b/src/engine/strat_engine/thinpool/thinpool.rs index 3ec736c719..612d13873e 100644 --- a/src/engine/strat_engine/thinpool/thinpool.rs +++ b/src/engine/strat_engine/thinpool/thinpool.rs @@ -1471,7 +1471,7 @@ mod tests { .path() .join(format!("stratis_test{}.txt", i)); let mut f = OpenOptions::new().read(true).open(file_path).unwrap(); - f.read(&mut read_buf).unwrap(); + f.read_exact(&mut read_buf).unwrap(); assert_eq!(read_buf[0..SECTOR_SIZE], write_buf[0..SECTOR_SIZE]); } } @@ -1829,7 +1829,7 @@ mod tests { .write(true) .open(&new_file) .unwrap() - .write(bytestring) + .write_all(bytestring) .unwrap(); } let filesystem_saves = pool.mdv.filesystems().unwrap(); @@ -1860,7 +1860,7 @@ mod tests { .read(true) .open(&new_file) .unwrap() - .read(&mut buf) + .read_exact(&mut buf) .unwrap(); } assert_eq!(&buf, bytestring); From aad4451bb9cce3d37c3293c93e0188348290a409 Mon Sep 17 00:00:00 2001 From: mulhern Date: Fri, 21 Dec 2018 14:28:58 -0500 Subject: [PATCH 24/24] Do not allow clippy failure in Travis tests All clippy lints and other clippy problems are dealt with now. Signed-off-by: mulhern --- .travis.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index e40d10266e..3494f97149 100644 --- a/.travis.yml +++ b/.travis.yml @@ -13,9 +13,6 @@ addons: language: rust matrix: - allow_failures: - # Temporarily allow failure until after next release. - - env: TASK=clippy TARGET=x86_64-unknown-linux-gnu include: # Verify formatting of Rust code - rust: stable