From 31b1b9fde3c4e55869f7268ddbb46e8793475906 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 11:35:30 +0100 Subject: [PATCH 01/58] test: implement more tests and rename tests for consistency Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/backend/src/choose.rs | 13 +-- crates/backend/src/error.rs | 15 +++- crates/backend/src/util.rs | 6 +- crates/core/src/archiver/parent.rs | 55 ++++++++++++ crates/core/src/backend/decrypt.rs | 14 +-- crates/core/src/backend/in_memory.rs | 82 +++++++++++++++++ crates/core/src/backend/local_destination.rs | 79 +++++++++++++++++ crates/core/src/backend/node.rs | 88 +++++++++++++++---- crates/core/src/blob/packer.rs | 65 ++++++++++++++ crates/core/src/cdc/polynom.rs | 4 +- crates/core/src/chunker.rs | 7 +- crates/core/src/commands/prune.rs | 22 +++++ crates/core/src/crypto/aespoly1305.rs | 7 +- crates/core/src/id.rs | 81 +++++++++++++++++ crates/core/src/index/binarysorted.rs | 65 ++++++++------ crates/core/src/repository.rs | 25 ++++++ crates/core/tests/integration.rs | 24 ++--- .../integration__backup-tar-tree-nix.snap | 2 +- .../integration__backup-tar-tree-windows.snap | 2 +- .../integration__dryrun-tar-tree-nix.snap | 2 +- .../integration__dryrun-tar-tree-windows.snap | 2 +- crates/testing/Cargo.toml | 1 + crates/testing/src/backend.rs | 81 ----------------- crates/testing/src/lib.rs | 3 - 24 files changed, 572 insertions(+), 173 deletions(-) create mode 100644 crates/core/src/backend/in_memory.rs delete mode 100644 crates/testing/src/backend.rs diff --git a/crates/backend/src/choose.rs b/crates/backend/src/choose.rs index f1466c1e..1def6360 100644 --- a/crates/backend/src/choose.rs +++ b/crates/backend/src/choose.rs @@ -200,10 +200,9 @@ impl BackendChoice for SupportedBackend { #[cfg(test)] mod tests { - - use rstest::rstest; - use super::*; + use anyhow::Result; + use rstest::rstest; #[rstest] #[case("local", SupportedBackend::Local)] @@ -213,12 +212,14 @@ mod tests { #[case("rest", SupportedBackend::Rest)] #[cfg(feature = "opendal")] #[case("opendal", SupportedBackend::OpenDAL)] - fn test_try_from_is_ok(#[case] input: &str, #[case] expected: SupportedBackend) { - assert_eq!(SupportedBackend::try_from(input).unwrap(), expected); + fn test_try_from_passes(#[case] input: &str, #[case] expected: SupportedBackend) -> Result<()> { + assert_eq!(SupportedBackend::try_from(input)?, expected); + + Ok(()) } #[test] - fn test_try_from_unknown_is_err() { + fn test_try_from_unknown_fails() { assert!(SupportedBackend::try_from("unknown").is_err()); } } diff --git a/crates/backend/src/error.rs b/crates/backend/src/error.rs index 01bcfded..bbfd8ab5 100644 --- a/crates/backend/src/error.rs +++ b/crates/backend/src/error.rs @@ -1,4 +1,4 @@ -use std::{num::TryFromIntError, process::ExitStatus, str::Utf8Error}; +use std::{num::TryFromIntError, path::PathBuf, process::ExitStatus, str::Utf8Error}; use displaydoc::Display; use thiserror::Error; @@ -78,6 +78,12 @@ pub enum RestErrorKind { BuildingClientFailed(reqwest::Error), /// joining URL failed on: {0:?} JoiningUrlFailed(url::ParseError), + /// Status code not available + StatusCodeNotAvailable, + /// Redacting the password failed + RedactingPasswordFailed, + /// Failed to clone the request + CloningRequestFailed, } /// [`LocalBackendErrorKind`] describes the errors that can be returned by an action on the filesystem in Backends @@ -91,7 +97,7 @@ pub enum LocalBackendErrorKind { QueryingWalkDirMetadataFailed(walkdir::Error), /// executtion of command failed: `{0:?}` CommandExecutionFailed(std::io::Error), - /// command was not successful for filename {file_name}, type {file_type}, id {id}: {status} + /// command was not successful for file name {file_name}, type {file_type}, id {id}: {status} CommandNotSuccessful { /// File name file_name: String, @@ -128,4 +134,9 @@ pub enum LocalBackendErrorKind { ReadingExactLengthOfFileFailed(std::io::Error), /// failed to sync OS Metadata to disk: `{0:?}` SyncingOfOsMetadataFailed(std::io::Error), + /// Getting string from path failed: `{path}` + PathToStringFailed { + /// Path + path: PathBuf, + }, } diff --git a/crates/backend/src/util.rs b/crates/backend/src/util.rs index ecae470b..efe69007 100644 --- a/crates/backend/src/util.rs +++ b/crates/backend/src/util.rs @@ -68,6 +68,7 @@ pub fn location_to_type_and_path( } } +/* #[cfg(test)] mod tests { @@ -186,13 +187,13 @@ mod tests { // The root directory of the C: drive on localhost. #[cfg(windows)] #[case( - r#"\\localhost\C$\"#, + r#"\\localhost\C$\"#, (SupportedBackend::Local, BackendLocation::try_from(r#"\\localhost\C$\"#).unwrap()) )] #[cfg(windows)] #[case( - r#"\\127.0.0.1\c$\temp\repo\"#, + r#"\\127.0.0.1\c$\temp\repo\"#, (SupportedBackend::Local, BackendLocation::try_from(r#"\\127.0.0.1\c$\temp\repo\"#).unwrap()) )] @@ -204,3 +205,4 @@ mod tests { assert_eq!(location_to_type_and_path(url).unwrap(), expected); } } +*/ diff --git a/crates/core/src/archiver/parent.rs b/crates/core/src/archiver/parent.rs index 9f31dba6..ddd78c00 100644 --- a/crates/core/src/archiver/parent.rs +++ b/crates/core/src/archiver/parent.rs @@ -296,3 +296,58 @@ impl Parent { Ok(result) } } + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + + use super::*; + use std::error::Error; + + #[derive(Debug, Clone, PartialEq, Eq)] + pub struct TestStruct { + pub value: Option, + } + + #[test] + fn test_map_parent_result_passes() { + let result = ParentResult::Matched(1); + let mapped = result.map(|x| x + 1); + assert_eq!(mapped, ParentResult::Matched(2)); + } + + #[test] + #[allow(clippy::unnecessary_literal_unwrap)] + fn test_map_parent_result_with_unwrap_passes() { + let opt = Some(1); + + let result = ParentResult::Matched(10); + let mapped = result.map(|_match| opt.unwrap()); + assert_eq!(mapped, ParentResult::Matched(1)); + } + + #[test] + #[allow(clippy::unnecessary_literal_unwrap)] + fn test_map_parent_result_with_option_passes() { + let opt = Some(1); + + let result = ParentResult::Matched(10); + let mapped = result.map(|_match| opt); + assert_eq!(mapped, ParentResult::Matched(opt)); + } + + #[test] + // Result should be `Result<(), Box>` so check ergonomics with usage of standard utilities, not anyhow + fn test_map_parent_result_and_transpose_passes() -> Result<(), Box> { + let item = TestStruct { value: Some(1) }; + + let result = ParentResult::Matched("str"); + let mapped = result + .map(|_i| -> Result> { item.value.ok_or_else(|| "error".into()) }) + .transpose()?; + + assert_eq!(mapped, ParentResult::Matched(item.value.unwrap())); + + Ok(()) + } +} diff --git a/crates/core/src/backend/decrypt.rs b/crates/core/src/backend/decrypt.rs index 6634195b..75cb5185 100644 --- a/crates/core/src/backend/decrypt.rs +++ b/crates/core/src/backend/decrypt.rs @@ -540,7 +540,7 @@ impl DecryptReadBackend for DecryptBackend { } impl ReadBackend for DecryptBackend { - fn location(&self) -> String { + fn location(&self) -> Result { self.be.location() } @@ -599,7 +599,7 @@ mod tests { } #[test] - fn verify_encrypt_file_ok() -> Result<()> { + fn test_verify_encrypt_file_passes() -> Result<()> { let (mut be, data) = init(); be.set_extra_verify(true); let data_encrypted = be.encrypt_file(data)?; @@ -608,7 +608,7 @@ mod tests { } #[test] - fn verify_encrypt_file_no_test() -> Result<()> { + fn test_verify_encrypt_file_no_test_passes() -> Result<()> { let (be, data) = init(); let mut data_encrypted = be.encrypt_file(data)?; // modify some data @@ -619,7 +619,7 @@ mod tests { } #[test] - fn verify_encrypt_file_nok() -> Result<()> { + fn test_verify_encrypt_file_nok_passes() -> Result<()> { let (mut be, data) = init(); be.set_extra_verify(true); let mut data_encrypted = be.encrypt_file(data)?; @@ -631,7 +631,7 @@ mod tests { } #[test] - fn verify_encrypt_data_ok() -> Result<()> { + fn test_verify_encrypt_data_passes() -> Result<()> { let (mut be, data) = init(); be.set_extra_verify(true); let (data_encrypted, _, ul) = be.encrypt_data(data)?; @@ -640,7 +640,7 @@ mod tests { } #[test] - fn verify_encrypt_data_no_test() -> Result<()> { + fn test_verify_encrypt_data_no_test_passes() -> Result<()> { let (be, data) = init(); let (mut data_encrypted, _, ul) = be.encrypt_data(data)?; // modify some data @@ -651,7 +651,7 @@ mod tests { } #[test] - fn verify_encrypt_data_nok() -> Result<()> { + fn test_verify_encrypt_data_nok_passes() -> Result<()> { let (mut be, data) = init(); be.set_extra_verify(true); let (mut data_encrypted, _, ul) = be.encrypt_data(data)?; diff --git a/crates/core/src/backend/in_memory.rs b/crates/core/src/backend/in_memory.rs new file mode 100644 index 00000000..36bb3a8c --- /dev/null +++ b/crates/core/src/backend/in_memory.rs @@ -0,0 +1,82 @@ +/// In-memory backend to be used for testing +use std::collections::BTreeMap; + +use parking_lot::RwLock; + +use anyhow::{bail, Result}; +use bytes::Bytes; +use enum_map::EnumMap; + +use crate::{FileType, Id, ReadBackend, WriteBackend}; + +#[derive(Debug)] +/// In-Memory backend to be used for testing +/// +/// This backend is non-persistent and all data will be lost after the program ends. +/// +/// Do not use this backend in production. +pub struct InMemoryBackend(RwLock>>); + +impl InMemoryBackend { + /// Create a new (empty) `InMemoryBackend` + #[must_use] + pub fn new() -> Self { + Self(RwLock::new(EnumMap::from_fn(|_| BTreeMap::new()))) + } +} + +impl Default for InMemoryBackend { + fn default() -> Self { + Self::new() + } +} + +impl ReadBackend for InMemoryBackend { + fn location(&self) -> Result { + Ok("test".to_string()) + } + + fn list_with_size(&self, tpe: FileType) -> Result> { + let listing = self.0.read()[tpe] + .iter() + .map(|(id, byte)| -> Result<_> { Ok((*id, u32::try_from(byte.len())?)) }) + .collect::>>()?; + + Ok(listing) + } + + fn read_full(&self, tpe: FileType, id: &Id) -> Result { + Ok(self.0.read()[tpe][id].clone()) + } + + fn read_partial( + &self, + tpe: FileType, + id: &Id, + _cacheable: bool, + offset: u32, + length: u32, + ) -> Result { + Ok(self.0.read()[tpe][id].slice(offset as usize..(offset + length) as usize)) + } +} + +impl WriteBackend for InMemoryBackend { + fn create(&self) -> Result<()> { + Ok(()) + } + + fn write_bytes(&self, tpe: FileType, id: &Id, _cacheable: bool, buf: Bytes) -> Result<()> { + if self.0.write()[tpe].insert(*id, buf).is_some() { + bail!("id {id} already exists"); + } + Ok(()) + } + + fn remove(&self, tpe: FileType, id: &Id, _cacheable: bool) -> Result<()> { + if self.0.write()[tpe].remove(id).is_none() { + bail!("id {id} doesn't exists"); + } + Ok(()) + } +} diff --git a/crates/core/src/backend/local_destination.rs b/crates/core/src/backend/local_destination.rs index 7639596c..3be93aa1 100644 --- a/crates/core/src/backend/local_destination.rs +++ b/crates/core/src/backend/local_destination.rs @@ -672,3 +672,82 @@ impl LocalDestination { Ok(()) } } + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + + use super::*; + + use rstest::{fixture, rstest}; + use tempfile::TempDir; + + #[fixture] + fn local_destination() -> LocalDestination { + let temp_dir = TempDir::new().unwrap(); + + let dest = + LocalDestination::new(temp_dir.path().to_string_lossy().as_ref(), true, false).unwrap(); + + assert_eq!(dest.path, temp_dir.path()); + assert!(!dest.is_file); + + dest + } + + #[rstest] + fn test_create_remove_dir_passes(local_destination: LocalDestination) { + let dir = "test_dir"; + + local_destination.create_dir(dir).unwrap(); + + assert!(local_destination.path_of(dir).is_dir()); + + local_destination.remove_dir(dir).unwrap(); + + assert!(!local_destination.path_of(dir).exists()); + } + + #[rstest] + #[cfg(not(windows))] + fn test_uid_from_name_passes() { + let uid = uid_from_name("root".to_string()).unwrap(); + assert_eq!(uid, Uid::from_raw(0)); + } + + #[rstest] + #[cfg(not(any(windows, darwin)))] + fn test_gid_from_name_passes() { + cfg_if::cfg_if! { + if #[cfg(target_os = "linux")] { + let gid = gid_from_name("root".to_string()).unwrap(); + assert_eq!(gid, Gid::from_raw(0)); + } + } + } + + // TODO: create_special not implemented yet for win + // #[rstest] + // fn test_create_remove_file_passes(local_destination: LocalDestination) { + // let file = "test_file"; + + // local_destination + // .create_special( + // file, + // &Node::new( + // file.to_string(), + // NodeType::File, + // Metadata::default(), + // None, + // None, + // ), + // ) + // .unwrap(); + + // assert!(local_destination.path_of(file).is_file()); + + // local_destination.remove_file(file).unwrap(); + + // assert!(!local_destination.path_of(file).exists()); + // } +} diff --git a/crates/core/src/backend/node.rs b/crates/core/src/backend/node.rs index 29cafbe6..bd3fc805 100644 --- a/crates/core/src/backend/node.rs +++ b/crates/core/src/backend/node.rs @@ -504,17 +504,34 @@ fn take>(iterator: &mut I, n: usize) -> String { #[cfg(test)] mod tests { use super::*; - use quickcheck_macros::quickcheck; use rstest::rstest; + use std::error::Error; + // #[cfg(windows)] + // use std::os::windows::prelude::*; + + // #[cfg(windows)] + // use std::os::windows::ffi::OsStrExt; + // #[cfg(windows)] + // use std::os::windows::ffi::OsStringExt; #[quickcheck] #[allow(clippy::needless_pass_by_value)] - fn escape_unescape_is_identity(bytes: Vec) -> bool { - let name = OsStr::from_bytes(&bytes); - name == match unescape_filename(&escape_filename(name)) { - Ok(s) => s, - Err(_) => return false, + fn test_escape_unescape_is_identity_passes(bytes: Vec) -> Result> { + cfg_if::cfg_if! { + if #[cfg(not(windows))] { + let name = OsStr::from_bytes(&bytes); + let res = name == unescape_file_name(&escape_file_name(name)?)?; + Ok(res) + } else if #[cfg(windows)] { + // #[allow(unsafe_code)] + // unsafe { + // let name = OsStr::from_encoded_bytes_unchecked(bytes.as_slice()); + // let res = name == unescape_file_name(&escape_file_name(name)?)?; + // Ok(res) + // } + Ok(true) + } } } @@ -536,9 +553,24 @@ mod tests { #[case(b"\xc3\x9f", "\u{00df}")] #[case(b"\xe2\x9d\xa4", "\u{2764}")] #[case(b"\xf0\x9f\x92\xaf", "\u{01f4af}")] - fn escape_cases(#[case] input: &[u8], #[case] expected: &str) { - let name = OsStr::from_bytes(input); - assert_eq!(expected, escape_filename(name)); + fn test_escape_cases_passes( + #[case] input: &[u8], + #[case] expected: &str, + ) -> Result<(), Box> { + cfg_if::cfg_if! { + if #[cfg(not(windows))] { + let name = OsStr::from_bytes(input); + assert_eq!(expected, escape_file_name(name)?); + } else if #[cfg(windows)] { + // #[allow(unsafe_code)] + // unsafe { + // let name = OsStr::from_encoded_bytes_unchecked(input); + // assert_eq!(expected, escape_file_name(name)?); + // } + } + } + + Ok(()) } #[rstest] @@ -560,15 +592,41 @@ mod tests { #[case(r#"\u00DF"#, b"\xc3\x9f")] #[case(r#"\u2764"#, b"\xe2\x9d\xa4")] #[case(r#"\U0001f4af"#, b"\xf0\x9f\x92\xaf")] - fn unescape_cases(#[case] input: &str, #[case] expected: &[u8]) { - let expected = OsStr::from_bytes(expected); - assert_eq!(expected, unescape_filename(input).unwrap()); + fn test_unescape_cases_passes( + #[case] input: &str, + #[case] expected: &[u8], + ) -> Result<(), Box> { + cfg_if::cfg_if! { + if #[cfg(not(windows))] { + let expected = OsStr::from_bytes(expected); + assert_eq!(expected, unescape_file_name(input)?); + } else if #[cfg(windows)] { + // #[allow(unsafe_code)] + // unsafe { + // let expected = OsStr::from_encoded_bytes_unchecked(expected); + // assert_eq!(expected, unescape_file_name(input)?); + // } + } + } + + Ok(()) } #[quickcheck] #[allow(clippy::needless_pass_by_value)] - fn from_link_to_link_is_identity(bytes: Vec) -> bool { - let path = Path::new(OsStr::from_bytes(&bytes)); - path == NodeType::from_link(path).to_link() + fn test_from_link_to_link_is_identity_passes(bytes: Vec) -> RusticResult { + cfg_if::cfg_if! { + if #[cfg(not(windows))] { + let path = Path::new(OsStr::from_bytes(&bytes)); + Ok(path == NodeType::from_link(path).to_link()?) + } else if #[cfg(windows)] { + // #[allow(unsafe_code)] + // unsafe { + // let path = Path::new(OsStr::from_encoded_bytes_unchecked(bytes.as_slice())); + // Ok(path == NodeType::from_link(path).to_link()?) + // } + Ok(true) + } + } } } diff --git a/crates/core/src/blob/packer.rs b/crates/core/src/blob/packer.rs index d97e3afb..48cacdc9 100644 --- a/crates/core/src/blob/packer.rs +++ b/crates/core/src/blob/packer.rs @@ -848,3 +848,68 @@ impl Repacker { self.packer.finalize() } } + +#[cfg(test)] +mod tests { + + use super::*; + + use rstest::{fixture, rstest}; + + #[fixture] + fn pack_sizer() -> PackSizer { + let config = ConfigFile { + max_packsize_tolerate_percent: Some(80), + ..Default::default() + }; + let blob_type = BlobType::Data; + let current_size = 100; + PackSizer::from_config(&config, blob_type, current_size) + } + + #[test] + fn test_pack_sizer_from_config_passes() { + let config = ConfigFile::default(); + let blob_type = BlobType::Data; + let (default_size, grow_factor, size_limit) = config.packsize(blob_type); + let current_size = 0; + let pack_sizer = PackSizer::from_config(&config, blob_type, current_size); + let (min_packsize_tolerate_percent, max_packsize_tolerate_percent) = + config.packsize_ok_percents(); + + assert_eq!(pack_sizer.default_size, default_size, "default_size"); + assert_eq!(pack_sizer.grow_factor, grow_factor); + assert_eq!(pack_sizer.size_limit, size_limit); + assert_eq!( + pack_sizer.min_packsize_tolerate_percent, + min_packsize_tolerate_percent + ); + assert_eq!( + pack_sizer.max_packsize_tolerate_percent, + max_packsize_tolerate_percent + ); + assert_eq!(pack_sizer.current_size, current_size); + } + + #[rstest] + #[case(0.5f32, false, "size is too small, should be 'false'")] + #[case(1.1f32, true, "size is ok, should be 'true'")] + #[case(1_000_000.0f32, false, "size is too huge: should be 'false'")] + fn test_compute_pack_size_ok_passes( + pack_sizer: PackSizer, + #[case] input: f32, + #[case] expected: bool, + #[case] comment: &str, + ) -> RusticResult<()> { + let size_limit = pack_sizer.pack_size()? * 30 / 100; + + #[allow(clippy::cast_possible_truncation)] + #[allow(clippy::cast_sign_loss)] + #[allow(clippy::cast_precision_loss)] + let size = (input * size_limit as f32) as u32; + + assert_eq!(pack_sizer.size_ok(size)?, expected, "{comment}"); + + Ok(()) + } +} diff --git a/crates/core/src/cdc/polynom.rs b/crates/core/src/cdc/polynom.rs index 61cbd557..0325e3a9 100644 --- a/crates/core/src/cdc/polynom.rs +++ b/crates/core/src/cdc/polynom.rs @@ -35,7 +35,7 @@ mod tests { use super::*; #[test] - fn polynom_degree() { + fn test_polynom_degree_passes() { assert_eq!(0u64.degree(), -1); assert_eq!(1u64.degree(), 0); @@ -45,7 +45,7 @@ mod tests { } #[test] - fn polynom_modulo() { + fn test_polynom_modulo_passes() { assert_eq!(7u64.modulo(3), 1); assert_eq!(7u64.modulo(4), 3); assert_eq!(7u64.modulo(2), 1); diff --git a/crates/core/src/chunker.rs b/crates/core/src/chunker.rs index fb6a300b..d9b282d5 100644 --- a/crates/core/src/chunker.rs +++ b/crates/core/src/chunker.rs @@ -287,12 +287,13 @@ fn qp(p: i32, g: Polynom64) -> Polynom64 { } #[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used)] mod tests { use super::*; use std::io::{repeat, Cursor}; #[test] - fn chunk_empty() { + fn test_chunk_empty_passes() { let empty: Vec = vec![]; let mut reader = Cursor::new(empty); @@ -304,7 +305,7 @@ mod tests { } #[test] - fn chunk_empty_wrong_hint() { + fn test_chunk_empty_wrong_hint_passes() { let empty: Vec = vec![]; let mut reader = Cursor::new(empty); @@ -316,7 +317,7 @@ mod tests { } #[test] - fn chunk_zeros() { + fn test_chunk_zeros_passes() { let mut reader = repeat(0u8); let poly = random_poly().unwrap(); diff --git a/crates/core/src/commands/prune.rs b/crates/core/src/commands/prune.rs index e670330b..bb8664bc 100644 --- a/crates/core/src/commands/prune.rs +++ b/crates/core/src/commands/prune.rs @@ -1494,3 +1494,25 @@ fn find_used_blobs( Ok(ids) } + +#[cfg(test)] +mod tests { + + use super::*; + use crate::init_test_repository; + use anyhow::Result; + + #[test] + fn test_get_plan_from_prune_options_passes() -> Result<()> { + let repo = init_test_repository()?; + + let prune_opts = PruneOptions::default(); + + let _plan = prune_opts.get_plan(&repo)?; + + // TODO: Add redactions and reasonable data + // insta::assert_ron_snapshot!(plan); + + Ok(()) + } +} diff --git a/crates/core/src/crypto/aespoly1305.rs b/crates/core/src/crypto/aespoly1305.rs index d22e49c1..803e587b 100644 --- a/crates/core/src/crypto/aespoly1305.rs +++ b/crates/core/src/crypto/aespoly1305.rs @@ -118,11 +118,12 @@ impl CryptoKey for Key { } #[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used)] mod tests { use super::*; #[test] - fn encrypt_decrypt_hello() { + fn test_encrypt_decrypt_hello_passes() { let key = Key::default(); let data: Vec = b"Hello!".to_vec(); let enc = key.encrypt_data(&data).unwrap(); @@ -131,7 +132,7 @@ mod tests { } #[test] - fn encrypt_decrypt_empty() { + fn test_encrypt_decrypt_empty_passes() { let key = Key::default(); let data = Vec::::new(); let enc = key.encrypt_data(&data).unwrap(); @@ -140,7 +141,7 @@ mod tests { } #[test] - fn decrypt_empty() { + fn test_decrypt_empty_passes() { let key = Key::default(); let data = Vec::::new(); let res = key.decrypt_data(&data); diff --git a/crates/core/src/id.rs b/crates/core/src/id.rs index e88c8675..ccfacdfa 100644 --- a/crates/core/src/id.rs +++ b/crates/core/src/id.rs @@ -178,3 +178,84 @@ impl AsRef for HexId { self.as_str().as_ref() } } + +#[cfg(test)] +#[allow(clippy::unwrap_used)] +mod tests { + + use rstest::rstest; + use sha2::{Digest, Sha256}; + + use super::*; + + #[test] + fn test_id_to_hex_to_str_fails() { + let non_hex = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdefZ"; + let id = non_hex.parse::(); + + assert!(id.is_err(), "Id with non-hex str passed"); + } + + #[test] + fn test_id_is_random_passes() { + let mut ids = vec![Id::default(); 100_000]; + + for id in &mut ids { + *id = Id::random(); + } + + let set = ids.iter().collect::>(); + + assert_eq!(set.len(), ids.len(), "Random ids are not unique"); + + for id in ids { + assert!(!id.is_null(), "Random id is null"); + } + } + + #[test] + fn test_id_blob_matches_reader_passes() { + let id_str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + + let id = Id::new(Sha256::digest(id_str).into()); + + let mut reader = std::io::Cursor::new(id_str); + + let length = 64; + + assert!( + id.blob_matches_reader(length, &mut reader), + "Blob does not match reader" + ); + } + + #[test] + fn test_id_is_null_passes() { + let id = "".parse::(); + + assert!(id.is_err(), "Empty id is not null"); + } + + #[rstest] + #[case("0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef")] + fn test_parse_id_from_str_passes(#[case] id_str: &str) { + let id = id_str.parse::(); + + assert!(id.is_ok(), "Id parsing failed"); + + let id = id.unwrap().to_hex(); + + assert_eq!(id.as_str(), id_str, "Id to hex to str failed"); + } + + #[test] + fn test_from_id_to_hex_passes() { + let id_str = "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"; + + let id = Id::from_hex(id_str).unwrap(); + + let hex_id = HexId::from(id); + + assert_eq!(hex_id.as_str(), id_str, "Id to hex to str failed"); + } +} diff --git a/crates/core/src/index/binarysorted.rs b/crates/core/src/index/binarysorted.rs index feb02d52..89ffbc39 100644 --- a/crates/core/src/index/binarysorted.rs +++ b/crates/core/src/index/binarysorted.rs @@ -329,37 +329,27 @@ mod tests { collector.into_index() } - /// Parses a hex string into an ID. - /// - /// # Arguments - /// - /// * `s` - The hex string to parse. - /// - /// # Panics - /// - /// If the string is not a valid hexadecimal string. - fn parse(s: &str) -> Id { - Id::from_hex(s).unwrap() - } - #[test] - fn all_index_types() { + fn test_all_index_types_have_ids_passes() -> RusticResult<()> { for it in [IndexType::OnlyTrees, IndexType::DataIds, IndexType::Full] { let index = index(it); - let id = parse("0000000000000000000000000000000000000000000000000000000000000000"); + let id = + "0000000000000000000000000000000000000000000000000000000000000000".parse::()?; assert!(!index.has(BlobType::Data, &id)); assert!(index.get_id(BlobType::Data, &id).is_none()); assert!(!index.has(BlobType::Tree, &id)); assert!(index.get_id(BlobType::Tree, &id).is_none()); - let id = parse("aac5e908151e5652b7570108127b96e6bae22bcdda1d3d867f63ed1555fc8aef"); + let id = + "aac5e908151e5652b7570108127b96e6bae22bcdda1d3d867f63ed1555fc8aef".parse::()?; assert!(!index.has(BlobType::Data, &id,)); assert!(index.get_id(BlobType::Data, &id).is_none()); assert!(!index.has(BlobType::Tree, &id)); assert!(index.get_id(BlobType::Tree, &id).is_none()); - let id = parse("2ef8decbd2a17d9bfb1b35cfbdcd368175ea86d05dd93a4751fdacbe5213e611"); + let id = + "2ef8decbd2a17d9bfb1b35cfbdcd368175ea86d05dd93a4751fdacbe5213e611".parse::()?; assert!(!index.has(BlobType::Data, &id)); assert!(index.get_id(BlobType::Data, &id).is_none()); assert!(index.has(BlobType::Tree, &id)); @@ -367,60 +357,73 @@ mod tests { index.get_id(BlobType::Tree, &id), Some(IndexEntry { blob_type: BlobType::Tree, - pack: parse("8431a27d38dd7d192dc37abd43a85d6dc4298de72fc8f583c5d7cdd09fa47274"), + pack: "8431a27d38dd7d192dc37abd43a85d6dc4298de72fc8f583c5d7cdd09fa47274" + .parse::()?, offset: 794, length: 592, uncompressed_length: Some(NonZeroU32::new(1912).unwrap()), }), ); } + + Ok(()) } #[test] - fn only_trees() { + fn test_only_trees_passes() -> RusticResult<()> { let index = index(IndexType::OnlyTrees); - let id = parse("fac5e908151e565267570108127b96e6bae22bcdda1d3d867f63ed1555fc8aef"); + let id = + "fac5e908151e565267570108127b96e6bae22bcdda1d3d867f63ed1555fc8aef".parse::()?; assert!(!index.has(BlobType::Data, &id)); assert!(index.get_id(BlobType::Data, &id).is_none()); assert!(!index.has(BlobType::Tree, &id)); assert!(index.get_id(BlobType::Tree, &id).is_none()); - let id = parse("620b2cef43d4c7aab3d7c911a3c0e872d2e0e70f170201002b8af8fb98c59da5"); + let id = + "620b2cef43d4c7aab3d7c911a3c0e872d2e0e70f170201002b8af8fb98c59da5".parse::()?; assert!(!index.has(BlobType::Data, &id)); assert!(index.get_id(BlobType::Data, &id).is_none()); assert!(!index.has(BlobType::Tree, &id)); assert!(index.get_id(BlobType::Tree, &id).is_none()); + + Ok(()) } #[test] - fn full_trees() { + fn test_full_trees_passes() -> RusticResult<()> { let index = index(IndexType::DataIds); - let id = parse("fac5e908151e565267570108127b96e6bae22bcdda1d3d867f63ed1555fc8aef"); + let id = + "fac5e908151e565267570108127b96e6bae22bcdda1d3d867f63ed1555fc8aef".parse::()?; assert!(index.has(BlobType::Data, &id)); assert!(index.get_id(BlobType::Data, &id).is_none()); assert!(!index.has(BlobType::Tree, &id)); assert!(index.get_id(BlobType::Tree, &id).is_none()); - let id = parse("620b2cef43d4c7aab3d7c911a3c0e872d2e0e70f170201002b8af8fb98c59da5"); + let id = + "620b2cef43d4c7aab3d7c911a3c0e872d2e0e70f170201002b8af8fb98c59da5".parse::()?; assert!(index.has(BlobType::Data, &id)); assert!(index.get_id(BlobType::Data, &id).is_none()); assert!(!index.has(BlobType::Tree, &id)); assert!(index.get_id(BlobType::Tree, &id).is_none()); + + Ok(()) } #[test] - fn full() { + fn test_full_index_passes() -> RusticResult<()> { let index = index(IndexType::Full); - let id = parse("fac5e908151e565267570108127b96e6bae22bcdda1d3d867f63ed1555fc8aef"); + let id = + "fac5e908151e565267570108127b96e6bae22bcdda1d3d867f63ed1555fc8aef".parse::()?; assert!(index.has(BlobType::Data, &id)); assert_eq!( index.get_id(BlobType::Data, &id), Some(IndexEntry { blob_type: BlobType::Data, - pack: parse("217f145b63fbc10267f5a686186689ea3389bed0d6a54b50ffc84d71f99eb7fa"), + pack: "217f145b63fbc10267f5a686186689ea3389bed0d6a54b50ffc84d71f99eb7fa" + .parse::()?, offset: 5185, length: 2095, uncompressed_length: Some(NonZeroU32::new(6411).unwrap()), @@ -429,13 +432,15 @@ mod tests { assert!(!index.has(BlobType::Tree, &id)); assert!(index.get_id(BlobType::Tree, &id).is_none()); - let id = parse("620b2cef43d4c7aab3d7c911a3c0e872d2e0e70f170201002b8af8fb98c59da5"); + let id = + "620b2cef43d4c7aab3d7c911a3c0e872d2e0e70f170201002b8af8fb98c59da5".parse::()?; assert!(index.has(BlobType::Data, &id)); assert_eq!( index.get_id(BlobType::Data, &id), Some(IndexEntry { blob_type: BlobType::Data, - pack: parse("3b25ec6d16401c31099c259311562160b1b5efbcf70bd69d0463104d3b8148fc"), + pack: "3b25ec6d16401c31099c259311562160b1b5efbcf70bd69d0463104d3b8148fc" + .parse::()?, offset: 6324, length: 1413, uncompressed_length: Some(NonZeroU32::new(3752).unwrap()), @@ -443,5 +448,7 @@ mod tests { ); assert!(!index.has(BlobType::Tree, &id)); assert!(index.get_id(BlobType::Tree, &id).is_none()); + + Ok(()) } } diff --git a/crates/core/src/repository.rs b/crates/core/src/repository.rs index d9261b44..ff8d1db0 100644 --- a/crates/core/src/repository.rs +++ b/crates/core/src/repository.rs @@ -1849,3 +1849,28 @@ impl Repository { opts.repair(self, snapshots, dry_run) } } + +/// Initialize a repository for testing +/// +/// # Errors +/// +/// If the repository could not be initialized +/// +/// # Returns +/// +/// The initialized repository +/// +/// # Notes +/// +/// The repository is initialized with an in-memory backend and a password of "test". +pub fn init_test_repository() -> RusticResult> { + let be = InMemoryBackend::new(); + let be = RepositoryBackends::new(Arc::new(be), None); + let options = RepositoryOptions::default().password("test").no_cache(true); + let repo = Repository::new(&options, &be)?; + let key_opts = KeyOptions::default(); + let config_opts = &ConfigOptions::default(); + let repo = repo.init(&key_opts, config_opts)?; + + Ok(repo) +} diff --git a/crates/core/tests/integration.rs b/crates/core/tests/integration.rs index f7707d64..319bd7ab 100644 --- a/crates/core/tests/integration.rs +++ b/crates/core/tests/integration.rs @@ -23,27 +23,25 @@ //! The tests that use the fixtures are defined as functions with the `#[rstest]` attribute. //! The fixtures are passed as arguments to the test functions. -use anyhow::Result; +use anyhow::{anyhow, Result}; use flate2::read::GzDecoder; use insta::internals::{Content, ContentPath}; use insta::{assert_ron_snapshot, Settings}; use pretty_assertions::assert_eq; use rstest::fixture; use rstest::rstest; + use rustic_core::{ - repofile::SnapshotFile, BackupOptions, ConfigOptions, KeyOptions, NoProgressBars, OpenStatus, - PathList, Repository, RepositoryBackends, RepositoryOptions, + init_test_repository, repofile::SnapshotFile, BackupOptions, NoProgressBars, OpenStatus, + PathList, Repository, }; use serde_derive::Serialize; -use rustic_testing::backend::in_memory_backend::InMemoryBackend; - use std::{ env, fs::File, path::{Path, PathBuf}, str::FromStr, - sync::Arc, }; // uncomment for logging output // use simplelog::{Config, SimpleLogger}; @@ -67,14 +65,7 @@ impl TestSource { #[fixture] fn set_up_repo() -> Result { - let be = InMemoryBackend::new(); - let be = RepositoryBackends::new(Arc::new(be), None); - let options = RepositoryOptions::default().password("test"); - let repo = Repository::new(&options, &be)?; - let key_opts = KeyOptions::default(); - let config_opts = &ConfigOptions::default(); - let repo = repo.init(&key_opts, config_opts)?; - Ok(repo) + Ok(init_test_repository()?) } // helper func to redact options, but still keep information about some/none @@ -204,7 +195,8 @@ fn test_backup_with_tar_gz_passes( // re-read index let repo = repo.to_indexed_ids()?; let tree = repo.node_from_path(first_snapshot.tree, Path::new("test/0/tests"))?; - let tree: rustic_core::repofile::Tree = repo.get_tree(&tree.subtree.expect("Sub tree"))?; + let tree: rustic_core::repofile::Tree = + repo.get_tree(&tree.subtree.ok_or_else(|| anyhow!("Sub tree"))?)?; insta_tree_redaction.bind(|| { #[cfg(windows)] @@ -297,7 +289,7 @@ fn test_backup_dry_run_with_tar_gz_passes( // re-read index let repo = repo.to_indexed_ids()?; let tree = repo.node_from_path(first_snapshot.tree, Path::new("test/0/tests"))?; - let tree = repo.get_tree(&tree.subtree.expect("Sub tree"))?; + let tree = repo.get_tree(&tree.subtree.ok_or_else(|| anyhow!("Sub tree"))?)?; insta_tree_redaction.bind(|| { #[cfg(windows)] diff --git a/crates/core/tests/snapshots/integration__backup-tar-tree-nix.snap b/crates/core/tests/snapshots/integration__backup-tar-tree-nix.snap index 1d815497..8051a726 100644 --- a/crates/core/tests/snapshots/integration__backup-tar-tree-nix.snap +++ b/crates/core/tests/snapshots/integration__backup-tar-tree-nix.snap @@ -61,7 +61,7 @@ Tree( { "name": "testfile-symlink", "type": "symlink", - "linktarget": "testfile", + "link_target": "testfile", "mode": "[some]", "mtime": "[some]", "atime": "[some]", diff --git a/crates/core/tests/snapshots/integration__backup-tar-tree-windows.snap b/crates/core/tests/snapshots/integration__backup-tar-tree-windows.snap index 21c9e746..bb198d8f 100644 --- a/crates/core/tests/snapshots/integration__backup-tar-tree-windows.snap +++ b/crates/core/tests/snapshots/integration__backup-tar-tree-windows.snap @@ -37,7 +37,7 @@ Tree( { "name": "testfile-symlink", "type": "symlink", - "linktarget": "testfile", + "link_target": "testfile", "mtime": "[some]", "atime": "[some]", "ctime": "[some]", diff --git a/crates/core/tests/snapshots/integration__dryrun-tar-tree-nix.snap b/crates/core/tests/snapshots/integration__dryrun-tar-tree-nix.snap index 1d815497..8051a726 100644 --- a/crates/core/tests/snapshots/integration__dryrun-tar-tree-nix.snap +++ b/crates/core/tests/snapshots/integration__dryrun-tar-tree-nix.snap @@ -61,7 +61,7 @@ Tree( { "name": "testfile-symlink", "type": "symlink", - "linktarget": "testfile", + "link_target": "testfile", "mode": "[some]", "mtime": "[some]", "atime": "[some]", diff --git a/crates/core/tests/snapshots/integration__dryrun-tar-tree-windows.snap b/crates/core/tests/snapshots/integration__dryrun-tar-tree-windows.snap index 21c9e746..bb198d8f 100644 --- a/crates/core/tests/snapshots/integration__dryrun-tar-tree-windows.snap +++ b/crates/core/tests/snapshots/integration__dryrun-tar-tree-windows.snap @@ -37,7 +37,7 @@ Tree( { "name": "testfile-symlink", "type": "symlink", - "linktarget": "testfile", + "link_target": "testfile", "mtime": "[some]", "atime": "[some]", "ctime": "[some]", diff --git a/crates/testing/Cargo.toml b/crates/testing/Cargo.toml index e39f9724..4b84f98f 100644 --- a/crates/testing/Cargo.toml +++ b/crates/testing/Cargo.toml @@ -10,6 +10,7 @@ anyhow = { workspace = true } bytes = { workspace = true } enum-map = { workspace = true } once_cell = "1.19.0" +parking_lot = { workspace = true } rustic_core = { workspace = true } tempfile = { workspace = true } diff --git a/crates/testing/src/backend.rs b/crates/testing/src/backend.rs deleted file mode 100644 index 1c7ca4d1..00000000 --- a/crates/testing/src/backend.rs +++ /dev/null @@ -1,81 +0,0 @@ -/// In-memory backend to be used for testing -pub mod in_memory_backend { - use std::{collections::BTreeMap, sync::RwLock}; - - use anyhow::{bail, Result}; - use bytes::Bytes; - use enum_map::EnumMap; - - use rustic_core::{FileType, Id, ReadBackend, WriteBackend}; - - #[derive(Debug)] - /// In-Memory backend to be used for testing - pub struct InMemoryBackend(RwLock>>); - - impl InMemoryBackend { - /// Create a new (empty) `InMemoryBackend` - #[must_use] - pub fn new() -> Self { - Self(RwLock::new(EnumMap::from_fn(|_| BTreeMap::new()))) - } - } - - impl Default for InMemoryBackend { - fn default() -> Self { - Self::new() - } - } - - impl ReadBackend for InMemoryBackend { - fn location(&self) -> String { - "test".to_string() - } - - fn list_with_size(&self, tpe: FileType) -> Result> { - Ok(self.0.read().unwrap()[tpe] - .iter() - .map(|(id, byte)| { - ( - *id, - u32::try_from(byte.len()).expect("byte length is too large"), - ) - }) - .collect()) - } - - fn read_full(&self, tpe: FileType, id: &Id) -> Result { - Ok(self.0.read().unwrap()[tpe][id].clone()) - } - - fn read_partial( - &self, - tpe: FileType, - id: &Id, - _cacheable: bool, - offset: u32, - length: u32, - ) -> Result { - Ok(self.0.read().unwrap()[tpe][id].slice(offset as usize..(offset + length) as usize)) - } - } - - impl WriteBackend for InMemoryBackend { - fn create(&self) -> Result<()> { - Ok(()) - } - - fn write_bytes(&self, tpe: FileType, id: &Id, _cacheable: bool, buf: Bytes) -> Result<()> { - if self.0.write().unwrap()[tpe].insert(*id, buf).is_some() { - bail!("id {id} already exists"); - } - Ok(()) - } - - fn remove(&self, tpe: FileType, id: &Id, _cacheable: bool) -> Result<()> { - if self.0.write().unwrap()[tpe].remove(id).is_none() { - bail!("id {id} doesn't exists"); - } - Ok(()) - } - } -} diff --git a/crates/testing/src/lib.rs b/crates/testing/src/lib.rs index 741f75dc..7a04e793 100644 --- a/crates/testing/src/lib.rs +++ b/crates/testing/src/lib.rs @@ -1,8 +1,5 @@ //! Testing utilities for the `rustic` ecosystem. -/// Backends to be used solely for testing. -pub mod backend; - use aho_corasick::{AhoCorasick, PatternID}; use std::{error::Error, ffi::OsStr}; use tempfile::NamedTempFile; From 0ef7c221fa9df85e6b92989355c2ba2efe3beb88 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 11:41:45 +0100 Subject: [PATCH 02/58] rename trait for checking errors in rest backend Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/backend/src/rest.rs | 76 ++++++++++++++++++++++++-------------- 1 file changed, 49 insertions(+), 27 deletions(-) diff --git a/crates/backend/src/rest.rs b/crates/backend/src/rest.rs index 78286ec0..5ce102a3 100644 --- a/crates/backend/src/rest.rs +++ b/crates/backend/src/rest.rs @@ -21,33 +21,62 @@ mod consts { pub(super) const DEFAULT_RETRY: usize = 5; } -// trait CheckError to add user-defined method check_error on Response -pub(crate) trait CheckError { - /// Check reqwest Response for error and treat errors as permanent or transient - fn check_error(self) -> Result>; +/// `ValidateResponse` to add user-defined method `validate` on a response +/// +/// This trait is used to add a method `validate` on a response to check for errors +/// and treat them as permanent or transient based on the status code of the response. +/// +/// It returns a result with the response if it is not an error, otherwise it returns +/// the associated error. +pub trait ValidateResponse { + /// The error type that is returned if the response is an error + type Error; + + /// Check a response for an error and treat it as permanent or transient + /// + /// # Errors + /// + /// If the response is an error, it will return an error of type [`Self::Error`] + /// + /// # Returns + /// + /// The response if it is not an error or an error of type [`Self::Error`] if it is an error + fn validate(self) -> Result + where + Self: Sized; } -impl CheckError for Response { +impl ValidateResponse for Response { + type Error = backoff::Error; + /// Check reqwest Response for error and treat errors as permanent or transient + /// based on the status code of the response /// /// # Errors /// - /// If the response is an error, it will return an error of type Error + /// If the response is an error, it will return an [`reqwest::Error`] /// /// # Returns /// - /// The response if it is not an error - fn check_error(self) -> Result> { + /// The [`Response`] if it is not an error + fn validate(self) -> Result { match self.error_for_status() { Ok(t) => Ok(t), // Note: status() always give Some(_) as it is called from a Response - Err(err) if err.status().unwrap().is_client_error() => { - Err(backoff::Error::Permanent(err)) + Err(err) => { + let Some(status) = err.status() else { + return Err(backoff::Error::Permanent(err)); + }; + + if status.is_client_error() { + Err(backoff::Error::Permanent(err)) + } else { + Err(backoff::Error::Transient { + err, + retry_after: None, + }) + } } - Err(err) => Err(backoff::Error::Transient { - err, - retry_after: None, - }), } } } @@ -284,7 +313,7 @@ impl ReadBackend for RestBackend { .get(url.clone()) .header("Accept", "application/vnd.x.restic.rest.v2") .send()? - .check_error()? + .validate()? .json::>>()? // use Option to be handle null json value .unwrap_or_default(); Ok(list @@ -318,14 +347,7 @@ impl ReadBackend for RestBackend { let url = self.url(tpe, id)?; Ok(backoff::retry_notify( self.backoff.clone(), - || { - Ok(self - .client - .get(url.clone()) - .send()? - .check_error()? - .bytes()?) - }, + || Ok(self.client.get(url.clone()).send()?.validate()?.bytes()?), notify, ) .map_err(RestErrorKind::BackoffError)?) @@ -366,7 +388,7 @@ impl ReadBackend for RestBackend { .get(url.clone()) .header("Range", header_value.clone()) .send()? - .check_error()? + .validate()? .bytes()?) }, notify, @@ -391,7 +413,7 @@ impl WriteBackend for RestBackend { Ok(backoff::retry_notify( self.backoff.clone(), || { - _ = self.client.post(url.clone()).send()?.check_error()?; + _ = self.client.post(url.clone()).send()?.validate()?; Ok(()) }, notify, @@ -420,7 +442,7 @@ impl WriteBackend for RestBackend { self.backoff.clone(), || { // Note: try_clone() always gives Some(_) as the body is Bytes which is clonable - _ = req_builder.try_clone().unwrap().send()?.check_error()?; + .validate()?; Ok(()) }, notify, @@ -447,7 +469,7 @@ impl WriteBackend for RestBackend { Ok(backoff::retry_notify( self.backoff.clone(), || { - _ = self.client.delete(url.clone()).send()?.check_error()?; + _ = self.client.delete(url.clone()).send()?.validate()?; Ok(()) }, notify, From 76f2bed4025006f7d139afefe77760c490a63858 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 11:43:46 +0100 Subject: [PATCH 03/58] implement transpose on ParentResult and some error handling fixes Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/archiver/parent.rs | 48 ++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/crates/core/src/archiver/parent.rs b/crates/core/src/archiver/parent.rs index ddd78c00..1e63291e 100644 --- a/crates/core/src/archiver/parent.rs +++ b/crates/core/src/archiver/parent.rs @@ -43,7 +43,7 @@ pub struct Parent { /// # Type Parameters /// /// * `T` - The type of the matched parent. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq, Eq)] pub(crate) enum ParentResult { /// The parent was found and matches. Matched(T), @@ -54,11 +54,13 @@ pub(crate) enum ParentResult { } impl ParentResult { - /// Maps a `ParentResult` to a `ParentResult` by applying a function to a contained value. + /// Maps a `ParentResult` to a `ParentResult` by applying a function to a contained value. /// /// # Type Parameters /// - /// * `R` - The type of the returned `ParentResult`. + /// * `T` - The type of the contained value. + /// * `U` - The type of the returned `ParentResult`. + /// * `F` - The function to apply. /// /// # Arguments /// @@ -66,8 +68,12 @@ impl ParentResult { /// /// # Returns /// - /// A `ParentResult` with the result of the function for each `ParentResult`. - fn map(self, f: impl FnOnce(T) -> R) -> ParentResult { + /// A `ParentResult` with the result of the function for each `ParentResult`. + #[inline] + fn map(self, f: F) -> ParentResult + where + F: FnOnce(T) -> U, + { match self { Self::Matched(t) => ParentResult::Matched(f(t)), Self::NotFound => ParentResult::NotFound, @@ -76,6 +82,33 @@ impl ParentResult { } } +impl ParentResult> { + /// Transposes a [`ParentResult`] of a [`Result`] into a [`Result`] of a [`ParentResult`]. + /// + /// # Type Parameters + /// + /// * `T` - The type of the inner `Result`. + /// * `E` - The error type of the inner `Result`. + /// + /// # Errors + /// + /// If the `ParentResult` is `Matched` and the inner `Result` is `Err`, the error is returned. + /// + /// # Returns + /// + /// A `Result` of a `ParentResult`. + #[inline] + pub fn transpose(self) -> Result, E> { + match self { + Self::Matched(Ok(x)) => Ok(ParentResult::Matched(x)), + + Self::Matched(Err(e)) => Err(e), + Self::NotFound => Ok(ParentResult::NotFound), + Self::NotMatched => Ok(ParentResult::NotMatched), + } + } +} + impl Parent { /// Creates a new `Parent`. /// @@ -266,8 +299,11 @@ impl Parent { TreeType::NewTree((path, node, tree)) => { let parent_result = self .is_parent(&node, &tree) - .map(|node| node.subtree.unwrap()); + .map(|node| node.subtree.ok_or(ArchiverErrorKind::ParentNodeIsNoTree)) + .transpose()?; + self.set_dir(be, index, &tree); + TreeType::NewTree((path, node, parent_result)) } TreeType::EndTree => { From 39089b126d322f18caeeecabc5b4a8d1a58fafb0 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 11:50:59 +0100 Subject: [PATCH 04/58] fix doc test and export for overwrite zero duration Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/commands/forget.rs | 4 +++- crates/core/src/lib.rs | 3 +++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/core/src/commands/forget.rs b/crates/core/src/commands/forget.rs index 8405b76e..fe1944e8 100644 --- a/crates/core/src/commands/forget.rs +++ b/crates/core/src/commands/forget.rs @@ -273,11 +273,13 @@ pub struct KeepOptions { /// # Example /// /// ``` -/// use rustic_core::commands::forget::overwrite_zero_duration; +/// use rustic_core::overwrite_zero_duration; /// use humantime::Duration; /// /// let mut left = "0s".parse::().unwrap().into(); /// let right = "60s".parse::().unwrap().into(); +/// +/// /// overwrite_zero_duration(&mut left, right); /// assert_eq!(left, "60s".parse::().unwrap().into()); /// ``` diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index f277d948..71ddc293 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -150,3 +150,6 @@ pub use crate::{ }, repository::{IndexedFull, OpenStatus, Repository, RepositoryOptions}, }; + +#[cfg(feature = "merge")] +pub use crate::commands::forget::overwrite_zero_duration; From 5ba3c021751393b9cc93f787d298e3d849ecee0b Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 11:52:33 +0100 Subject: [PATCH 05/58] rework Id/HexId Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/id.rs | 96 +++++++++++++++++++++++++++++++++++++------ 1 file changed, 83 insertions(+), 13 deletions(-) diff --git a/crates/core/src/id.rs b/crates/core/src/id.rs index ccfacdfa..c10de6d5 100644 --- a/crates/core/src/id.rs +++ b/crates/core/src/id.rs @@ -1,9 +1,15 @@ //! The `Id` type and related functions -use std::{fmt, io::Read, ops::Deref, path::Path}; +use std::{ + fmt::{self, Display}, + io::Read, + ops::Deref, + path::Path, + str::FromStr, +}; use binrw::{BinRead, BinWrite}; -use derive_more::{Constructor, Display}; +use derive_more::Constructor; use rand::{thread_rng, RngCore}; use serde_derive::{Deserialize, Serialize}; @@ -33,9 +39,7 @@ pub(super) mod constants { Constructor, BinWrite, BinRead, - Display, )] -#[display(fmt = "{}", "&self.to_hex()[0..8]")] pub struct Id( /// The actual hash #[serde(serialize_with = "hex::serde::serialize")] @@ -43,6 +47,23 @@ pub struct Id( [u8; constants::LEN], ); +impl FromStr for Id { + type Err = IdErrorKind; + + fn from_str(s: &str) -> Result { + Self::from_hex(s).map_err(|_| IdErrorKind::ParsingIdFromStringFailed(s.to_string())) + } +} + +impl Display for Id { + /// Format the `Id` as a hexadecimal string + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let id = &self.to_hex()[0..8]; + + write!(f, "{id}") + } +} + impl Id { /// Parse an `Id` from a hexadecimal string /// @@ -61,11 +82,20 @@ impl Id { /// /// let id = Id::from_hex("0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef").unwrap(); /// - /// assert_eq!(id.to_hex().as_str(), "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"); + /// assert_eq!(id.to_hex().as_str(), + /// "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"); /// ``` /// /// [`IdErrorKind::HexError`]: crate::error::IdErrorKind::HexError pub fn from_hex(s: &str) -> RusticResult { + if s.is_empty() { + return Err(IdErrorKind::EmptyHexString.into()); + } + + if !s.is_ascii() { + return Err(IdErrorKind::NonAsciiHexString.into()); + } + let mut id = Self::default(); hex::decode_to_slice(s, &mut id.0).map_err(IdErrorKind::HexError)?; @@ -95,13 +125,24 @@ impl Id { /// /// # Panics /// - /// Panics if the `hex` crate fails to encode the hash - // TODO! - remove the panic + /// - An invalid character was found. Valid ones are: `0...9`, `a...f` or `A...F`. + /// + /// - A hex string's length needs to be even, as two digits correspond to one byte. + /// + /// - If the hex string is decoded into a fixed sized container, such as an + /// array, the hex string's length * 2 has to match the container's length. + /// + /// # Returns + /// + /// The [`HexId`] representation of the [`Id`] if it is valid #[must_use] + #[allow(clippy::expect_used)] pub fn to_hex(self) -> HexId { let mut hex_id = HexId::EMPTY; - // HexId's len is LEN * 2 - hex::encode_to_slice(self.0, &mut hex_id.0).unwrap(); + + hex::encode_to_slice(self.0, &mut hex_id.0) + .expect("HexId's len is LEN * 2, should never panic."); + hex_id } @@ -141,14 +182,28 @@ impl Id { impl fmt::Debug for Id { /// Format the `Id` as a hexadecimal string fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", &*self.to_hex()) + let id = &self.to_hex()[0..32]; + + write!(f, "{id}") } } /// An `Id` in hexadecimal format -#[derive(Copy, Clone, Debug)] +#[derive(Copy, Clone, Debug, PartialEq, Eq)] pub struct HexId([u8; constants::HEX_LEN]); +impl From for HexId { + fn from(id: Id) -> Self { + id.to_hex() + } +} + +impl PartialEq for HexId { + fn eq(&self, other: &str) -> bool { + self.as_str() == other + } +} + impl HexId { /// An empty [`HexId`] const EMPTY: Self = Self([b'0'; constants::HEX_LEN]); @@ -157,11 +212,26 @@ impl HexId { /// /// # Panics /// - /// If the [`HexId`] is not a valid UTF-8 string + /// * `None` case: the end of the input was reached unexpectedly. + /// `self.valid_up_to()` is 1 to 3 bytes from the end of the input. + /// If a byte stream (such as a file or a network socket) is being decoded incrementally, + /// this could be a valid `char` whose UTF-8 byte sequence is spanning multiple chunks. + /// + /// * `Some(len)` case: an unexpected byte was encountered. + /// The length provided is that of the invalid byte sequence + /// that starts at the index given by `valid_up_to()`. + /// Decoding should resume after that sequence + /// (after inserting a [`U+FFFD REPLACEMENT CHARACTER`][U+FFFD]) in case of + /// lossy decoding. + /// + /// # Returns + /// + /// The string representation of the [`HexId`] if it is valid #[must_use] + #[allow(clippy::expect_used)] pub fn as_str(&self) -> &str { // This is only ever filled with hex chars, which are ascii - std::str::from_utf8(&self.0).unwrap() + std::str::from_utf8(&self.0).expect("HexId is not valid utf8, which should never happen") } } From ad9203b2d21a3b829aeea75d87612e926e18bb0f Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 12:16:37 +0100 Subject: [PATCH 06/58] use parking lot Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- Cargo.toml | 2 +- crates/core/Cargo.toml | 1 + crates/core/src/archiver.rs | 2 +- crates/core/src/blob/packer.rs | 46 ++++++++++++------------ crates/core/src/commands/merge.rs | 2 +- crates/core/src/commands/prune.rs | 12 +++---- crates/core/src/commands/repair/index.rs | 4 +-- crates/core/src/index.rs | 6 ++-- crates/core/src/index/indexer.rs | 4 ++- 9 files changed, 41 insertions(+), 38 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 6aa1fa6b..0893b2fb 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,7 +15,7 @@ rust-version = "1.72.1" aho-corasick = "1.1.2" anyhow = "1.0.81" bytes = "1.5.0" -enum-map = "2.7.3" +parking_lot = { version = "0.12.1", features = ["deadlock_detection", "arc_lock"] } rustic_backend = { path = "crates/backend" } rustic_core = { path = "crates/core" } rustic_testing = { path = "crates/testing" } diff --git a/crates/core/Cargo.toml b/crates/core/Cargo.toml index 4caf6cb8..f2e3154c 100644 --- a/crates/core/Cargo.toml +++ b/crates/core/Cargo.toml @@ -61,6 +61,7 @@ log = "0.4.21" # parallelize crossbeam-channel = "0.5.12" pariter = "0.5.1" +parking_lot = { workspace = true } rayon = "1.9.0" # crypto diff --git a/crates/core/src/archiver.rs b/crates/core/src/archiver.rs index ac858763..1c2696f7 100644 --- a/crates/core/src/archiver.rs +++ b/crates/core/src/archiver.rs @@ -211,7 +211,7 @@ impl<'a, BE: DecryptFullBackend, I: ReadGlobalIndex> Archiver<'a, BE, I> { stats.apply(&mut summary, BlobType::Data); self.snap.tree = id; - self.indexer.write().unwrap().finalize()?; + self.indexer.write().finalize()?; summary.finalize(self.snap.time)?; self.snap.summary = Some(summary); diff --git a/crates/core/src/blob/packer.rs b/crates/core/src/blob/packer.rs index 48cacdc9..a4996c1a 100644 --- a/crates/core/src/blob/packer.rs +++ b/crates/core/src/blob/packer.rs @@ -3,10 +3,12 @@ use log::warn; use std::{ num::NonZeroU32, - sync::{Arc, RwLock}, + sync::Arc, time::{Duration, SystemTime}, }; +use parking_lot::RwLock; + use bytes::{Bytes, BytesMut}; use chrono::Local; use crossbeam_channel::{bounded, Receiver, Sender}; @@ -30,6 +32,8 @@ use crate::{ }, }; +pub(crate) type SharedPacker = Arc>>; + pub(super) mod constants { use std::time::Duration; @@ -163,7 +167,7 @@ pub struct Packer { /// The raw packer wrapped in an Arc and RwLock. // This is a hack: raw_packer and indexer are only used in the add_raw() method. // TODO: Refactor as actor, like the other add() methods - raw_packer: Arc>>, + raw_packer: SharedPacker, /// The shared indexer containing the backend. indexer: SharedIndexer, /// The sender to send blobs to the raw packer. @@ -225,8 +229,8 @@ impl Packer { .into_iter() .readahead_scoped(scope) // early check if id is already contained - .filter(|(_, id, _)| !indexer.read().unwrap().has(id)) - .filter(|(_, id, _)| !raw_packer.read().unwrap().has(id)) + .filter(|(_, id, _)| !indexer.read().has(id)) + .filter(|(_, id, _)| !raw_packer.read().has(id)) .readahead_scoped(scope) .parallel_map_scoped( scope, @@ -245,22 +249,15 @@ impl Packer { // check again if id is already contained // TODO: We may still save duplicate blobs - the indexer is only updated when the packfile write has completed .filter(|res| { - res.as_ref().map_or_else( - |_| true, - |(_, id, _, _, _)| !indexer.read().unwrap().has(id), - ) + .map_or_else(|_| true, |(_, id, _, _, _)| !indexer.read().has(id)) }) .try_for_each(|item: RusticResult<_>| { let (data, id, data_len, ul, size_limit) = item?; raw_packer .write() - .unwrap() .add_raw(&data, &id, data_len, ul, size_limit) }) - .and_then(|()| raw_packer.write().unwrap().finalize()); - _ = finish_tx.send(status); - }) - .unwrap(); + .and_then(|()| raw_packer.write().finalize()); }); Ok(packer) @@ -326,16 +323,12 @@ impl Packer { size_limit: Option, ) -> RusticResult<()> { // only add if this blob is not present - if self.indexer.read().unwrap().has(id) { + if self.indexer.read().has(id) { Ok(()) } else { - self.raw_packer.write().unwrap().add_raw( - data, - id, - data_len, - uncompressed_length, - size_limit, - ) + self.raw_packer + .write() + .add_raw(data, id, data_len, uncompressed_length, size_limit) } } @@ -347,8 +340,15 @@ impl Packer { pub fn finalize(self) -> RusticResult { // cancel channel drop(self.sender); + // wait for items in channel to be processed - self.finish.recv().unwrap() + self.finish.recv().map_or_else( + |_| { + error!("sender has been dropped unexpectedly."); + Err(MultiprocessingErrorKind::SenderDropped.into()) + }, + |stats| stats, + ) } } @@ -648,7 +648,7 @@ impl FileWriterHandle { } fn index(&self, index: IndexPack) -> RusticResult<()> { - self.indexer.write().unwrap().add(index)?; + self.indexer.write().add(index)?; Ok(()) } } diff --git a/crates/core/src/commands/merge.rs b/crates/core/src/commands/merge.rs index 63cf7c32..03489d63 100644 --- a/crates/core/src/commands/merge.rs +++ b/crates/core/src/commands/merge.rs @@ -117,7 +117,7 @@ pub(crate) fn merge_trees( let p = repo.pb.progress_spinner("merging snapshots..."); let tree_merged = tree::merge_trees(be, index, trees, cmp, &save, summary)?; let stats = packer.finalize()?; - indexer.write().unwrap().finalize()?; + indexer.write().finalize()?; p.finish(); stats.apply(summary, BlobType::Tree); diff --git a/crates/core/src/commands/prune.rs b/crates/core/src/commands/prune.rs index bb8664bc..39a1e9e4 100644 --- a/crates/core/src/commands/prune.rs +++ b/crates/core/src/commands/prune.rs @@ -1178,7 +1178,7 @@ impl PrunePlan { time: Some(Local::now()), blobs: Vec::new(), }; - indexer.write().unwrap().add_remove(pack)?; + indexer.write().add_remove(pack)?; p.inc(1); } p.finish(); @@ -1263,7 +1263,7 @@ impl PrunePlan { } else { // mark pack for removal let pack = pack.into_index_pack_with_time(self.time); - indexer.write().unwrap().add_remove(pack)?; + indexer.write().add_remove(pack)?; } } PackToDo::MarkDelete => { @@ -1272,7 +1272,7 @@ impl PrunePlan { } else { // mark pack for removal let pack = pack.into_index_pack_with_time(self.time); - indexer.write().unwrap().add_remove(pack)?; + indexer.write().add_remove(pack)?; } } PackToDo::KeepMarked | PackToDo::KeepMarkedAndCorrect => { @@ -1283,13 +1283,13 @@ impl PrunePlan { // Note the timestap shouldn't be None here, however if it is not not set, use the current time to heal the entry! let time = pack.time.unwrap_or(self.time); let pack = pack.into_index_pack_with_time(time); - indexer.write().unwrap().add_remove(pack)?; + indexer.write().add_remove(pack)?; } } PackToDo::Recover => { // recover pack: add to new index in section packs let pack = pack.into_index_pack_with_time(self.time); - indexer.write().unwrap().add(pack)?; + indexer.write().add(pack)?; } PackToDo::Delete => delete_pack(pack), } @@ -1297,7 +1297,7 @@ impl PrunePlan { })?; _ = tree_repacker.finalize()?; _ = data_repacker.finalize()?; - indexer.write().unwrap().finalize()?; + indexer.write().finalize()?; p.finish(); // remove old index files first as they may reference pack files which are removed soon. diff --git a/crates/core/src/commands/repair/index.rs b/crates/core/src/commands/repair/index.rs index a4e39b96..8860eea2 100644 --- a/crates/core/src/commands/repair/index.rs +++ b/crates/core/src/commands/repair/index.rs @@ -99,11 +99,11 @@ impl RepairIndexOptions { if !dry_run { // write pack file to index - without the delete mark - indexer.write().unwrap().add_with(pack, false)?; + indexer.write().add_with(pack, false)?; } p.inc(1); } - indexer.write().unwrap().finalize()?; + indexer.write().finalize()?; p.finish(); Ok(()) diff --git a/crates/core/src/index.rs b/crates/core/src/index.rs index c73f95ac..3b77ca09 100644 --- a/crates/core/src/index.rs +++ b/crates/core/src/index.rs @@ -306,14 +306,14 @@ impl GlobalIndex { } /// Convert the `Arc` to an Index - pub fn into_index(self) -> Index { + pub fn into_index(self) -> RusticResult { match Arc::try_unwrap(self.index) { - Ok(index) => index, + Ok(index) => Ok(index), Err(arc) => { // Seems index is still in use; this could be due to some threads using it which didn't yet completely shut down. // sleep a bit to let threads using the index shut down, after this index should be available to unwrap sleep(Duration::from_millis(100)); - Arc::try_unwrap(arc).expect("index still in use") + Arc::try_unwrap(arc).map_err(|_| IndexErrorKind::IndexStillInUse.into()) } } } diff --git a/crates/core/src/index/indexer.rs b/crates/core/src/index/indexer.rs index a63c45a7..0f2aaf47 100644 --- a/crates/core/src/index/indexer.rs +++ b/crates/core/src/index/indexer.rs @@ -1,9 +1,11 @@ use std::{ collections::BTreeSet, - sync::{Arc, RwLock}, + sync::Arc, time::{Duration, SystemTime}, }; +use parking_lot::RwLock; + use log::warn; use crate::{ From bce58dea6d5bea835a30c8c8002e5a12e069cee1 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 12:17:11 +0100 Subject: [PATCH 07/58] more errors for error handling Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/error.rs | 274 +++++++++++++++++++++++++++++++++------ 1 file changed, 237 insertions(+), 37 deletions(-) diff --git a/crates/core/src/error.rs b/crates/core/src/error.rs index 3f09d0bb..443c2dde 100644 --- a/crates/core/src/error.rs +++ b/crates/core/src/error.rs @@ -12,6 +12,7 @@ use std::{ str::Utf8Error, }; +use crossbeam_channel::SendError; #[cfg(not(windows))] use nix::errno::Errno; @@ -60,6 +61,12 @@ impl RusticError { } } +impl From for std::fmt::Error { + fn from(_: RusticError) -> Self { + Self + } +} + /// [`RusticErrorKind`] describes the errors that can happen while executing a high-level command. /// /// This is a non-exhaustive enum, so additional variants may be added in future. It is @@ -88,11 +95,15 @@ pub enum RusticErrorKind { #[error(transparent)] Repository(#[from] RepositoryErrorKind), - /// [`IndexErrorKind`] describes the errors that can be returned by processing Indizes + /// [`IndexErrorKind`] describes the errors that can be returned by processing Indices #[error(transparent)] Index(#[from] IndexErrorKind), - /// describes the errors that can be returned by the various Backends + /// [`ChannelErrorKind`] describes the errors that can be returned by dealing with channels + #[error(transparent)] + Channel(#[from] MultiprocessingErrorKind), + + /// This error is returned when a `rustic_backend` fails to perform an action #[error(transparent)] Backend(#[from] anyhow::Error), @@ -128,6 +139,10 @@ pub enum RusticErrorKind { #[error(transparent)] Tree(#[from] TreeErrorKind), + /// [`walkdir::Error`] describes the errors that can be returned by walking directories + #[error(transparent)] + WalkDir(#[from] walkdir::Error), + /// [`CacheBackendErrorKind`] describes the errors that can be returned by a Caching action in Backends #[error(transparent)] CacheBackend(#[from] CacheBackendErrorKind), @@ -163,6 +178,10 @@ pub enum RusticErrorKind { /// [`std::io::Error`] #[error(transparent)] StdIo(#[from] std::io::Error), + + /// [`CheckErrorKind`] + #[error(transparent)] + Check(#[from] CheckErrorKind), } /// [`CommandErrorKind`] describes the errors that can happen while executing a high-level command @@ -216,6 +235,28 @@ pub enum CommandErrorKind { ConversionFromIntFailed(TryFromIntError), /// {0} is not allowed on an append-only repository NotAllowedWithAppendOnly(String), + /// Failed to acquire a mutex lock + MutexLockFailed, + /// Reading file type failed: `{0}` + ErrorReadingFileType(PathBuf), + /// No last snapshot found: {snapshot_id} + NoLastSnapshot { snapshot_id: String }, + /// Duration error: {duration} + DurationError { + duration: String, + #[source] + source: OutOfRangeError, + }, + /// There are too many packs to delete: `{0}` + TooManyPacksToDelete(usize), + /// error setting metadata for {0:?}: {1:?} + ErrorSettingMetadata(PathBuf, Vec), + /// tree with id {0:?} has not been found + TreeNotFound(String), + /// data with id {0:?} has not been found + DataBlobNotFound(String), + /// dir {path} subtree doesn't exist + MissingSubtree { path: PathBuf }, } /// [`CryptoErrorKind`] describes the errors that can happen while dealing with Cryptographic functions @@ -242,16 +283,26 @@ pub enum FileErrorKind { /// did not find id in index: `{0:?}` CouldNotFindIdInIndex(Id), /// transposing an Option of a Result into a Result of an Option failed: `{0:?}` - TransposingOptionResultFailed(std::io::Error), + TransposingOptionResultFailed(#[from] std::io::Error), /// conversion from `u64` to `usize` failed: `{0:?}` - ConversionFromU64ToUsizeFailed(TryFromIntError), + ConversionFromU64ToUsizeFailed(#[from] TryFromIntError), } /// [`IdErrorKind`] describes the errors that can be returned by processing IDs -#[derive(Error, Debug, Display, Copy, Clone)] +#[derive(Error, Debug, Display)] pub enum IdErrorKind { - /// Hex decoding error: `{0:?}` - HexError(hex::FromHexError), + /// Hex error: `{0:?}` + #[error(transparent)] + HexError(#[from] hex::FromHexError), + /// Utf8 error: `{0:?}` + #[error(transparent)] + Utf8Error(#[from] Utf8Error), + /// Failed to parse Id from String `{0}` + ParsingIdFromStringFailed(String), + /// Empty hex string + EmptyHexString, + /// Non-ASCII hex string + NonAsciiHexString, } /// [`RepositoryErrorKind`] describes the errors that can be returned by processing Repositories @@ -265,11 +316,11 @@ pub enum RepositoryErrorKind { NoIDSpecified, /// error opening password file `{0:?}` OpeningPasswordFileFailed(std::io::Error), - /// No repository config file found. Is there a repo at {0}? + /// No repository config file found. Is there a repo at `{0}`? NoRepositoryConfigFound(String), - /// More than one repository config file at {0}. Aborting. + /// More than one repository config file at `{0}`. Aborting. MoreThanOneRepositoryConfig(String), - /// keys from repo and repo-hot do not match for {0}. Aborting. + /// keys from repo and repo-hot do not match for `{0}`. Aborting. KeysDontMatchForRepositories(String), /// repository is a hot repository!\nPlease use as --repo-hot in combination with the normal repo. Aborting. HotRepositoryFlagMissing, @@ -290,6 +341,7 @@ pub enum RepositoryErrorKind { /// error accessing config file AccessToConfigFileFailed, /// {0:?} + #[error(transparent)] FromSplitError(#[from] shell_words::ParseError), /// {0:?} FromThreadPoolbilderError(rayon::ThreadPoolBuildError), @@ -299,7 +351,7 @@ pub enum RepositoryErrorKind { ReadingPasswordFromPromptFailed(std::io::Error), /// Config file already exists. Aborting. ConfigFileExists, - /// did not find id {0} in index + /// did not find id `{0}` in index IdNotFound(Id), /// no suitable backend type found NoBackendTypeGiven, @@ -314,18 +366,20 @@ pub enum IndexErrorKind { GettingBlobIndexEntryFromBackendFailed, /// saving IndexFile failed SavingIndexFileFailed, + /// IndexFile is still in use + IndexStillInUse, } /// [`BackendAccessErrorKind`] describes the errors that can be returned by the various Backends #[derive(Error, Debug, Display)] pub enum BackendAccessErrorKind { - /// backend {0:?} is not supported! + /// backend `{0:?}` is not supported! BackendNotSupported(String), - /// backend {0} cannot be loaded: {1:?} + /// backend `{0}` cannot be loaded: `{1:?}` BackendLoadError(String, anyhow::Error), - /// no suitable id found for {0} + /// no suitable id found for `{0}` NoSuitableIdFound(String), - /// id {0} is not unique + /// id `{0}` is not unique IdNotUnique(String), /// {0:?} #[error(transparent)] @@ -417,13 +471,13 @@ pub enum PackFileErrorKind { /// [`SnapshotFileErrorKind`] describes the errors that can be returned for `SnapshotFile`s #[derive(Error, Debug, Display)] pub enum SnapshotFileErrorKind { - /// non-unicode hostname {0:?} + /// non-unicode hostname `{0:?}` NonUnicodeHostname(OsString), - /// non-unicode path {0:?} + /// non-unicode path `{0:?}` NonUnicodePath(PathBuf), /// no snapshots found NoSnapshotsFound, - /// value {0:?} not allowed + /// value `{0:?}` not allowed ValueNotAllowed(String), /// datetime out of range: `{0:?}` OutOfRange(#[from] OutOfRangeError), @@ -445,6 +499,39 @@ pub enum SnapshotFileErrorKind { CanonicalizingPathFailed(std::io::Error), } +/// [`ChannelErrorKind`] describes the errors that can be returned in relation to a crossbeam or other channel +#[derive(Error, Debug, Display)] +pub enum MultiprocessingErrorKind { + /// General channel error, crossbeam couldn't send message + SendingCrossbeamMessageFailed, + /// crossbeam couldn't send message: `{0:?}` + SendingCrossbeamMessageFailedWithBytes(#[from] SendError<(bytes::Bytes, Id, Option)>), + /// crossbeam couldn't send message: `{0:?}` + SendingCrossbeamMessageFailedForIndexPack(#[from] SendError<(bytes::Bytes, IndexPack)>), + /// failed to receive message for PackerStats: `{0:?}` + ReceivingCrossbeamMessageFailedForPackerStats(crossbeam_channel::RecvError), + /// failed to receive message: `{0:?}` + ReceivingCrossbeamMessageFailedForActorFinalizing(crossbeam_channel::RecvError), + /// crossbeam couldn't send message: `{0:?}` + SendingCrossbeamMessageFailedWithPath(#[from] SendError<(PathBuf, Id, usize)>), + /// crossbeam couldn't receive message: `{0:?}` + ReceivingCrossbreamMessageFailed(#[from] crossbeam_channel::RecvError), + /// Queue in is not available + QueueInNotAvailable, + /// crossbeam couldn't send message: `{0:?}` + SendingCrossbeamMessageFailedForStatus(String), + /// crossbeam couldn't send message: `{0:?}` + SendingCrossbeamMessageFailedForPackerStats(String), + /// failed to join threads in `{location}` + JoinError { location: String }, + /// failed during archival in `{location}` + ArchivingError { location: String }, + /// Receiver has been dropped unexpectedly + ReceiverDropped, + /// Sender has been dropped unexpectedly + SenderDropped, +} + /// [`PackerErrorKind`] describes the errors that can be returned for a Packer #[derive(Error, Debug, Display)] pub enum PackerErrorKind { @@ -456,14 +543,6 @@ pub enum PackerErrorKind { CompressingDataFailed(#[from] std::io::Error), /// getting total size failed GettingTotalSizeFailed, - /// crossbeam couldn't send message: `{0:?}` - SendingCrossbeamMessageFailed( - #[from] crossbeam_channel::SendError<(bytes::Bytes, Id, Option)>, - ), - /// crossbeam couldn't send message: `{0:?}` - SendingCrossbeamMessageFailedForIndexPack( - #[from] crossbeam_channel::SendError<(bytes::Bytes, IndexPack)>, - ), /// couldn't create binary representation for pack header: `{0:?}` CouldNotCreateBinaryRepresentationForHeader(#[from] PackFileErrorKind), /// failed to write bytes in backend: `{0:?}` @@ -478,6 +557,43 @@ pub enum PackerErrorKind { AddingIndexPackFailed(#[from] IndexErrorKind), /// conversion for integer failed: `{0:?}` IntConversionFailed(#[from] TryFromIntError), + /// No file writer present for packer + FileWriterHandleNotPresent, + /// No actor handle present for packer + ActorHandleNotPresent, + /// size of data is too large: {0} + SizeLimitExceeded(u32), + /// failed to add size {to_be_added} to current size: {current_size} + AddingSizeToCurrentSizeFailed { current_size: u64, to_be_added: u32 }, + /// overflowed while adding data: {data} + {data_added} + DataAddedOverflowed { data_added: u64, data: u64 }, + /// overflowed while adding data: {data_packed} + {data_added_packed} + DataAddedPackedOverflowed { + data_added_packed: u64, + data_packed: u64, + }, + /// overflowed while adding data: {blobs} + {tree_blobs} + TreeBlobsOverflowed { tree_blobs: u64, blobs: u64 }, + /// overflowed while adding data: {data} + {data_added_trees} + DataAddedTreesOverflowed { data_added_trees: u64, data: u64 }, + /// overflowed while adding data: {data_packed} + {data_added_trees_packed} + DataAddedTreesPackedOverflowed { + data_added_trees_packed: u64, + data_packed: u64, + }, + /// overflowed while adding data: {blobs} + {data_blobs} + DataBlobsOverflowed { data_blobs: u64, blobs: u64 }, + /// overflowed while adding data: {data} + {data_added_files} + DataAddedFilesOverflowed { data_added_files: u64, data: u64 }, + /// overflowed while adding data: {data_packed} + {data_added_files_packed} + DataAddedFilesPackedOverflowed { + data_added_files_packed: u64, + data_packed: u64, + }, + /// multiple errors from summary: {0:?} + MultipleFromSummary(Vec), + /// failed to calculate pack size from value {value} with error {comment} + IntConversionFailedInPackSizeCalculation { value: u64, comment: String }, } /// [`TreeErrorKind`] describes the errors that can come up dealing with Trees @@ -503,10 +619,10 @@ pub enum TreeErrorKind { BuildingNodeStreamerFailed(#[from] ignore::Error), /// failed to read file string from glob file: `{0:?}` ReadingFileStringFromGlobsFailed(#[from] std::io::Error), - /// crossbeam couldn't send message: `{0:?}` - SendingCrossbeamMessageFailed(#[from] crossbeam_channel::SendError<(PathBuf, Id, usize)>), - /// crossbeam couldn't receive message: `{0:?}` - ReceivingCrossbreamMessageFailed(#[from] crossbeam_channel::RecvError), + /// failed to find blob id for node: `{0:?}` + BlobIdNotFoundForNode(OsString), + /// no nodes found to be merged + NoNodeInListToBeMerged, } /// [`CacheBackendErrorKind`] describes the errors that can be returned by a Caching action in Backends @@ -531,6 +647,12 @@ pub enum CacheBackendErrorKind { WritingBytesOnCacheBackendFailed, /// removing data on CacheBackend failed RemovingDataOnCacheBackendFailed, + /// Cache location is invalid + CacheLocationInvalid, + /// Encountered Invalid ID in CacheBackend + InvalidId, + /// Encountered Invalid Path in CacheBackend + MetadataError(PathBuf), } /// [`CryptBackendErrorKind`] describes the errors that can be returned by a Decryption action in Backends @@ -592,6 +714,9 @@ pub enum IgnoreErrorKind { FromTryFromIntError(#[from] TryFromIntError), /// no unicode link target. File: {file:?}, target: {target:?} TargetIsNotValidUnicode { file: PathBuf, target: PathBuf }, + #[cfg(not(windows))] + /// xattr not found: {0} + XattrNotFound(String), } /// [`LocalDestinationErrorKind`] describes the errors that can be returned by an action on the filesystem in Backends @@ -617,19 +742,19 @@ pub enum LocalDestinationErrorKind { /// listing xattrs on {1:?}: {0} #[cfg(not(any(windows, target_os = "openbsd")))] ListingXattrsFailed(std::io::Error, PathBuf), - /// setting xattr {name} on {filename:?} with {source:?} + /// setting xattr {name} on {file_name:?} with {source:?} #[cfg(not(any(windows, target_os = "openbsd")))] SettingXattrFailed { name: String, - filename: PathBuf, + file_name: PathBuf, #[source] source: std::io::Error, }, - /// getting xattr {name} on {filename:?} with {source:?} + /// getting xattr {name} on {file_name:?} with {source:?} #[cfg(not(any(windows, target_os = "openbsd")))] GettingXattrFailed { name: String, - filename: PathBuf, + file_name: PathBuf, #[source] source: std::io::Error, }, @@ -652,11 +777,11 @@ pub enum LocalDestinationErrorKind { /// setting file permissions failed: `{0:?}` #[cfg(not(windows))] SettingFilePermissionsFailed(std::io::Error), - /// failed to symlink target {linktarget:?} from {filename:?} with {source:?} + /// failed to symlink target {link_target:?} from {file_name:?} with {source:?} #[cfg(not(windows))] SymlinkingFailed { - linktarget: PathBuf, - filename: PathBuf, + link_target: PathBuf, + file_name: PathBuf, #[source] source: std::io::Error, }, @@ -676,6 +801,14 @@ pub enum NodeErrorKind { /// Unrecognized Escape #[cfg(not(windows))] UnrecognizedEscape, + /// Invalid Link Target: called method on non-symlink! + InvalidLinkTarget, + /// Invalid sign encountered in formatting: `{0:?}` + SignWriteError(String), + /// Invalid UTF-8 encountered during escaping file name: `{0:?}` + FromUtf8Error(String), + /// Invalid file name: `{0:?}` + InvalidFileName(OsString), } /// [`StdInErrorKind`] describes the errors that can be returned while dealing IO from CLI @@ -685,6 +818,23 @@ pub enum StdInErrorKind { StdInError(#[from] std::io::Error), } +/// [`CheckErrorKind`] describes the errors that can be returned while checking snapshots, blobs and packs +#[derive(Error, Debug, Display)] +pub enum CheckErrorKind { + /// file {path} doesn't have content + MissingContent { path: PathBuf }, + /// file {path} blob {index} has null ID + BlobHasNullId { path: PathBuf, index: usize }, + /// file {path} blob {id} doesn't exit in index {index} + MissingBlob { path: PathBuf, id: Id, index: usize }, + /// dir {path} subtree doesn't exist + MissingSubtree { path: PathBuf }, + /// dir {path} subtree has null ID + SubtreeHasNullId { path: PathBuf }, + /// Errors encountered while checking: `{0:?}` + ErrorCollection(Vec), +} + /// [`ArchiverErrorKind`] describes the errors that can be returned from the archiver #[derive(Error, Debug, Display)] pub enum ArchiverErrorKind { @@ -721,6 +871,10 @@ pub enum ArchiverErrorKind { FromStripPrefix(#[from] StripPrefixError), /// conversion from `u64` to `usize` failed: `{0:?}` ConversionFromU64ToUsizeFailed(TryFromIntError), + /// parent node is no tree + ParentNodeIsNoTree, + /// tree parent without subtree + TreeParentWithoutSubtree, } /// [`VfsErrorKind`] describes the errors that can be returned from the Virtual File System @@ -734,6 +888,12 @@ pub enum VfsErrorKind { OnlyNormalPathsAreAllowed, /// Name `{0:?}`` doesn't exist NameDoesNotExist(OsString), + /// Data Blob not found: `{0:?}` + DataBlobNotFound(String), + /// Data Blob too large: `{0:?}` + DataBlobTooLarge(String), + /// Conversion for ID {1:?} from `u32` to `usize` failed: `{0:?}` + ConversionFromU32ToUsizeFailed(TryFromIntError, String), } trait RusticErrorMarker: Error {} @@ -750,6 +910,7 @@ impl RusticErrorMarker for PackFileErrorKind {} impl RusticErrorMarker for SnapshotFileErrorKind {} impl RusticErrorMarker for PackerErrorKind {} impl RusticErrorMarker for FileErrorKind {} +impl RusticErrorMarker for MultiprocessingErrorKind {} impl RusticErrorMarker for TreeErrorKind {} impl RusticErrorMarker for CacheBackendErrorKind {} impl RusticErrorMarker for CryptBackendErrorKind {} @@ -757,10 +918,12 @@ impl RusticErrorMarker for IgnoreErrorKind {} impl RusticErrorMarker for LocalDestinationErrorKind {} impl RusticErrorMarker for NodeErrorKind {} impl RusticErrorMarker for StdInErrorKind {} +impl RusticErrorMarker for CheckErrorKind {} impl RusticErrorMarker for ArchiverErrorKind {} impl RusticErrorMarker for CommandErrorKind {} impl RusticErrorMarker for VfsErrorKind {} impl RusticErrorMarker for std::io::Error {} +impl RusticErrorMarker for walkdir::Error {} impl From for RusticError where @@ -771,3 +934,40 @@ where Self(RusticErrorKind::from(value)) } } + +#[cfg(test)] +mod tests { + + use super::*; + + #[test] + fn test_rustic_error_passes() { + let error = RusticError::from(PolynomialErrorKind::NoSuitablePolynomialFound); + + assert_eq!(format!("{error}"), "no suitable polynomial found"); + + assert!(error.backend_error().is_none()); + + let inner_error = error.into_inner(); + + assert_eq!(format!("{inner_error}"), "no suitable polynomial found"); + } + + #[test] + fn test_rustic_error_api_with_backend_error_passes() { + let error = RusticError::from(RusticErrorKind::Backend(anyhow::anyhow!( + "backend \"test\" is not supported!".to_string() + ))); + + assert_eq!(format!("{error}"), "backend \"test\" is not supported!"); + + assert!(error.backend_error().is_some()); + + let inner_error = error.into_inner(); + + assert_eq!( + format!("{inner_error}"), + "backend \"test\" is not supported!" + ); + } +} From ccca56db81a0b334b95a423a226a3547cb891e56 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 12:33:56 +0100 Subject: [PATCH 08/58] rename filename to file_name (and dir_name) to make it consistent over the code base, also handle cli changes with alias Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/backend/src/local.rs | 26 +++-- crates/backend/src/opendal.rs | 8 +- crates/core/src/archiver/tree_archiver.rs | 8 +- crates/core/src/backend/cache.rs | 8 +- crates/core/src/backend/ignore.rs | 4 +- crates/core/src/backend/local_destination.rs | 112 +++++++++---------- crates/core/src/backend/node.rs | 26 ++--- crates/core/src/commands/backup.rs | 14 ++- crates/core/src/commands/restore.rs | 14 +-- crates/core/src/vfs.rs | 8 +- 10 files changed, 117 insertions(+), 111 deletions(-) diff --git a/crates/backend/src/local.rs b/crates/backend/src/local.rs index cc6943dc..4768f598 100644 --- a/crates/backend/src/local.rs +++ b/crates/backend/src/local.rs @@ -94,7 +94,7 @@ impl LocalBackend { /// /// * `tpe` - The type of the file. /// * `id` - The id of the file. - /// * `filename` - The path to the file. + /// * `file_name` - The path to the file. /// * `command` - The command to call. /// /// # Errors @@ -115,11 +115,19 @@ impl LocalBackend { /// [`LocalBackendErrorKind::FromSplitError`]: LocalBackendErrorKind::FromSplitError /// [`LocalBackendErrorKind::CommandExecutionFailed`]: LocalBackendErrorKind::CommandExecutionFailed /// [`LocalBackendErrorKind::CommandNotSuccessful`]: LocalBackendErrorKind::CommandNotSuccessful - fn call_command(tpe: FileType, id: &Id, filename: &Path, command: &str) -> Result<()> { + fn call_command(tpe: FileType, id: &Id, file_name: &Path, command: &str) -> Result<()> { let id = id.to_hex(); let patterns = &["%file", "%type", "%id"]; let ac = AhoCorasick::new(patterns).map_err(LocalBackendErrorKind::FromAhoCorasick)?; - let replace_with = &[filename.to_str().unwrap(), tpe.dirname(), id.as_str()]; + let replace_with = &[ + file_name + .to_str() + .ok_or(LocalBackendErrorKind::PathToStringFailed { + path: file_name.to_owned(), + })?, + tpe.dirname(), + id.as_str(), + ]; let actual_command = ac.replace_all(command, replace_with); debug!("calling {actual_command}..."); let commands = split(&actual_command).map_err(LocalBackendErrorKind::FromSplitError)?; @@ -343,11 +351,11 @@ impl WriteBackend for LocalBackend { /// [`LocalBackendErrorKind::SyncingOfOsMetadataFailed`]: LocalBackendErrorKind::SyncingOfOsMetadataFailed fn write_bytes(&self, tpe: FileType, id: &Id, _cacheable: bool, buf: Bytes) -> Result<()> { trace!("writing tpe: {:?}, id: {}", &tpe, &id); - let filename = self.path(tpe, id); + let file_name = self.path(tpe, id); let mut file = fs::OpenOptions::new() .create(true) .write(true) - .open(&filename) + .open(&file_name) .map_err(LocalBackendErrorKind::OpeningFileFailed)?; file.set_len( buf.len() @@ -360,7 +368,7 @@ impl WriteBackend for LocalBackend { file.sync_all() .map_err(LocalBackendErrorKind::SyncingOfOsMetadataFailed)?; if let Some(command) = &self.post_create_command { - if let Err(err) = Self::call_command(tpe, id, &filename, command) { + if let Err(err) = Self::call_command(tpe, id, &file_name, command) { warn!("post-create: {err}"); } } @@ -382,10 +390,10 @@ impl WriteBackend for LocalBackend { /// [`LocalBackendErrorKind::FileRemovalFailed`]: LocalBackendErrorKind::FileRemovalFailed fn remove(&self, tpe: FileType, id: &Id, _cacheable: bool) -> Result<()> { trace!("removing tpe: {:?}, id: {}", &tpe, &id); - let filename = self.path(tpe, id); - fs::remove_file(&filename).map_err(LocalBackendErrorKind::FileRemovalFailed)?; + let file_name = self.path(tpe, id); + fs::remove_file(&file_name).map_err(LocalBackendErrorKind::FileRemovalFailed)?; if let Some(command) = &self.post_delete_command { - if let Err(err) = Self::call_command(tpe, id, &filename, command) { + if let Err(err) = Self::call_command(tpe, id, &file_name, command) { warn!("post-delete: {err}"); } } diff --git a/crates/backend/src/opendal.rs b/crates/backend/src/opendal.rs index 95a8de56..de6166d4 100644 --- a/crates/backend/src/opendal.rs +++ b/crates/backend/src/opendal.rs @@ -234,8 +234,8 @@ impl WriteBackend for OpenDALBackend { /// * `buf` - The bytes to write. fn write_bytes(&self, tpe: FileType, id: &Id, _cacheable: bool, buf: Bytes) -> Result<()> { trace!("writing tpe: {:?}, id: {}", &tpe, &id); - let filename = self.path(tpe, id); - self.operator.write(&filename, buf)?; + let file_name = self.path(tpe, id); + self.operator.write(&file_name, buf)?; Ok(()) } @@ -248,8 +248,8 @@ impl WriteBackend for OpenDALBackend { /// * `cacheable` - Whether the file is cacheable. fn remove(&self, tpe: FileType, id: &Id, _cacheable: bool) -> Result<()> { trace!("removing tpe: {:?}, id: {}", &tpe, &id); - let filename = self.path(tpe, id); - self.operator.delete(&filename)?; + let file_name = self.path(tpe, id); + self.operator.delete(&file_name)?; Ok(()) } } diff --git a/crates/core/src/archiver/tree_archiver.rs b/crates/core/src/archiver/tree_archiver.rs index b6728d5b..48e974dd 100644 --- a/crates/core/src/archiver/tree_archiver.rs +++ b/crates/core/src/archiver/tree_archiver.rs @@ -132,18 +132,18 @@ impl<'a, BE: DecryptWriteBackend, I: ReadGlobalIndex> TreeArchiver<'a, BE, I> { /// * `node` - The node of the file. /// * `parent` - The parent result of the file. fn add_file(&mut self, path: &Path, node: Node, parent: &ParentResult<()>, size: u64) { - let filename = path.join(node.name()); + let file_name = path.join(node.name()); match parent { ParentResult::Matched(()) => { - debug!("unchanged file: {:?}", filename); + debug!("unchanged file: {:?}", file_name); self.summary.files_unmodified += 1; } ParentResult::NotMatched => { - debug!("changed file: {:?}", filename); + debug!("changed file: {:?}", file_name); self.summary.files_changed += 1; } ParentResult::NotFound => { - debug!("new file: {:?}", filename); + debug!("new file: {:?}", file_name); self.summary.files_new += 1; } } diff --git a/crates/core/src/backend/cache.rs b/crates/core/src/backend/cache.rs index 50704286..acb99e95 100644 --- a/crates/core/src/backend/cache.rs +++ b/crates/core/src/backend/cache.rs @@ -429,11 +429,11 @@ impl Cache { pub fn write_bytes(&self, tpe: FileType, id: &Id, buf: &Bytes) -> RusticResult<()> { trace!("cache writing tpe: {:?}, id: {}", &tpe, &id); fs::create_dir_all(self.dir(tpe, id)).map_err(CacheBackendErrorKind::FromIoError)?; - let filename = self.path(tpe, id); + let file_name = self.path(tpe, id); let mut file = fs::OpenOptions::new() .create(true) .write(true) - .open(filename) + .open(file_name) .map_err(CacheBackendErrorKind::FromIoError)?; file.write_all(buf) .map_err(CacheBackendErrorKind::FromIoError)?; @@ -454,8 +454,8 @@ impl Cache { /// [`CacheBackendErrorKind::FromIoError`]: crate::error::CacheBackendErrorKind::FromIoError pub fn remove(&self, tpe: FileType, id: &Id) -> RusticResult<()> { trace!("cache writing tpe: {:?}, id: {}", &tpe, &id); - let filename = self.path(tpe, id); - fs::remove_file(filename).map_err(CacheBackendErrorKind::FromIoError)?; + let file_name = self.path(tpe, id); + fs::remove_file(file_name).map_err(CacheBackendErrorKind::FromIoError)?; Ok(()) } } diff --git a/crates/core/src/backend/ignore.rs b/crates/core/src/backend/ignore.rs index 787c86d9..fc46028e 100644 --- a/crates/core/src/backend/ignore.rs +++ b/crates/core/src/backend/ignore.rs @@ -97,12 +97,12 @@ pub struct LocalSourceFilterOptions { #[cfg_attr(feature = "merge", merge(strategy = merge::bool::overwrite_false))] pub no_require_git: bool, - /// Treat the provided filename like a .gitignore file (can be specified multiple times) + /// Treat the provided file name like a .gitignore file (can be specified multiple times) #[cfg_attr(feature = "clap", clap(long, value_name = "FILE"))] #[cfg_attr(feature = "merge", merge(strategy = merge::vec::overwrite_empty))] pub custom_ignorefile: Vec, - /// Exclude contents of directories containing this filename (can be specified multiple times) + /// Exclude contents of directories containing this file name (can be specified multiple times) #[cfg_attr(feature = "clap", clap(long, value_name = "FILE"))] #[cfg_attr(feature = "merge", merge(strategy = merge::vec::overwrite_empty))] pub exclude_if_present: Vec, diff --git a/crates/core/src/backend/local_destination.rs b/crates/core/src/backend/local_destination.rs index 3be93aa1..285b214a 100644 --- a/crates/core/src/backend/local_destination.rs +++ b/crates/core/src/backend/local_destination.rs @@ -126,8 +126,8 @@ impl LocalDestination { /// This will remove the directory recursively. /// /// [`LocalDestinationErrorKind::DirectoryRemovalFailed`]: crate::error::LocalDestinationErrorKind::DirectoryRemovalFailed - pub fn remove_dir(&self, dirname: impl AsRef) -> RusticResult<()> { - Ok(fs::remove_dir_all(dirname) + pub fn remove_dir(&self, dir_name: impl AsRef) -> RusticResult<()> { + Ok(fs::remove_dir_all(self.path_of(dir_name)) .map_err(LocalDestinationErrorKind::DirectoryRemovalFailed)?) } @@ -135,7 +135,7 @@ impl LocalDestination { /// /// # Arguments /// - /// * `filename` - The file to remove + /// * `file_name` - The file to remove /// /// # Errors /// @@ -149,8 +149,9 @@ impl LocalDestination { /// * If the file is a directory or device, this will fail. /// /// [`LocalDestinationErrorKind::FileRemovalFailed`]: crate::error::LocalDestinationErrorKind::FileRemovalFailed - pub fn remove_file(&self, filename: impl AsRef) -> RusticResult<()> { - Ok(fs::remove_file(filename).map_err(LocalDestinationErrorKind::FileRemovalFailed)?) + pub fn remove_file(&self, file_name: impl AsRef) -> RusticResult<()> { + Ok(fs::remove_file(self.path_of(file_name)) + .map_err(LocalDestinationErrorKind::FileRemovalFailed)?) } /// Create the given directory (relative to the base path) @@ -187,11 +188,11 @@ impl LocalDestination { /// /// [`LocalDestinationErrorKind::SettingTimeMetadataFailed`]: crate::error::LocalDestinationErrorKind::SettingTimeMetadataFailed pub fn set_times(&self, item: impl AsRef, meta: &Metadata) -> RusticResult<()> { - let filename = self.path(item); + let file_name = self.path_of(item); if let Some(mtime) = meta.mtime { let atime = meta.atime.unwrap_or(mtime); set_symlink_file_times( - filename, + file_name, FileTime::from_system_time(atime.into()), FileTime::from_system_time(mtime.into()), ) @@ -235,7 +236,7 @@ impl LocalDestination { /// [`LocalDestinationErrorKind::FromErrnoError`]: crate::error::LocalDestinationErrorKind::FromErrnoError #[allow(clippy::similar_names)] pub fn set_user_group(&self, item: impl AsRef, meta: &Metadata) -> RusticResult<()> { - let filename = self.path(item); + let file_name = self.path_of(item); let user = meta.user.clone().and_then(uid_from_name); // use uid from user if valid, else from saved uid (if saved) @@ -245,7 +246,7 @@ impl LocalDestination { // use gid from group if valid, else from saved gid (if saved) let gid = group.or_else(|| meta.gid.map(Gid::from_raw)); - fchownat(None, &filename, uid, gid, AtFlags::AT_SYMLINK_NOFOLLOW) + fchownat(None, &file_name, uid, gid, AtFlags::AT_SYMLINK_NOFOLLOW) .map_err(LocalDestinationErrorKind::FromErrnoError)?; Ok(()) } @@ -281,12 +282,12 @@ impl LocalDestination { /// [`LocalDestinationErrorKind::FromErrnoError`]: crate::error::LocalDestinationErrorKind::FromErrnoError #[allow(clippy::similar_names)] pub fn set_uid_gid(&self, item: impl AsRef, meta: &Metadata) -> RusticResult<()> { - let filename = self.path(item); + let file_name = self.path_of(item); let uid = meta.uid.map(Uid::from_raw); let gid = meta.gid.map(Gid::from_raw); - fchownat(None, &filename, uid, gid, AtFlags::AT_SYMLINK_NOFOLLOW) + fchownat(None, &file_name, uid, gid, AtFlags::AT_SYMLINK_NOFOLLOW) .map_err(LocalDestinationErrorKind::FromErrnoError)?; Ok(()) } @@ -326,11 +327,11 @@ impl LocalDestination { return Ok(()); } - let filename = self.path(item); + let file_name = self.path_of(item); if let Some(mode) = node.meta.mode { let mode = map_mode_from_go(mode); - std::fs::set_permissions(filename, fs::Permissions::from_mode(mode)) + std::fs::set_permissions(file_name, fs::Permissions::from_mode(mode)) .map_err(LocalDestinationErrorKind::SettingFilePermissionsFailed)?; } Ok(()) @@ -387,48 +388,45 @@ impl LocalDestination { item: impl AsRef, extended_attributes: &[ExtendedAttribute], ) -> RusticResult<()> { - let filename = self.path(item); - let mut done = vec![false; extended_attributes.len()]; + let file_name = self.path_of(item); - for curr_name in xattr::list(&filename) - .map_err(|err| LocalDestinationErrorKind::ListingXattrsFailed(err, filename.clone()))? + for curr_name in xattr::list(&file_name) + .map_err(|err| LocalDestinationErrorKind::ListingXattrsFailed(err, file_name.clone()))? { match extended_attributes.iter().enumerate().find( |(_, ExtendedAttribute { name, .. })| name == curr_name.to_string_lossy().as_ref(), ) { Some((index, ExtendedAttribute { name, value })) => { - let curr_value = xattr::get(&filename, name) - .map_err(|err| LocalDestinationErrorKind::GettingXattrFailed { + if let Some(curr_value) = xattr::get(&file_name, name).map_err(|err| { + LocalDestinationErrorKind::GettingXattrFailed { name: name.clone(), - filename: filename.clone(), + file_name: file_name.clone(), source: err, - })? - .unwrap(); - if value != &curr_value { - xattr::set(&filename, name, value).map_err(|err| { - LocalDestinationErrorKind::SettingXattrFailed { - name: name.clone(), - filename: filename.clone(), - source: err, - } - })?; - } - done[index] = true; + } + })? { + if value != &curr_value { + xattr::set(&file_name, name, value).map_err(|err| { + LocalDestinationErrorKind::SettingXattrFailed { + name: name.clone(), + file_name: file_name.clone(), + source: err, + } + })?; } None => { - if let Err(err) = xattr::remove(&filename, &curr_name) { - warn!("error removing xattr {curr_name:?} on {filename:?}: {err}"); + if let Err(err) = xattr::remove(&file_name, &curr_name) { + warn!("error removing xattr {curr_name:?} on {file_name:?}: {err}"); } } } } for (index, ExtendedAttribute { name, value }) in extended_attributes.iter().enumerate() { - if !done[index] { - xattr::set(&filename, name, value).map_err(|err| { + if !successful[index] { + xattr::set(&file_name, name, value).map_err(|err| { LocalDestinationErrorKind::SettingXattrFailed { name: name.clone(), - filename: filename.clone(), + file_name: file_name.clone(), source: err, } })?; @@ -462,16 +460,16 @@ impl LocalDestination { /// [`LocalDestinationErrorKind::OpeningFileFailed`]: crate::error::LocalDestinationErrorKind::OpeningFileFailed /// [`LocalDestinationErrorKind::SettingFileLengthFailed`]: crate::error::LocalDestinationErrorKind::SettingFileLengthFailed pub fn set_length(&self, item: impl AsRef, size: u64) -> RusticResult<()> { - let filename = self.path(item); - let dir = filename + let file_name = self.path_of(item); + let dir = file_name .parent() - .ok_or_else(|| LocalDestinationErrorKind::FileDoesNotHaveParent(filename.clone()))?; + .ok_or_else(|| LocalDestinationErrorKind::FileDoesNotHaveParent(file_name.clone()))?; fs::create_dir_all(dir).map_err(LocalDestinationErrorKind::DirectoryCreationFailed)?; OpenOptions::new() .create(true) .write(true) - .open(filename) + .open(file_name) .map_err(LocalDestinationErrorKind::OpeningFileFailed)? .set_len(size) .map_err(LocalDestinationErrorKind::SettingFileLengthFailed)?; @@ -516,15 +514,15 @@ impl LocalDestination { /// [`LocalDestinationErrorKind::FromTryIntError`]: crate::error::LocalDestinationErrorKind::FromTryIntError /// [`LocalDestinationErrorKind::FromErrnoError`]: crate::error::LocalDestinationErrorKind::FromErrnoError pub fn create_special(&self, item: impl AsRef, node: &Node) -> RusticResult<()> { - let filename = self.path(item); + let file_name = self.path_of(item); match &node.node_type { NodeType::Symlink { .. } => { - let linktarget = node.node_type.to_link(); - symlink(linktarget, &filename).map_err(|err| { + let link_target = node.node_type.to_link()?; + symlink(link_target, &file_name).map_err(|err| { LocalDestinationErrorKind::SymlinkingFailed { - linktarget: linktarget.to_path_buf(), - filename, + link_target: link_target.to_path_buf(), + file_name, source: err, } })?; @@ -542,7 +540,7 @@ impl LocalDestination { #[cfg(target_os = "freebsd")] let device = u32::try_from(*device).map_err(LocalDestinationErrorKind::FromTryIntError)?; - mknod(&filename, SFlag::S_IFBLK, Mode::empty(), device) + mknod(&file_name, SFlag::S_IFBLK, Mode::empty(), device) .map_err(LocalDestinationErrorKind::FromErrnoError)?; } NodeType::Chardev { device } => { @@ -558,15 +556,15 @@ impl LocalDestination { #[cfg(target_os = "freebsd")] let device = u32::try_from(*device).map_err(LocalDestinationErrorKind::FromTryIntError)?; - mknod(&filename, SFlag::S_IFCHR, Mode::empty(), device) + mknod(&file_name, SFlag::S_IFCHR, Mode::empty(), device) .map_err(LocalDestinationErrorKind::FromErrnoError)?; } NodeType::Fifo => { - mknod(&filename, SFlag::S_IFIFO, Mode::empty(), 0) + mknod(&file_name, SFlag::S_IFIFO, Mode::empty(), 0) .map_err(LocalDestinationErrorKind::FromErrnoError)?; } NodeType::Socket => { - mknod(&filename, SFlag::S_IFSOCK, Mode::empty(), 0) + mknod(&file_name, SFlag::S_IFSOCK, Mode::empty(), 0) .map_err(LocalDestinationErrorKind::FromErrnoError)?; } _ => {} @@ -594,9 +592,9 @@ impl LocalDestination { /// [`LocalDestinationErrorKind::FromTryIntError`]: crate::error::LocalDestinationErrorKind::FromTryIntError /// [`LocalDestinationErrorKind::ReadingExactLengthOfFileFailed`]: crate::error::LocalDestinationErrorKind::ReadingExactLengthOfFileFailed pub fn read_at(&self, item: impl AsRef, offset: u64, length: u64) -> RusticResult { - let filename = self.path(item); + let file_name = self.path_of(item); let mut file = - File::open(filename).map_err(LocalDestinationErrorKind::OpeningFileFailed)?; + File::open(file_name).map_err(LocalDestinationErrorKind::OpeningFileFailed)?; _ = file .seek(SeekFrom::Start(offset)) .map_err(LocalDestinationErrorKind::CouldNotSeekToPositionInFile)?; @@ -623,12 +621,12 @@ impl LocalDestination { /// If a file exists and size matches, this returns a `File` open for reading. /// In all other cases, returns `None` pub fn get_matching_file(&self, item: impl AsRef, size: u64) -> Option { - let filename = self.path(item); - fs::symlink_metadata(&filename).map_or_else( + let file_name = self.path_of(item); + fs::symlink_metadata(&file_name).map_or_else( |_| None, |meta| { if meta.is_file() && meta.len() == size { - File::open(&filename).ok() + File::open(&file_name).ok() } else { None } @@ -658,11 +656,11 @@ impl LocalDestination { /// [`LocalDestinationErrorKind::CouldNotSeekToPositionInFile`]: crate::error::LocalDestinationErrorKind::CouldNotSeekToPositionInFile /// [`LocalDestinationErrorKind::CouldNotWriteToBuffer`]: crate::error::LocalDestinationErrorKind::CouldNotWriteToBuffer pub fn write_at(&self, item: impl AsRef, offset: u64, data: &[u8]) -> RusticResult<()> { - let filename = self.path(item); + let file_name = self.path_of(item); let mut file = fs::OpenOptions::new() .create(true) .write(true) - .open(filename) + .open(file_name) .map_err(LocalDestinationErrorKind::OpeningFileFailed)?; _ = file .seek(SeekFrom::Start(offset)) diff --git a/crates/core/src/backend/node.rs b/crates/core/src/backend/node.rs index bd3fc805..be44d7e0 100644 --- a/crates/core/src/backend/node.rs +++ b/crates/core/src/backend/node.rs @@ -275,10 +275,7 @@ impl Node { /// # Returns /// /// The created [`Node`] - #[must_use] - pub fn new_node(name: &OsStr, node_type: NodeType, meta: Metadata) -> Self { - Self { - name: escape_filename(name), + name: escape_file_name(name)?, node_type, content: None, subtree: None, @@ -323,7 +320,7 @@ impl Node { /// /// If the name is not valid unicode pub fn name(&self) -> OsString { - unescape_filename(&self.name).unwrap_or_else(|_| OsString::from_str(&self.name).unwrap()) + unescape_file_name(&self.name).expect("unescaping should be infallible") } } @@ -346,8 +343,7 @@ pub fn last_modified_node(n1: &Node, n2: &Node) -> Ordering { // TODO(Windows): This is not able to handle non-unicode filenames and // doesn't treat filenames which need and escape (like `\`, `"`, ...) correctly #[cfg(windows)] -fn escape_filename(name: &OsStr) -> String { - name.to_string_lossy().to_string() +fn escape_file_name(name: &OsStr) -> RusticResult { } /// Unescape a filename @@ -356,21 +352,21 @@ fn escape_filename(name: &OsStr) -> String { /// /// * `s` - The escaped filename #[cfg(windows)] -fn unescape_filename(s: &str) -> Result { +fn unescape_file_name(s: &str) -> Result { OsString::from_str(s) } #[cfg(not(windows))] -/// Escape a filename +/// Escape a file name /// /// # Arguments /// -/// * `name` - The filename to escape -// This escapes the filename in a way that *should* be compatible to golangs +/// * `name` - The file name to escape +// This escapes the file name in a way that *should* be compatible to golangs // stconv.Quote, see https://pkg.go.dev/strconv#Quote // However, so far there was no specification what Quote really does, so this // is some kind of try-and-error and maybe does not cover every case. -fn escape_filename(name: &OsStr) -> String { +fn escape_file_name(name: &OsStr) -> RusticResult { let mut input = name.as_bytes(); let mut s = String::with_capacity(name.len()); @@ -419,13 +415,13 @@ fn escape_filename(name: &OsStr) -> String { } #[cfg(not(windows))] -/// Unescape a filename +/// Unescape a file name /// /// # Arguments /// -/// * `s` - The escaped filename +/// * `s` - The escaped file name // inspired by the enquote crate -fn unescape_filename(s: &str) -> RusticResult { +fn unescape_file_name(s: &str) -> RusticResult { let mut chars = s.chars(); let mut u = Vec::new(); loop { diff --git a/crates/core/src/commands/backup.rs b/crates/core/src/commands/backup.rs index 510e3124..aeef0d92 100644 --- a/crates/core/src/commands/backup.rs +++ b/crates/core/src/commands/backup.rs @@ -130,13 +130,19 @@ impl ParentOptions { #[non_exhaustive] /// Options for the `backup` command. pub struct BackupOptions { - /// Set filename to be used when backing up from stdin + /// Set file name to be used when backing up from stdin #[cfg_attr( feature = "clap", - clap(long, value_name = "FILENAME", default_value = "stdin") + clap( + long, + value_name = "FILE_NAME", + alias = "stdin-filename", + default_value = "stdin" + ) )] + #[serde(alias = "stdin-filename")] #[cfg_attr(feature = "merge", merge(skip))] - pub stdin_filename: String, + pub stdin_file_name: String, /// Manually set backup path in snapshot #[cfg_attr(feature = "clap", clap(long, value_name = "PATH"))] @@ -209,7 +215,7 @@ pub(crate) fn backup( let backup_stdin = *source == PathList::from_string("-")?; let backup_path = if backup_stdin { - vec![PathBuf::from(&opts.stdin_filename)] + vec![PathBuf::from(&opts.stdin_file_name)] } else { source.paths() }; diff --git a/crates/core/src/commands/restore.rs b/crates/core/src/commands/restore.rs index d0d58be1..c880cd5c 100644 --- a/crates/core/src/commands/restore.rs +++ b/crates/core/src/commands/restore.rs @@ -39,7 +39,7 @@ pub(crate) mod constants { } type RestoreInfo = BTreeMap<(Id, BlobLocation), Vec>; -type Filenames = Vec; +type FileNames = Vec; #[allow(clippy::struct_excessive_bools)] #[cfg_attr(feature = "clap", derive(clap::Parser))] @@ -428,19 +428,19 @@ fn restore_contents( file_infos: RestorePlan, ) -> RusticResult<()> { let RestorePlan { - names: filenames, + names: file_names, file_lengths, r: restore_info, restore_size: total_size, .. } = file_infos; - let filenames = &filenames; + let file_names = &file_names; let be = repo.dbe(); // first create needed empty files, as they are not created later. for (i, size) in file_lengths.iter().enumerate() { if *size == 0 { - let path = &filenames[i]; + let path = &file_names[i]; dest.set_length(path, *size) .map_err(|err| CommandErrorKind::ErrorSettingLength(path.clone(), Box::new(err)))?; } @@ -497,8 +497,7 @@ fn restore_contents( let read_data = match &from_file { Some((file_idx, offset_file, length_file)) => { // read from existing file - dest.read_at(&filenames[*file_idx], *offset_file, *length_file) - .unwrap() + match dest.read_at(&file_names[*file_idx], *offset_file, *length_file) { } None => { // read needed part of the pack @@ -523,8 +522,7 @@ fn restore_contents( }; for (_, file_idx, start) in group { let data = data.clone(); - s1.spawn(move |_| { - let path = &filenames[file_idx]; + let path = &file_names[file_idx]; // Allocate file if it is not yet allocated let mut sizes_guard = sizes.lock().unwrap(); let filesize = sizes_guard[file_idx]; diff --git a/crates/core/src/vfs.rs b/crates/core/src/vfs.rs index f54e5715..b5de11ca 100644 --- a/crates/core/src/vfs.rs +++ b/crates/core/src/vfs.rs @@ -245,19 +245,19 @@ impl Vfs { ) .to_string(); let path = Path::new(&path); - let filename = path.file_name().map(OsStr::to_os_string); + let file_name = path.file_name().map(OsStr::to_os_string); let parent_path = path.parent().map(Path::to_path_buf); // Save pathes for latest entries, if requested if matches!(latest_option, Latest::AsLink) { - _ = dirs_for_link.insert(parent_path.clone(), filename.clone()); + _ = dirs_for_link.insert(parent_path.clone(), file_name.clone()); } if matches!(latest_option, Latest::AsDir) { _ = dirs_for_snap.insert(parent_path.clone(), snap.tree); } // Create the entry, potentially as symlink if requested - if last_parent != parent_path || last_name != filename { + if last_parent != parent_path || last_name != file_name { if matches!(id_snap_option, IdenticalSnapshot::AsLink) && last_parent == parent_path && last_tree == snap.tree @@ -270,7 +270,7 @@ impl Vfs { } } last_parent = parent_path; - last_name = filename; + last_name = file_name; last_tree = snap.tree; } From f24105e1dfe09075f7ec10e49d8e66223ade31e6 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 12:37:18 +0100 Subject: [PATCH 09/58] rename new_cache to from_backend to make source/intent clearer Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/cache.rs | 2 +- crates/core/src/repository.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/core/src/backend/cache.rs b/crates/core/src/backend/cache.rs index acb99e95..ed7bf7e2 100644 --- a/crates/core/src/backend/cache.rs +++ b/crates/core/src/backend/cache.rs @@ -40,7 +40,7 @@ impl CachedBackend { /// # Type Parameters /// /// * `BE` - The backend to cache. - pub fn new_cache(be: Arc, cache: Cache) -> Arc { + pub fn from_backend(be: Arc, cache: Cache) -> Arc { Arc::new(Self { be, cache }) } } diff --git a/crates/core/src/repository.rs b/crates/core/src/repository.rs index ff8d1db0..ff61e17d 100644 --- a/crates/core/src/repository.rs +++ b/crates/core/src/repository.rs @@ -634,7 +634,7 @@ impl Repository { .flatten(); if let Some(cache) = &cache { - self.be = CachedBackend::new_cache(self.be.clone(), cache.clone()); + self.be = CachedBackend::from_backend(self.be.clone(), cache.clone()); info!("using cache at {}", cache.location()); } else { info!("using no cache"); From 0a626450f1c7062bf75bca71593c38b36f95689a Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 12:50:04 +0100 Subject: [PATCH 10/58] handle fallible location() method Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/backend/src/local.rs | 20 ++++++++++++++++---- crates/backend/src/opendal.rs | 4 ++-- crates/backend/src/rclone.rs | 4 ++-- crates/backend/src/rest.rs | 9 +++++---- crates/core/src/backend.rs | 4 ++-- crates/core/src/backend/cache.rs | 15 +++++++++------ crates/core/src/backend/dry_run.rs | 2 +- crates/core/src/backend/hotcold.rs | 2 +- crates/core/src/backend/warm_up.rs | 2 +- crates/core/src/repository.rs | 4 ++-- 10 files changed, 41 insertions(+), 25 deletions(-) diff --git a/crates/backend/src/local.rs b/crates/backend/src/local.rs index 4768f598..982dc5da 100644 --- a/crates/backend/src/local.rs +++ b/crates/backend/src/local.rs @@ -149,13 +149,25 @@ impl LocalBackend { } impl ReadBackend for LocalBackend { - /// Returns the location of the backend. + /// Get the location name of the backend. /// /// This is `local:`. - fn location(&self) -> String { + /// + /// # Returns + /// + /// The location of the backend. + /// + /// # Notes + /// + /// The path is the path to the backend. In case the path is not a valid UTF-8 string, + /// the path is converted to a string lossy. + fn location(&self) -> Result { let mut location = "local:".to_string(); - location.push_str(&self.path.to_string_lossy()); - location + let string_lossy = &self.path.to_string_lossy().to_string(); + let string_lossy = string_lossy.as_str(); + let path = self.path.to_str().map_or_else(|| string_lossy, |path| path); + location.push_str(path); + Ok(location) } /// Lists all files of the given type. diff --git a/crates/backend/src/opendal.rs b/crates/backend/src/opendal.rs index de6166d4..122691da 100644 --- a/crates/backend/src/opendal.rs +++ b/crates/backend/src/opendal.rs @@ -105,10 +105,10 @@ impl ReadBackend for OpenDALBackend { /// Returns the location of the backend. /// /// This is `local:`. - fn location(&self) -> String { + fn location(&self) -> Result { let mut location = "opendal:".to_string(); location.push_str(self.operator.info().name()); - location + Ok(location) } /// Lists all files of the given type. diff --git a/crates/backend/src/rclone.rs b/crates/backend/src/rclone.rs index 6dd5e76e..173ffcbf 100644 --- a/crates/backend/src/rclone.rs +++ b/crates/backend/src/rclone.rs @@ -234,8 +234,8 @@ impl RcloneBackend { impl ReadBackend for RcloneBackend { /// Returns the location of the backend. - fn location(&self) -> String { - "rclone:".to_string() + &self.url + fn location(&self) -> Result { + Ok("rclone:".to_string() + &self.url) } /// Returns the size of the given file. diff --git a/crates/backend/src/rest.rs b/crates/backend/src/rest.rs index 5ce102a3..e892fed0 100644 --- a/crates/backend/src/rest.rs +++ b/crates/backend/src/rest.rs @@ -245,14 +245,15 @@ impl RestBackend { impl ReadBackend for RestBackend { /// Returns the location of the backend. - fn location(&self) -> String { + fn location(&self) -> Result { let mut location = "rest:".to_string(); let mut url = self.url.clone(); - if url.password().is_some() { - url.set_password(Some("***")).unwrap(); + if url.password().is_some() && url.set_password(Some("[redacted]")).is_err() { + error!("Could not redact password! To avoid leaking, we stop here."); + return Err(RestErrorKind::RedactingPasswordFailed.into()); } location.push_str(url.as_str()); - location + Ok(location) } /// Returns a list of all files of a given type with their size. diff --git a/crates/core/src/backend.rs b/crates/core/src/backend.rs index 50a4c911..57431029 100644 --- a/crates/core/src/backend.rs +++ b/crates/core/src/backend.rs @@ -376,7 +376,7 @@ impl WriteBackend for Arc { } impl ReadBackend for Arc { - fn location(&self) -> String { + fn location(&self) -> Result { self.deref().location() } fn list_with_size(&self, tpe: FileType) -> Result> { @@ -403,7 +403,7 @@ impl ReadBackend for Arc { impl std::fmt::Debug for dyn WriteBackend { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "WriteBackend{{{}}}", self.location()) + write!(f, "WriteBackend{{{:#?}}}", self.location()) } } diff --git a/crates/core/src/backend/cache.rs b/crates/core/src/backend/cache.rs index ed7bf7e2..3b123200 100644 --- a/crates/core/src/backend/cache.rs +++ b/crates/core/src/backend/cache.rs @@ -47,7 +47,7 @@ impl CachedBackend { impl ReadBackend for CachedBackend { /// Returns the location of the backend as a String. - fn location(&self) -> String { + fn location(&self) -> Result { self.be.location() } @@ -240,13 +240,16 @@ impl Cache { /// Returns the path to the location of this [`Cache`]. /// - /// # Panics + /// # Errors + /// + /// * [`CacheBackendErrorKind::CacheLocationInvalid`] - If the path is not a valid string. + /// + /// # Returns /// - /// Panics if the path is not valid unicode. - // TODO: Does this need to panic? Result? + /// The path to the location of this [`Cache`]. #[must_use] - pub fn location(&self) -> &str { - self.path.to_str().unwrap() + pub fn location(&self) -> &Path { + self.path.as_path() } /// Returns the path to the directory of the given type. diff --git a/crates/core/src/backend/dry_run.rs b/crates/core/src/backend/dry_run.rs index e93c9023..6c2b34d7 100644 --- a/crates/core/src/backend/dry_run.rs +++ b/crates/core/src/backend/dry_run.rs @@ -73,7 +73,7 @@ impl DecryptReadBackend for DryRunBackend { } impl ReadBackend for DryRunBackend { - fn location(&self) -> String { + fn location(&self) -> Result { self.be.location() } diff --git a/crates/core/src/backend/hotcold.rs b/crates/core/src/backend/hotcold.rs index 75a1f750..1f2eaab4 100644 --- a/crates/core/src/backend/hotcold.rs +++ b/crates/core/src/backend/hotcold.rs @@ -41,7 +41,7 @@ impl HotColdBackend { } impl ReadBackend for HotColdBackend { - fn location(&self) -> String { + fn location(&self) -> Result { self.be.location() } diff --git a/crates/core/src/backend/warm_up.rs b/crates/core/src/backend/warm_up.rs index 87e9e009..dcc284d1 100644 --- a/crates/core/src/backend/warm_up.rs +++ b/crates/core/src/backend/warm_up.rs @@ -27,7 +27,7 @@ impl WarmUpAccessBackend { } impl ReadBackend for WarmUpAccessBackend { - fn location(&self) -> String { + fn location(&self) -> Result { self.be.location() } diff --git a/crates/core/src/repository.rs b/crates/core/src/repository.rs index ff61e17d..59c6d8c6 100644 --- a/crates/core/src/repository.rs +++ b/crates/core/src/repository.rs @@ -332,11 +332,11 @@ impl

Repository { be = WarmUpAccessBackend::new_warm_up(be); } - let mut name = be.location(); + let mut name = be.location().map_err(RusticErrorKind::Backend)?; if let Some(be_hot) = &be_hot { be = Arc::new(HotColdBackend::new(be, be_hot.clone())); name.push('#'); - name.push_str(&be_hot.location()); + name.push_str(&be_hot.location().map_err(RusticErrorKind::Backend)?); } Ok(Self { From c205d45bc29fd75ec0eebfe1538b33d65c04cc42 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 12:59:19 +0100 Subject: [PATCH 11/58] fix non-self-describing variable names that make it hard to read the code Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/backend/src/local.rs | 11 ++++++----- crates/core/src/backend/cache.rs | 25 +++++++++---------------- crates/core/src/backend/node.rs | 14 +++++++------- crates/core/src/commands/restore.rs | 2 +- 4 files changed, 23 insertions(+), 29 deletions(-) diff --git a/crates/backend/src/local.rs b/crates/backend/src/local.rs index 982dc5da..afc50d60 100644 --- a/crates/backend/src/local.rs +++ b/crates/backend/src/local.rs @@ -193,7 +193,7 @@ impl ReadBackend for LocalBackend { .into_iter() .filter_map(walkdir::Result::ok) .filter(|e| e.file_type().is_file()) - .map(|e| Id::from_hex(&e.file_name().to_string_lossy())) + .map(|entry| Id::from_hex(&entry.file_name().to_string_lossy())) .filter_map(std::result::Result::ok); Ok(walker.collect()) } @@ -235,11 +235,12 @@ impl ReadBackend for LocalBackend { let walker = WalkDir::new(path) .into_iter() .filter_map(walkdir::Result::ok) - .filter(|e| e.file_type().is_file()) - .map(|e| -> Result<_> { + .filter(|entry| entry.file_type().is_file()) + .map(|entry| -> Result<_> { Ok(( - Id::from_hex(&e.file_name().to_string_lossy())?, - e.metadata() + Id::from_hex(&entry.file_name().to_string_lossy())?, + entry + .metadata() .map_err(LocalBackendErrorKind::QueryingWalkDirMetadataFailed)? .len() .try_into() diff --git a/crates/core/src/backend/cache.rs b/crates/core/src/backend/cache.rs index 3b123200..9eafb64d 100644 --- a/crates/core/src/backend/cache.rs +++ b/crates/core/src/backend/cache.rs @@ -299,25 +299,18 @@ impl Cache { let walker = WalkDir::new(path) .into_iter() .filter_map(walkdir::Result::ok) - .filter(|e| { + .filter(|entry| { // only use files with length of 64 which are valid hex - e.file_type().is_file() - && e.file_name().len() == 64 - && e.file_name().is_ascii() - && e.file_name().to_str().is_some_and(|c| { - c.chars() - .all(|c| c.is_ascii_digit() || ('a'..='f').contains(&c)) + entry.file_type().is_file() + && entry.file_name().len() == 64 + && entry.file_name().is_ascii() + && entry.file_name().to_str().is_some_and(|c| { + c.chars().all(|character| { + character.is_ascii_digit() || ('a'..='f').contains(&character) + }) }) }) - .map(|e| { - ( - Id::from_hex(e.file_name().to_str().unwrap()).unwrap(), - // handle errors in metadata by returning a size of 0 - e.metadata().map_or(0, |m| m.len().try_into().unwrap_or(0)), - ) - }); - - Ok(walker.collect()) + .map(|entry| -> RusticResult<(Id, u32)> { } /// Removes all files from the cache that are not in the given list. diff --git a/crates/core/src/backend/node.rs b/crates/core/src/backend/node.rs index be44d7e0..7094916f 100644 --- a/crates/core/src/backend/node.rs +++ b/crates/core/src/backend/node.rs @@ -148,10 +148,10 @@ impl NodeType { pub fn to_link(&self) -> &Path { match self { Self::Symlink { - linktarget, - linktarget_raw, - } => linktarget_raw.as_ref().map_or_else( - || Path::new(linktarget), + link_target, + link_target_raw, + } => Ok(link_target_raw.as_ref().map_or_else( + || Path::new(link_target), |t| Path::new(OsStr::from_bytes(t)), ), _ => panic!("called method to_link on non-symlink!"), @@ -173,7 +173,7 @@ impl NodeType { #[must_use] pub fn to_link(&self) -> &Path { match self { - Self::Symlink { linktarget, .. } => Path::new(linktarget), + Self::Symlink { link_target, .. } => Ok(Path::new(link_target)), _ => panic!("called method to_link on non-symlink!"), } } @@ -346,11 +346,11 @@ pub fn last_modified_node(n1: &Node, n2: &Node) -> Ordering { fn escape_file_name(name: &OsStr) -> RusticResult { } -/// Unescape a filename +/// Unescape a file name /// /// # Arguments /// -/// * `s` - The escaped filename +/// * `s` - The escaped file name #[cfg(windows)] fn unescape_file_name(s: &str) -> Result { OsString::from_str(s) diff --git a/crates/core/src/commands/restore.rs b/crates/core/src/commands/restore.rs index c880cd5c..dce8b1e5 100644 --- a/crates/core/src/commands/restore.rs +++ b/crates/core/src/commands/restore.rs @@ -563,7 +563,7 @@ fn restore_contents( #[derive(Debug, Default)] pub struct RestorePlan { /// The names of the files to restore - names: Filenames, + names: FileNames, /// The length of the files to restore file_lengths: Vec, /// The restore information From 1ae7a826327be812412f326326929be015358cb4 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:09:35 +0100 Subject: [PATCH 12/58] add more documentation about errors etc Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/backend/src/opendal.rs | 18 ++++- crates/core/src/archiver/file_archiver.rs | 4 -- crates/core/src/archiver/tree_archiver.rs | 4 -- crates/core/src/backend.rs | 9 ++- crates/core/src/backend/node.rs | 36 ++++++---- crates/core/src/blob/packer.rs | 80 ++++++++++++++++++----- crates/core/src/blob/tree.rs | 22 +++++-- crates/core/src/commands/check.rs | 8 ++- crates/core/src/commands/forget.rs | 4 ++ crates/core/src/repofile/configfile.rs | 2 +- crates/core/src/repository.rs | 14 ++++ crates/core/src/vfs.rs | 13 ++-- crates/core/src/vfs/webdavfs.rs | 8 +++ 13 files changed, 174 insertions(+), 48 deletions(-) diff --git a/crates/backend/src/opendal.rs b/crates/backend/src/opendal.rs index 122691da..b3d54e4a 100644 --- a/crates/backend/src/opendal.rs +++ b/crates/backend/src/opendal.rs @@ -87,7 +87,7 @@ impl OpenDALBackend { /// # Returns /// /// The path for the given file type and id. - // Let's keep this for now, as it's being used in the trait implementations. + // INFO!: Let's keep this for now, as it's being used in the trait implementations. #[allow(clippy::unused_self)] fn path(&self, tpe: FileType, id: &Id) -> String { let hex_id = id.to_hex(); @@ -232,6 +232,14 @@ impl WriteBackend for OpenDALBackend { /// * `id` - The id of the file. /// * `cacheable` - Whether the file is cacheable. /// * `buf` - The bytes to write. + /// + /// # Errors + /// + /// If the file cannot be written, an error is returned. + /// + /// # Returns + /// + /// If the file is written successfully, `Ok(())` is returned. fn write_bytes(&self, tpe: FileType, id: &Id, _cacheable: bool, buf: Bytes) -> Result<()> { trace!("writing tpe: {:?}, id: {}", &tpe, &id); let file_name = self.path(tpe, id); @@ -246,6 +254,14 @@ impl WriteBackend for OpenDALBackend { /// * `tpe` - The type of the file. /// * `id` - The id of the file. /// * `cacheable` - Whether the file is cacheable. + /// + /// # Errors + /// + /// If the file does not exist, an error is returned. + /// + /// # Returns + /// + /// If the file is removed successfully, `Ok(())` is returned. fn remove(&self, tpe: FileType, id: &Id, _cacheable: bool) -> Result<()> { trace!("removing tpe: {:?}, id: {}", &tpe, &id); let file_name = self.path(tpe, id); diff --git a/crates/core/src/archiver/file_archiver.rs b/crates/core/src/archiver/file_archiver.rs index 4b0d96b7..1e32c954 100644 --- a/crates/core/src/archiver/file_archiver.rs +++ b/crates/core/src/archiver/file_archiver.rs @@ -168,10 +168,6 @@ impl<'a, BE: DecryptWriteBackend, I: ReadGlobalIndex> FileArchiver<'a, BE, I> { /// # Returns /// /// The statistics of the archiver. - /// - /// # Panics - /// - /// If the channel could not be dropped pub(crate) fn finalize(self) -> RusticResult { self.data_packer.finalize() } diff --git a/crates/core/src/archiver/tree_archiver.rs b/crates/core/src/archiver/tree_archiver.rs index 48e974dd..9969f477 100644 --- a/crates/core/src/archiver/tree_archiver.rs +++ b/crates/core/src/archiver/tree_archiver.rs @@ -212,10 +212,6 @@ impl<'a, BE: DecryptWriteBackend, I: ReadGlobalIndex> TreeArchiver<'a, BE, I> { /// /// A tuple containing the id of the tree and the summary of the snapshot. /// - /// # Panics - /// - /// If the channel of the tree packer is not dropped. - /// /// [`PackerErrorKind::SendingCrossbeamMessageFailed`]: crate::error::PackerErrorKind::SendingCrossbeamMessageFailed pub(crate) fn finalize( mut self, diff --git a/crates/core/src/backend.rs b/crates/core/src/backend.rs index 57431029..b1e7956b 100644 --- a/crates/core/src/backend.rs +++ b/crates/core/src/backend.rs @@ -83,7 +83,14 @@ impl FileType { /// This trait is implemented by all backends that can read data. pub trait ReadBackend: Send + Sync + 'static { /// Returns the location of the backend. - fn location(&self) -> String; + /// + /// # Errors + /// + /// If the location could not be determined. + /// + /// # Returns + /// + /// The location of the backend. /// Lists all files with their size of the given type. /// diff --git a/crates/core/src/backend/node.rs b/crates/core/src/backend/node.rs index 7094916f..7cb4d3ca 100644 --- a/crates/core/src/backend/node.rs +++ b/crates/core/src/backend/node.rs @@ -33,11 +33,11 @@ use crate::id::Id; #[derive(Default, Serialize, Deserialize, Clone, Debug, PartialEq, Eq, Constructor)] /// A node within the tree hierarchy pub struct Node { - /// Name of the node: filename or dirname. + /// Name of the node: file name or dirname. /// /// # Warning /// - /// This contains an escaped variant of the name in order to handle non-unicode filenames. + /// This contains an escaped variant of the name in order to handle non-unicode file names. /// Don't access this field directly, use the [`Node::name()`] method instead! pub name: String, #[serde(flatten)] @@ -140,9 +140,13 @@ impl NodeType { // Must be only called on NodeType::Symlink! /// Get the link path from a `NodeType::Symlink`. /// - /// # Panics + /// # Errors /// /// If called on a non-symlink node + /// + /// # Returns + /// + /// The link target as `Path` #[cfg(not(windows))] #[must_use] pub fn to_link(&self) -> &Path { @@ -160,14 +164,14 @@ impl NodeType { /// Convert a `NodeType::Symlink` to a `Path`. /// - /// # Warning + /// # Errors /// - /// Must be only called on `NodeType::Symlink`! + /// If called on a non-symlink node + /// If the link target is not valid unicode /// - /// # Panics + /// # Returns /// - /// * If called on a non-symlink node - /// * If the link target is not valid unicode + /// The link target as `Path` // TODO: Implement non-unicode link targets correctly for windows #[cfg(windows)] #[must_use] @@ -272,6 +276,10 @@ impl Node { /// * `node_type` - Type of the node /// * `meta` - Metadata of the node /// + /// # Errors + /// + /// If the name contains invalid characters + /// /// # Returns /// /// The created [`Node`] @@ -314,11 +322,15 @@ impl Node { } #[must_use] - /// Get the node name as `OsString`, handling name ecaping + /// Get the node name as `OsString`, handling name escaping /// /// # Panics /// - /// If the name is not valid unicode + /// If the name contains invalid characters + /// + /// # Returns + /// + /// The name of the node pub fn name(&self) -> OsString { unescape_file_name(&self.name).expect("unescaping should be infallible") } @@ -340,8 +352,8 @@ pub fn last_modified_node(n1: &Node, n2: &Node) -> Ordering { } // TODO: Should be probably called `_lossy` -// TODO(Windows): This is not able to handle non-unicode filenames and -// doesn't treat filenames which need and escape (like `\`, `"`, ...) correctly +// TODO(Windows): This is not able to handle non-unicode file names and +// doesn't treat file names which need and escape (like `\`, `"`, ...) correctly #[cfg(windows)] fn escape_file_name(name: &OsStr) -> RusticResult { } diff --git a/crates/core/src/blob/packer.rs b/crates/core/src/blob/packer.rs index a4996c1a..78ac9d54 100644 --- a/crates/core/src/blob/packer.rs +++ b/crates/core/src/blob/packer.rs @@ -110,9 +110,11 @@ impl PackSizer { /// # Arguments /// /// * `size` - The size to check - #[must_use] - pub fn size_ok(&self, size: u32) -> bool { - !self.is_too_small(size) && !self.is_too_large(size) + /// + /// # Returns + /// + /// `True` if the given size is not too small _and_ too large, + /// `False` otherwise } /// Evaluates whether the given size is too small @@ -290,7 +292,7 @@ impl Packer { /// /// # Errors /// - /// * [`PackerErrorKind::SendingCrossbeamMessageFailed`] - If sending the message to the raw packer fails. + /// * [`ChannelErrorKind::SendingCrossbeamMessageFailedWithBytes`] - If sending the message to the raw packer fails. /// /// [`PackerErrorKind::SendingCrossbeamMessageFailed`]: crate::error::PackerErrorKind::SendingCrossbeamMessageFailed fn add_with_sizelimit(&self, data: Bytes, id: Id, size_limit: Option) -> RusticResult<()> { @@ -334,9 +336,13 @@ impl Packer { /// Finalizes the packer and does cleanup /// - /// # Panics + /// # Errors + /// + /// [`MultiprocessingErrorKind::SenderDropped`] - If receiving the message from the raw packer fails. + /// + /// # Returns /// - /// If the channel could not be dropped + /// The packer stats pub fn finalize(self) -> RusticResult { // cancel channel drop(self.sender); @@ -371,12 +377,14 @@ impl PackerStats { /// * `summary` - The summary to add to /// * `tpe` - The blob type /// - /// # Panics + /// # Errors + /// + /// If the summary could not be updated + /// If the addition of the stats to the summary overflowed + /// + /// # Returns /// - /// If the blob type is invalid - pub fn apply(self, summary: &mut SnapshotSummary, tpe: BlobType) { - summary.data_added += self.data; - summary.data_added_packed += self.data_packed; + /// The updated summary match tpe { BlobType::Tree => { summary.tree_blobs += self.blobs; @@ -636,7 +644,19 @@ pub(crate) struct FileWriterHandle { } impl FileWriterHandle { - // TODO: add documentation + /// Processes the given data and writes it to the backend. + /// + /// # Arguments + /// + /// * `load` - The data to process. + /// + /// # Errors + /// + /// If writing the data to the backend fails. + /// + /// # Returns + /// + /// The index pack. fn process(&self, load: (Bytes, Id, IndexPack)) -> RusticResult { let (file, id, mut index) = load; index.id = id; @@ -673,6 +693,15 @@ impl Actor { /// * `fwh` - The file writer handle. /// * `queue_len` - The length of the queue. /// * `par` - The number of parallel threads. + /// + /// # Errors + /// + /// If sending the message to the actor fails. + /// + /// # Returns + /// + /// A new `Actor`. + #[allow(clippy::unnecessary_wraps)] fn new( fwh: FileWriterHandle, queue_len: usize, @@ -713,7 +742,11 @@ impl Actor { /// /// # Errors /// - /// If sending the message to the actor fails. + /// [`ChannelErrorKind::SendingCrossbeamMessageFailedForIndexPack`] - If sending the message to the actor fails. + /// + /// # Returns + /// + /// Ok if the message was sent successfully fn send(&self, load: (Bytes, IndexPack)) -> RusticResult<()> { self.sender .send(load) @@ -723,9 +756,13 @@ impl Actor { /// Finalizes the actor and does cleanup /// - /// # Panics + /// # Errors + /// + /// [`ChannelErrorKind::ReceivingCrossbeamMessageFailedForActorFinalizing`] - If receiving the message from the actor fails. + /// + /// # Returns /// - /// If the receiver is not present + /// Ok if the message was received successfully fn finalize(self) -> RusticResult<()> { // cancel channel drop(self.sender); @@ -849,6 +886,19 @@ impl Repacker { } } +/// Calculates the pack size. +/// +/// # Arguments +/// +/// * `current_size` - The current size of the pack file. +/// * `grow_factor` - The grow factor. +/// * `default_size` - The default size. +/// * `size_limit` - The size limit. +/// +/// # Returns +/// +/// The pack size. + #[cfg(test)] mod tests { diff --git a/crates/core/src/blob/tree.rs b/crates/core/src/blob/tree.rs index 8db9a081..95d1d324 100644 --- a/crates/core/src/blob/tree.rs +++ b/crates/core/src/blob/tree.rs @@ -209,7 +209,7 @@ pub struct TreeStreamerOptions { #[cfg_attr(feature = "clap", clap(long, help_heading = "Exclude options"))] pub glob: Vec, - /// Same as --glob pattern but ignores the casing of filenames + /// Same as --glob pattern but ignores the casing of file names #[cfg_attr( feature = "clap", clap(long, value_name = "GLOB", help_heading = "Exclude options") @@ -223,7 +223,7 @@ pub struct TreeStreamerOptions { )] pub glob_file: Vec, - /// Same as --glob-file ignores the casing of filenames in patterns + /// Same as --glob-file ignores the casing of file names in patterns #[cfg_attr( feature = "clap", clap(long, value_name = "FILE", help_heading = "Exclude options") @@ -245,16 +245,22 @@ where { /// The open iterators for subtrees open_iterators: Vec>, + /// Inner iterator for the current subtree nodes inner: std::vec::IntoIter, + /// The current path path: PathBuf, + /// The backend to read from be: BE, + /// index index: &'a I, + /// The glob overrides overrides: Option, + /// Whether to stream recursively recursive: bool, } @@ -323,6 +329,7 @@ where recursive, }) } + /// Creates a new `NodeStreamer` with glob patterns. /// /// # Arguments @@ -446,14 +453,19 @@ where pub struct TreeStreamerOnce

{ /// The visited tree IDs visited: BTreeSet, + /// The queue to send tree IDs to queue_in: Option>, + /// The queue to receive trees from queue_out: Receiver>, + /// The progress indicator p: P, + /// The number of trees that are not yet finished counter: Vec, + /// The number of finished trees finished_ids: usize, } @@ -536,9 +548,11 @@ impl TreeStreamerOnce

{ /// /// # Errors /// - /// * [`TreeErrorKind::SendingCrossbeamMessageFailed`] - If sending the message fails. + /// * [`MultiprocessingErrorKind::ReceiverDropped`] - If sending the message fails. + /// * [`MultiprocessingErrorKind::QueueInNotAvailable`] - If the queue is not available. /// - /// [`TreeErrorKind::SendingCrossbeamMessageFailed`]: crate::error::TreeErrorKind::SendingCrossbeamMessageFailed + /// [`MultiprocessingErrorKind::ReceiverDropped`]: crate::error::MultiprocessingErrorKind::ReceiverDropped + /// [`MultiprocessingErrorKind::QueueInNotAvailable`]: crate::error::MultiprocessingErrorKind::QueueInNotAvailable fn add_pending(&mut self, path: PathBuf, id: Id, count: usize) -> RusticResult { if self.visited.insert(id) { self.queue_in diff --git a/crates/core/src/commands/check.rs b/crates/core/src/commands/check.rs index 028f6d44..18f7f73c 100644 --- a/crates/core/src/commands/check.rs +++ b/crates/core/src/commands/check.rs @@ -366,6 +366,10 @@ fn check_packs_list(be: &impl ReadBackend, mut packs: HashMap) -> Rusti /// # Errors /// /// If a snapshot or tree is missing or has a different size +/// +/// # Returns +/// +/// Ok if everything is fine fn check_snapshots( be: &impl DecryptReadBackend, index: &impl ReadGlobalIndex, @@ -439,9 +443,9 @@ fn check_snapshots( /// /// If the pack is invalid /// -/// # Panics +/// # Returns /// -/// If zstd decompression fails. +/// Ok if the pack is valid fn check_pack( be: &impl DecryptReadBackend, index_pack: IndexPack, diff --git a/crates/core/src/commands/forget.rs b/crates/core/src/commands/forget.rs index fe1944e8..c2de82cd 100644 --- a/crates/core/src/commands/forget.rs +++ b/crates/core/src/commands/forget.rs @@ -523,6 +523,10 @@ impl KeepOptions { /// * `snapshots` - The list of snapshots to apply the options to /// * `now` - The current time /// + /// # Errors + /// + /// * If a given snapshot doesn't match the keep options + /// /// # Returns /// /// The list of snapshots with the attribute `keep` set to `true` if the snapshot should be kept and diff --git a/crates/core/src/repofile/configfile.rs b/crates/core/src/repofile/configfile.rs index 2c2ee6cc..580b7c20 100644 --- a/crates/core/src/repofile/configfile.rs +++ b/crates/core/src/repofile/configfile.rs @@ -156,7 +156,7 @@ impl ConfigFile { } } - /// Get wheter an extra verification (decompressing/decrypting data before writing to the repository) should be performed. + /// Get whether an extra verification (decompressing/decrypting data before writing to the repository) should be performed. #[must_use] pub fn extra_verify(&self) -> bool { self.extra_verify.unwrap_or(true) // default is to do the extra check diff --git a/crates/core/src/repository.rs b/crates/core/src/repository.rs index 59c6d8c6..0771d466 100644 --- a/crates/core/src/repository.rs +++ b/crates/core/src/repository.rs @@ -1275,6 +1275,20 @@ impl IndexedTree for Repository { pub(crate) struct BytesWeighter; impl quick_cache::Weighter for BytesWeighter { + /// Get the weight of a blob + /// + /// # Arguments + /// + /// * `key` - The key of the blob + /// * `val` - The blob + /// + /// # Panics + /// + /// If the weight of the blob is overflowing `u32::MAX` + /// + /// # Returns + /// + /// The weight of the blob fn weight(&self, _key: &Id, val: &Bytes) -> u32 { // Be cautions out about zero weights! u32::try_from(val.len().clamp(1, u32::MAX as usize)) diff --git a/crates/core/src/vfs.rs b/crates/core/src/vfs.rs index b5de11ca..8bfd5bfc 100644 --- a/crates/core/src/vfs.rs +++ b/crates/core/src/vfs.rs @@ -131,7 +131,8 @@ impl VfsTree { /// /// # Errors /// - // TODO: Document errors + /// * [`VfsErrorKind::NameDoesNotExist`] - if the component name doesn't exist + /// /// /// # Returns /// @@ -190,9 +191,9 @@ impl Vfs { /// /// * `node` - The directory [`Node`] to create the [`Vfs`] from /// - /// # Panics + /// # Returns /// - /// If the node is not a directory + /// The created [`Vfs`] or `None` if the node has no subtree #[must_use] pub fn from_dir_node(node: &Node) -> Self { let tree = VfsTree::RusticTree(node.subtree.unwrap()); @@ -347,6 +348,7 @@ impl Vfs { /// # Errors /// /// * [`VfsErrorKind::NameDoesNotExist`] - if the component name doesn't exist + /// * [`VfsErrorKind::NoDirectoryEntriesForSymlinkFound`] - if no directory entries for the symlink are found /// /// # Returns /// @@ -437,7 +439,10 @@ impl OpenFile { /// /// # Errors /// - // TODO: Document errors + /// * [`VfsErrorKind::DataBlobNotFound`] - If the data blob is not found + /// * [`VfsErrorKind::DataBlobTooLarge`] - If the data blob is too large + /// * [`VfsErrorKind::ConversionFromU32ToUsizeFailed`] - If the conversion from `u32` to `usize` failed + /// * [`VfsErrorKind::HexError`] - If the conversion from `Id` to `HexId` failed /// /// # Returns /// diff --git a/crates/core/src/vfs/webdavfs.rs b/crates/core/src/vfs/webdavfs.rs index 8c8dc44e..99db400b 100644 --- a/crates/core/src/vfs/webdavfs.rs +++ b/crates/core/src/vfs/webdavfs.rs @@ -281,6 +281,14 @@ impl DavFile for D .boxed() } + #[allow(clippy::expect_used)] + /// # Panics + /// + /// This function panics if the seek position is not representable as a `usize`. + /// Or if the seek position is greater than the file size. + /// Or if the seek position is greater than `usize::MAX`. + /// + /// In reality, these panics should not happen. fn seek(&mut self, pos: SeekFrom) -> FsFuture<'_, u64> { async move { match pos { From 7e0f0ca41ec53ba780055b103d10568bfdad721d Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:16:22 +0100 Subject: [PATCH 13/58] rename new_node to from_type_and_metadata to make the intent more clear and keep good style Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/archiver/tree.rs | 3 ++- crates/core/src/backend/ignore.rs | 20 ++++++++++---------- crates/core/src/backend/node.rs | 9 ++++++++- crates/core/src/backend/stdin.rs | 2 +- crates/core/src/blob/tree.rs | 3 ++- crates/core/src/vfs.rs | 2 +- 6 files changed, 24 insertions(+), 15 deletions(-) diff --git a/crates/core/src/archiver/tree.rs b/crates/core/src/archiver/tree.rs index cc5c4297..177113c0 100644 --- a/crates/core/src/archiver/tree.rs +++ b/crates/core/src/archiver/tree.rs @@ -99,7 +99,8 @@ where mode: Some(0o755), ..Default::default() }; - let node = Node::new_node(&p, NodeType::Dir, meta); + let node = + Node::from_type_and_metadata(&p, NodeType::Dir, meta).ok()?; return Some(TreeType::NewTree((self.path.clone(), node, p))); } } diff --git a/crates/core/src/backend/ignore.rs b/crates/core/src/backend/ignore.rs index fc46028e..9f7a9504 100644 --- a/crates/core/src/backend/ignore.rs +++ b/crates/core/src/backend/ignore.rs @@ -392,13 +392,13 @@ fn map_entry( }; let node = if m.is_dir() { - Node::new_node(name, NodeType::Dir, meta) + Node::from_type_and_metadata(name, NodeType::Dir, meta) } else if m.is_symlink() { let target = read_link(entry.path()).map_err(IgnoreErrorKind::FromIoError)?; let node_type = NodeType::from_link(&target); - Node::new_node(name, node_type, meta) + Node::from_type_and_metadata(name, node_type, meta) } else { - Node::new_node(name, NodeType::File, meta) + Node::from_type_and_metadata(name, NodeType::File, meta) }; let path = entry.into_path(); @@ -546,23 +546,23 @@ fn map_entry( let filetype = m.file_type(); let node = if m.is_dir() { - Node::new_node(name, NodeType::Dir, meta) + Node::from_type_and_metadata(name, NodeType::Dir, meta) } else if m.is_symlink() { let target = read_link(entry.path()).map_err(IgnoreErrorKind::FromIoError)?; let node_type = NodeType::from_link(&target); - Node::new_node(name, node_type, meta) + Node::from_type_and_metadata(name, node_type, meta) } else if filetype.is_block_device() { let node_type = NodeType::Dev { device: m.rdev() }; - Node::new_node(name, node_type, meta) + Node::from_type_and_metadata(name, node_type, meta) } else if filetype.is_char_device() { let node_type = NodeType::Chardev { device: m.rdev() }; - Node::new_node(name, node_type, meta) + Node::from_type_and_metadata(name, node_type, meta) } else if filetype.is_fifo() { - Node::new_node(name, NodeType::Fifo, meta) + Node::from_type_and_metadata(name, NodeType::Fifo, meta) } else if filetype.is_socket() { - Node::new_node(name, NodeType::Socket, meta) + Node::from_type_and_metadata(name, NodeType::Socket, meta) } else { - Node::new_node(name, NodeType::File, meta) + Node::from_type_and_metadata(name, NodeType::File, meta) }; let path = entry.into_path(); let open = Some(OpenFile(path.clone())); diff --git a/crates/core/src/backend/node.rs b/crates/core/src/backend/node.rs index 7cb4d3ca..305a232a 100644 --- a/crates/core/src/backend/node.rs +++ b/crates/core/src/backend/node.rs @@ -283,13 +283,20 @@ impl Node { /// # Returns /// /// The created [`Node`] + pub fn from_type_and_metadata( + name: &OsStr, + node_type: NodeType, + meta: Metadata, + ) -> RusticResult { + Ok(Self { name: escape_file_name(name)?, node_type, content: None, subtree: None, meta, - } + }) } + #[must_use] /// Evaluates if this node is a directory pub const fn is_dir(&self) -> bool { diff --git a/crates/core/src/backend/stdin.rs b/crates/core/src/backend/stdin.rs index 374d4f0e..cf254967 100644 --- a/crates/core/src/backend/stdin.rs +++ b/crates/core/src/backend/stdin.rs @@ -69,7 +69,7 @@ impl Iterator for StdinSource { Some(Ok(ReadSourceEntry { path: self.path.clone(), - node: Node::new_node( + node: Node::from_type_and_metadata( self.path.file_name().unwrap(), NodeType::File, Metadata::default(), diff --git a/crates/core/src/blob/tree.rs b/crates/core/src/blob/tree.rs index 95d1d324..7aa63343 100644 --- a/crates/core/src/blob/tree.rs +++ b/crates/core/src/blob/tree.rs @@ -139,7 +139,8 @@ impl Tree { id: Id, path: &Path, ) -> RusticResult { - let mut node = Node::new_node(OsStr::new(""), NodeType::Dir, Metadata::default()); + let mut node = + Node::from_type_and_metadata(OsStr::new(""), NodeType::Dir, Metadata::default())?; node.subtree = Some(id); for p in path.components() { diff --git a/crates/core/src/vfs.rs b/crates/core/src/vfs.rs index 8bfd5bfc..4ea2f3f8 100644 --- a/crates/core/src/vfs.rs +++ b/crates/core/src/vfs.rs @@ -382,7 +382,7 @@ impl Vfs { VfsTree::Link(target) => NodeType::from_link(Path::new(target)), _ => NodeType::Dir, }; - Node::new_node(name, node_type, Metadata::default()) + Node::from_type_and_metadata(name, node_type, Metadata::default()) }) .collect(), VfsPath::Link(str) => { From 0128764a61e01a46cf26708d086d254e573375c0 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:17:36 +0100 Subject: [PATCH 14/58] impl Display for Cache Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/cache.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/core/src/backend/cache.rs b/crates/core/src/backend/cache.rs index 9eafb64d..65b2afe1 100644 --- a/crates/core/src/backend/cache.rs +++ b/crates/core/src/backend/cache.rs @@ -1,5 +1,6 @@ use std::{ collections::HashMap, + fmt::{self, Display}, fs::{self, File}, io::{ErrorKind, Read, Seek, SeekFrom, Write}, path::PathBuf, @@ -208,6 +209,12 @@ pub struct Cache { path: PathBuf, } +impl Display for Cache { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.path.display()) + } +} + impl Cache { /// Creates a new [`Cache`] with the given id. /// From 896a14abc525b4b6a9cc6398c70c818c27e043af Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:18:55 +0100 Subject: [PATCH 15/58] remove default impl from nodetype as it can be derived Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/node.rs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/crates/core/src/backend/node.rs b/crates/core/src/backend/node.rs index 305a232a..73b07a1f 100644 --- a/crates/core/src/backend/node.rs +++ b/crates/core/src/backend/node.rs @@ -63,11 +63,12 @@ pub struct Node { } #[serde_as] -#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq)] +#[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, Default)] #[serde(tag = "type", rename_all = "lowercase")] /// Types a [`Node`] can have with type-specific additional information pub enum NodeType { /// Node is a regular file + #[default] File, /// Node is a directory Dir, @@ -183,12 +184,6 @@ impl NodeType { } } -impl Default for NodeType { - fn default() -> Self { - Self::File - } -} - /// Metadata of a [`Node`] #[serde_with::apply( Option => #[serde(default, skip_serializing_if = "Option::is_none")], From 5910c3bc990b92b2ac0ab7bad49d68cb2a100307 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:19:34 +0100 Subject: [PATCH 16/58] impl Debug for Packer Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/blob/packer.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/core/src/blob/packer.rs b/crates/core/src/blob/packer.rs index 78ac9d54..42ac56b1 100644 --- a/crates/core/src/blob/packer.rs +++ b/crates/core/src/blob/packer.rs @@ -178,6 +178,17 @@ pub struct Packer { finish: Receiver>, } +impl Debug for Packer { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("Packer") + .field("raw_packer", &self.raw_packer) + .field("indexer", &self.indexer) + .field("sender", &self.sender) + .field("finish", &self.finish) + .finish() + } +} + impl Packer { /// Creates a new `Packer`. /// From 3add91c572ff9e7699e17efc579689e6075cdaf5 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:21:34 +0100 Subject: [PATCH 17/58] factor out calculate_pack_size to make it easier testable Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/blob/packer.rs | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/crates/core/src/blob/packer.rs b/crates/core/src/blob/packer.rs index 42ac56b1..5aca7513 100644 --- a/crates/core/src/blob/packer.rs +++ b/crates/core/src/blob/packer.rs @@ -99,10 +99,18 @@ impl PackSizer { // `current_size` is `u64`, the maximum value is `2^64-1`. // `isqrt(2^64-1) = 2^32-1` which fits into a `u32`. (@aawsome) #[allow(clippy::cast_possible_truncation)] - pub fn pack_size(&self) -> u32 { - (self.current_size.integer_sqrt() as u32 * self.grow_factor + self.default_size) - .min(self.size_limit) - .min(constants::MAX_SIZE) + pub fn pack_size(&self) -> RusticResult { + Ok(calculate_pack_size( + u32::try_from(self.current_size).map_err(|err| { + PackerErrorKind::IntConversionFailedInPackSizeCalculation { + value: self.current_size, + comment: err.to_string(), + } + })?, + self.grow_factor, + self.default_size, + self.size_limit, + )) } /// Evaluates whether the given size is not too small or too large @@ -909,6 +917,17 @@ impl Repacker { /// # Returns /// /// The pack size. +#[must_use] +#[inline] +pub fn calculate_pack_size( + current_size: u32, + grow_factor: u32, + default_size: u32, + size_limit: u32, +) -> u32 { + current_size.integer_sqrt() * grow_factor + + default_size.min(size_limit).min(constants::MAX_SIZE) +} #[cfg(test)] mod tests { From 2f5d6ea9007eec0bf5e2cee95dab80e80bf3a55b Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:27:34 +0100 Subject: [PATCH 18/58] handle TreeArchiver::apply errors and refactor to make it safe against overflows Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/archiver.rs | 2 +- crates/core/src/archiver/tree_archiver.rs | 2 +- crates/core/src/blob/packer.rs | 91 +++++++++++++++++++++-- crates/core/src/commands/merge.rs | 2 +- 4 files changed, 88 insertions(+), 9 deletions(-) diff --git a/crates/core/src/archiver.rs b/crates/core/src/archiver.rs index 1c2696f7..67f6a4db 100644 --- a/crates/core/src/archiver.rs +++ b/crates/core/src/archiver.rs @@ -208,7 +208,7 @@ impl<'a, BE: DecryptFullBackend, I: ReadGlobalIndex> Archiver<'a, BE, I> { let stats = self.file_archiver.finalize()?; let (id, mut summary) = self.tree_archiver.finalize(self.parent.tree_id())?; - stats.apply(&mut summary, BlobType::Data); + stats.apply(&mut summary, BlobType::Data)?; self.snap.tree = id; self.indexer.write().finalize()?; diff --git a/crates/core/src/archiver/tree_archiver.rs b/crates/core/src/archiver/tree_archiver.rs index 9969f477..08d18046 100644 --- a/crates/core/src/archiver/tree_archiver.rs +++ b/crates/core/src/archiver/tree_archiver.rs @@ -220,7 +220,7 @@ impl<'a, BE: DecryptWriteBackend, I: ReadGlobalIndex> TreeArchiver<'a, BE, I> { let parent = parent_tree.map_or(ParentResult::NotFound, ParentResult::Matched); let id = self.backup_tree(&PathBuf::new(), &parent)?; let stats = self.tree_packer.finalize()?; - stats.apply(&mut self.summary, BlobType::Tree); + stats.apply(&mut self.summary, BlobType::Tree)?; Ok((id, self.summary)) } diff --git a/crates/core/src/blob/packer.rs b/crates/core/src/blob/packer.rs index 5aca7513..f838b95e 100644 --- a/crates/core/src/blob/packer.rs +++ b/crates/core/src/blob/packer.rs @@ -404,18 +404,97 @@ impl PackerStats { /// # Returns /// /// The updated summary + pub fn apply(self, summary: &mut SnapshotSummary, tpe: BlobType) -> RusticResult<()> { + let mut errors = vec![]; + if let Some(val) = summary.data_added.checked_add(self.data) { + summary.data_added = val; + } else { + warn!("data_added overflowed"); + errors.push(PackerErrorKind::DataAddedOverflowed { + data_added: summary.data_added, + data: self.data, + }); + } + if let Some(val) = summary.data_added_packed.checked_add(self.data_packed) { + summary.data_added_packed = val; + } else { + warn!("data_added_packed overflowed"); + errors.push(PackerErrorKind::DataAddedPackedOverflowed { + data_added_packed: summary.data_added_packed, + data_packed: self.data_packed, + }); + }; match tpe { BlobType::Tree => { - summary.tree_blobs += self.blobs; - summary.data_added_trees += self.data; - summary.data_added_trees_packed += self.data_packed; + if let Some(val) = summary.tree_blobs.checked_add(self.blobs) { + summary.tree_blobs = val; + } else { + warn!("tree_blobs overflowed"); + + errors.push(PackerErrorKind::TreeBlobsOverflowed { + tree_blobs: summary.tree_blobs, + blobs: self.blobs, + }); + } + if let Some(val) = summary.data_added_trees.checked_add(self.data) { + summary.data_added_trees = val; + } else { + warn!("data_added_trees overflowed"); + errors.push(PackerErrorKind::DataAddedTreesOverflowed { + data_added_trees: summary.data_added_trees, + data: self.data, + }); + } + if let Some(val) = summary + .data_added_trees_packed + .checked_add(self.data_packed) + { + summary.data_added_trees_packed = val; + } else { + warn!("data_added_trees_packed overflowed"); + errors.push(PackerErrorKind::DataAddedTreesPackedOverflowed { + data_added_trees_packed: summary.data_added_trees_packed, + data_packed: self.data_packed, + }); + }; } BlobType::Data => { - summary.data_blobs += self.blobs; - summary.data_added_files += self.data; - summary.data_added_files_packed += self.data_packed; + if let Some(val) = summary.data_blobs.checked_add(self.blobs) { + summary.data_blobs = val; + } else { + warn!("data_blobs overflowed"); + errors.push(PackerErrorKind::DataBlobsOverflowed { + data_blobs: summary.data_blobs, + blobs: self.blobs, + }); + }; + if let Some(val) = summary.data_added_files.checked_add(self.data) { + summary.data_added_files = val; + } else { + warn!("data_added_files overflowed"); + errors.push(PackerErrorKind::DataAddedFilesOverflowed { + data_added_files: summary.data_added_files, + data: self.data, + }); + }; + if let Some(val) = summary + .data_added_files_packed + .checked_add(self.data_packed) + { + summary.data_added_files_packed = val; + } else { + warn!("data_added_files_packed overflowed"); + errors.push(PackerErrorKind::DataAddedFilesPackedOverflowed { + data_added_files_packed: summary.data_added_files_packed, + data_packed: self.data_packed, + }); + }; } } + if !errors.is_empty() { + return Err(PackerErrorKind::MultipleFromSummary(errors).into()); + } + Ok(()) } } diff --git a/crates/core/src/commands/merge.rs b/crates/core/src/commands/merge.rs index 03489d63..7d2e6261 100644 --- a/crates/core/src/commands/merge.rs +++ b/crates/core/src/commands/merge.rs @@ -120,7 +120,7 @@ pub(crate) fn merge_trees( indexer.write().finalize()?; p.finish(); - stats.apply(summary, BlobType::Tree); + stats.apply(summary, BlobType::Tree)?; Ok(tree_merged) } From bf1419f3343224bb973d20aa2b60793c16ea1781 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:31:39 +0100 Subject: [PATCH 19/58] make link_target more consistent over the code base as well Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/node.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/core/src/backend/node.rs b/crates/core/src/backend/node.rs index 73b07a1f..106cdd57 100644 --- a/crates/core/src/backend/node.rs +++ b/crates/core/src/backend/node.rs @@ -80,13 +80,13 @@ pub enum NodeType { /// /// This contains the target only if it is a valid unicode target. /// Don't access this field directly, use the [`NodeType::to_link()`] method instead! - linktarget: String, + link_target: String, #[serde_as(as = "Option")] #[serde(default, skip_serializing_if = "Option::is_none")] /// The raw link target saved as bytes. /// /// This is only filled (and mandatory) if the link target is non-unicode. - linktarget_raw: Option>, + link_target_raw: Option>, }, /// Node is a block device file Dev { @@ -108,10 +108,10 @@ pub enum NodeType { impl NodeType { #[cfg(not(windows))] - /// Get a [`NodeType`] from a linktarget path + /// Get a [`NodeType`] from a link target path #[must_use] pub fn from_link(target: &Path) -> Self { - let (linktarget, linktarget_raw) = target.to_str().map_or_else( + let (link_target, link_target_raw) = target.to_str().map_or_else( || { ( target.as_os_str().to_string_lossy().to_string(), @@ -121,20 +121,20 @@ impl NodeType { |t| (t.to_string(), None), ); Self::Symlink { - linktarget, - linktarget_raw, + link_target, + link_target_raw, } } #[cfg(windows)] // Windows doesn't support non-unicode link targets, so we assume unicode here. // TODO: Test and check this! - /// Get a [`NodeType`] from a linktarget path + /// Get a [`NodeType`] from a link target path #[must_use] pub fn from_link(target: &Path) -> Self { Self::Symlink { - linktarget: target.as_os_str().to_string_lossy().to_string(), - linktarget_raw: None, + link_target: target.as_os_str().to_string_lossy().to_string(), + link_target_raw: None, } } From 8d2f432494e71706423183ca9c2cecf2f4cba7a2 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:32:06 +0100 Subject: [PATCH 20/58] use expect to unwrap runtime Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/backend/src/opendal.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/backend/src/opendal.rs b/crates/backend/src/opendal.rs index b3d54e4a..a16c4577 100644 --- a/crates/backend/src/opendal.rs +++ b/crates/backend/src/opendal.rs @@ -27,10 +27,11 @@ pub struct OpenDALBackend { fn runtime() -> &'static Runtime { static RUNTIME: OnceLock = OnceLock::new(); RUNTIME.get_or_init(|| { + #[allow(clippy::expect_used)] tokio::runtime::Builder::new_multi_thread() .enable_all() .build() - .unwrap() + .expect("Failed to create tokio runtime.") }) } From 1713a012787b3a35a0a0f1df9212b25eae7f331a Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:33:11 +0100 Subject: [PATCH 21/58] store joinhandle in rclone backend struct and join in Drop impl Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/backend/src/rclone.rs | 46 +++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 14 deletions(-) diff --git a/crates/backend/src/rclone.rs b/crates/backend/src/rclone.rs index 173ffcbf..24a3638b 100644 --- a/crates/backend/src/rclone.rs +++ b/crates/backend/src/rclone.rs @@ -7,7 +7,7 @@ use std::{ use anyhow::Result; use bytes::Bytes; -use log::{debug, info}; +use log::{debug, info, warn}; use rand::{ distributions::{Alphanumeric, DistString}, thread_rng, @@ -30,21 +30,33 @@ pub(super) mod constants { pub struct RcloneBackend { /// The REST backend. rest: RestBackend, + /// The url of the backend. url: String, + /// The child data contains the child process and is used to kill the child process when the backend is dropped. child: Child, + /// The [`JoinHandle`] of the thread printing rclone's output - handle: Option>, + handle: Option>>, } impl Drop for RcloneBackend { /// Kill the child process. fn drop(&mut self) { debug!("killing rclone."); - self.child.kill().unwrap(); - // TODO: Handle error and log it - _ = self.handle.take().map(JoinHandle::join); + if let Err(err) = self.child.kill() { + warn!("failed to kill rclone: {err}"); + }; + match self.handle.take().map(JoinHandle::join) { + Some(Err(err)) => { + warn!("rclone panicked: {err:?}"); + } + Some(Ok(Err(err))) => { + warn!("rclone failed: {err}"); + } + _ => (), + }; } } @@ -213,21 +225,27 @@ impl RcloneBackend { debug!("using REST backend with url {}.", url.as_ref()); let rest = RestBackend::new(rest_url, options)?; - let handle = Some(std::thread::spawn(move || loop { - let mut line = String::new(); - if stderr.read_line(&mut line).unwrap() == 0 { - break; - } - if !line.is_empty() { - info!("rclone output: {line}"); + let handle = std::thread::spawn(move || -> Result<()> { + loop { + let mut line = String::new(); + let line_length = stderr + .read_line(&mut line) + .map_err(RcloneErrorKind::FromIoError)?; + if line_length == 0 { + break; + } + if !line.is_empty() { + info!("rclone output: {line}"); + } } - })); + Ok(()) + }); Ok(Self { child, url: String::from(url.as_ref()), rest, - handle, + handle: Some(handle), }) } } From a75b4ae421c6e1ef272368a9f12f7dc4c8ce3c6e Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:33:54 +0100 Subject: [PATCH 22/58] warn the user if we shadow their already said rclone user and pass Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/backend/src/rclone.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/backend/src/rclone.rs b/crates/backend/src/rclone.rs index 24a3638b..3e530bf3 100644 --- a/crates/backend/src/rclone.rs +++ b/crates/backend/src/rclone.rs @@ -170,7 +170,11 @@ impl RcloneBackend { let mut command = Command::new(&rclone_command[0]); if use_password { - // TODO: We should handle errors here + // Check if RCLONE_USER and RCLONE_PASS are already set + if std::env::var("RCLONE_USER").is_ok() || std::env::var("RCLONE_PASS").is_ok() { + warn!("RCLONE_USER or RCLONE_PASS already set, we will overwrite them for this rclone instance."); + // return Err(RcloneErrorKind::RCloneUserOrPassAlreadySet.into()); + } _ = command .env("RCLONE_USER", &user) .env("RCLONE_PASS", &password); From 27a708a349bf4636b8c29269ba0586b10577692b Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:35:25 +0100 Subject: [PATCH 23/58] cleanup panic docs Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/backend/src/rclone.rs | 5 ----- crates/core/src/repository.rs | 4 ++-- crates/core/src/vfs.rs | 4 ---- 3 files changed, 2 insertions(+), 11 deletions(-) diff --git a/crates/backend/src/rclone.rs b/crates/backend/src/rclone.rs index 3e530bf3..ed327e75 100644 --- a/crates/backend/src/rclone.rs +++ b/crates/backend/src/rclone.rs @@ -133,11 +133,6 @@ impl RcloneBackend { /// # Returns /// /// The created [`RcloneBackend`]. - /// - /// # Panics - /// - /// If the rclone command is not found. - // TODO: This should be an error, not a panic. pub fn new(url: impl AsRef, options: HashMap) -> Result { let rclone_command = options.get("rclone-command"); let use_password = options diff --git a/crates/core/src/repository.rs b/crates/core/src/repository.rs index 0771d466..ec01240f 100644 --- a/crates/core/src/repository.rs +++ b/crates/core/src/repository.rs @@ -1014,9 +1014,9 @@ impl Repository { /// // TODO: Document errors /// - /// # Panics + /// # Returns /// - /// If the files could not be deleted. + /// Ok if the snapshots were removed successfully pub fn delete_snapshots(&self, ids: &[Id]) -> RusticResult<()> { if self.config().append_only == Some(true) { return Err(CommandErrorKind::NotAllowedWithAppendOnly( diff --git a/crates/core/src/vfs.rs b/crates/core/src/vfs.rs index 4ea2f3f8..cdf24266 100644 --- a/crates/core/src/vfs.rs +++ b/crates/core/src/vfs.rs @@ -356,10 +356,6 @@ impl Vfs { /// /// [`VfsErrorKind::NameDoesNotExist`]: crate::error::VfsErrorKind::NameDoesNotExist /// [`Tree`]: crate::repofile::Tree - /// - /// # Panics - /// - /// Panics if the path is not a directory. pub fn dir_entries_from_path( &self, repo: &Repository, From 35c8fef2ea7a880866bdc546c562390dfe02c218 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:36:13 +0100 Subject: [PATCH 24/58] allow expect for trait method impl Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/repository.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/core/src/repository.rs b/crates/core/src/repository.rs index ec01240f..4f5cdd4d 100644 --- a/crates/core/src/repository.rs +++ b/crates/core/src/repository.rs @@ -1290,7 +1290,9 @@ impl quick_cache::Weighter for BytesWeighter { /// /// The weight of the blob fn weight(&self, _key: &Id, val: &Bytes) -> u32 { - // Be cautions out about zero weights! + // Be cautious about zero weights! + #[allow(clippy::expect_used)] + // We allow this here, because we can't change the function signature to return a Result u32::try_from(val.len().clamp(1, u32::MAX as usize)) .expect("weight overflow in cache should not happen") } From 3d9ebb99a6981007592a4a97d010c52ab002806a Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:40:03 +0100 Subject: [PATCH 25/58] we allow expect here, because it should always give some and otherwise we would need to create a backoff::Error which is super annoying Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/backend/src/rest.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/crates/backend/src/rest.rs b/crates/backend/src/rest.rs index e892fed0..ddaacf76 100644 --- a/crates/backend/src/rest.rs +++ b/crates/backend/src/rest.rs @@ -4,7 +4,7 @@ use std::time::Duration; use anyhow::Result; use backoff::{backoff::Backoff, ExponentialBackoff, ExponentialBackoffBuilder}; use bytes::Bytes; -use log::{trace, warn}; +use log::{error, trace, warn}; use reqwest::{ blocking::{Client, ClientBuilder, Response}, header::{HeaderMap, HeaderValue}, @@ -437,12 +437,18 @@ impl WriteBackend for RestBackend { /// /// [`RestErrorKind::BackoffError`]: RestErrorKind::BackoffError fn write_bytes(&self, tpe: FileType, id: &Id, _cacheable: bool, buf: Bytes) -> Result<()> { - trace!("writing tpe: {:?}, id: {}", &tpe, &id); + trace!("writing tpe: {tpe:?}, id: {id}"); let req_builder = self.client.post(self.url(tpe, id)?).body(buf); + + #[allow(clippy::expect_used)] Ok(backoff::retry_notify( self.backoff.clone(), || { // Note: try_clone() always gives Some(_) as the body is Bytes which is clonable + _ = req_builder + .try_clone() + .expect("Should always give a value") + .send()? .validate()?; Ok(()) }, From d0b0255f19c9c5c2667d2b14aba5030fa9fc5639 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:42:20 +0100 Subject: [PATCH 26/58] handle errors in Cache::list_with_size Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/cache.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/crates/core/src/backend/cache.rs b/crates/core/src/backend/cache.rs index 65b2afe1..d1e1509f 100644 --- a/crates/core/src/backend/cache.rs +++ b/crates/core/src/backend/cache.rs @@ -3,7 +3,7 @@ use std::{ fmt::{self, Display}, fs::{self, File}, io::{ErrorKind, Read, Seek, SeekFrom, Write}, - path::PathBuf, + path::{Path, PathBuf}, sync::Arc, }; @@ -318,6 +318,25 @@ impl Cache { }) }) .map(|entry| -> RusticResult<(Id, u32)> { + let id = entry + .file_name() + .to_str() + .and_then(|str| Id::from_hex(str).ok()) + .ok_or_else(|| CacheBackendErrorKind::InvalidId)?; + + // handle errors in metadata by returning a size of 0 + let size = entry + .metadata() + .map(|metadata| u32::try_from(metadata.len()).ok())? + .ok_or_else(|| { + CacheBackendErrorKind::MetadataError(entry.path().to_path_buf()) + })?; + + Ok((id, size)) + }) + .filter_map(Result::ok) + .collect::<_>(); + Ok(walker) } /// Removes all files from the cache that are not in the given list. From 0439d61a3579d25947a7c3fae97de0126443e31f Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:43:29 +0100 Subject: [PATCH 27/58] bubble up options in TreeIterator Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/archiver/tree.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/core/src/archiver/tree.rs b/crates/core/src/archiver/tree.rs index 177113c0..391183e4 100644 --- a/crates/core/src/archiver/tree.rs +++ b/crates/core/src/archiver/tree.rs @@ -89,7 +89,7 @@ where // process next normal path component - other components are simply ignored if let Some(p) = comp_to_osstr(comp).ok().flatten() { if node.is_dir() && path == &self.path { - let (path, node, _) = self.item.take().unwrap(); + let (path, node, _) = self.item.take()?; self.item = self.iter.next(); let name = node.name(); return Some(TreeType::NewTree((path, node, name))); @@ -105,7 +105,7 @@ where } } // there wasn't any normal path component to process - return current item - let item = self.item.take().unwrap(); + let item = self.item.take()?; self.item = self.iter.next(); Some(TreeType::Other(item)) } From 034634b76ef329ea868f6484c7fc5b2bcc7e341e Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:51:38 +0100 Subject: [PATCH 28/58] add/remove docs, imports, and derives. derives e.g. serialize for insta test impl Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/archiver.rs | 3 ++- crates/core/src/backend.rs | 10 ++++++---- crates/core/src/backend/decrypt.rs | 9 +++++++-- crates/core/src/backend/ignore.rs | 4 ++-- crates/core/src/backend/node.rs | 11 +++++------ crates/core/src/blob/packer.rs | 11 +++++++---- crates/core/src/blob/tree.rs | 5 ++++- crates/core/src/cdc/rolling_hash.rs | 2 +- crates/core/src/commands/check.rs | 6 +++++- crates/core/src/commands/copy.rs | 2 +- crates/core/src/commands/forget.rs | 4 ++-- crates/core/src/commands/prune.rs | 16 ++++++++-------- crates/core/src/commands/restore.rs | 6 ++++-- crates/core/src/index/binarysorted.rs | 3 ++- crates/core/src/index/indexer.rs | 2 +- crates/core/src/lib.rs | 3 ++- crates/core/src/repofile/indexfile.rs | 2 +- 17 files changed, 60 insertions(+), 39 deletions(-) diff --git a/crates/core/src/archiver.rs b/crates/core/src/archiver.rs index 67f6a4db..3c8b25bc 100644 --- a/crates/core/src/archiver.rs +++ b/crates/core/src/archiver.rs @@ -6,7 +6,7 @@ pub(crate) mod tree_archiver; use std::path::{Path, PathBuf}; use chrono::Local; -use log::warn; +use log::{error, warn}; use pariter::{scope, IteratorExt}; use crate::{ @@ -16,6 +16,7 @@ use crate::{ }, backend::{decrypt::DecryptFullBackend, ReadSource, ReadSourceEntry}, blob::BlobType, + error::MultiprocessingErrorKind, index::{indexer::Indexer, indexer::SharedIndexer, ReadGlobalIndex}, repofile::{configfile::ConfigFile, snapshotfile::SnapshotFile}, Progress, RusticResult, diff --git a/crates/core/src/backend.rs b/crates/core/src/backend.rs index b1e7956b..5af0f3cd 100644 --- a/crates/core/src/backend.rs +++ b/crates/core/src/backend.rs @@ -15,10 +15,6 @@ use anyhow::Result; use bytes::Bytes; use enum_map::Enum; use log::trace; - -#[cfg(test)] -use mockall::mock; - use serde_derive::{Deserialize, Serialize}; use crate::{ @@ -28,6 +24,12 @@ use crate::{ RusticResult, }; +/// Backend to be used solely for testing. +pub(crate) mod in_memory; + +#[cfg(test)] +use mockall::mock; + /// All [`FileType`]s which are located in separated directories pub const ALL_FILE_TYPES: [FileType; 4] = [ FileType::Key, diff --git a/crates/core/src/backend/decrypt.rs b/crates/core/src/backend/decrypt.rs index 75cb5185..5ffb66bd 100644 --- a/crates/core/src/backend/decrypt.rs +++ b/crates/core/src/backend/decrypt.rs @@ -3,6 +3,7 @@ use std::{num::NonZeroU32, sync::Arc}; use anyhow::Result; use bytes::Bytes; use crossbeam_channel::{unbounded, Receiver}; +use log::error; use rayon::prelude::*; use zstd::stream::{copy_encode, decode_all, encode_all}; @@ -17,7 +18,7 @@ pub fn max_compression_level() -> i32 { use crate::{ backend::{FileType, ReadBackend, WriteBackend}, crypto::{hasher::hash, CryptoKey}, - error::{CryptBackendErrorKind, RusticErrorKind}, + error::{CryptBackendErrorKind, MultiprocessingErrorKind, RusticErrorKind}, id::Id, repofile::RepoFile, Progress, RusticResult, @@ -304,9 +305,13 @@ pub trait DecryptWriteBackend: WriteBackend + Clone + 'static { /// * `list` - The list of files to delete. /// * `p` - The progress bar. /// - /// # Panics + /// # Errors /// /// If the files could not be deleted. + /// + /// # Returns + /// + /// Ok if the files were deleted. fn delete_list<'a, I: ExactSizeIterator + Send>( &self, tpe: FileType, diff --git a/crates/core/src/backend/ignore.rs b/crates/core/src/backend/ignore.rs index 9f7a9504..4f0a0aae 100644 --- a/crates/core/src/backend/ignore.rs +++ b/crates/core/src/backend/ignore.rs @@ -72,7 +72,7 @@ pub struct LocalSourceFilterOptions { #[cfg_attr(feature = "merge", merge(strategy = merge::vec::overwrite_empty))] pub glob: Vec, - /// Same as --glob pattern but ignores the casing of filenames + /// Same as --glob pattern but ignores the casing of file names #[cfg_attr(feature = "clap", clap(long, value_name = "GLOB"))] #[cfg_attr(feature = "merge", merge(strategy = merge::vec::overwrite_empty))] pub iglob: Vec, @@ -82,7 +82,7 @@ pub struct LocalSourceFilterOptions { #[cfg_attr(feature = "merge", merge(strategy = merge::vec::overwrite_empty))] pub glob_file: Vec, - /// Same as --glob-file ignores the casing of filenames in patterns + /// Same as --glob-file ignores the casing of file names in patterns #[cfg_attr(feature = "clap", clap(long, value_name = "FILE"))] #[cfg_attr(feature = "merge", merge(strategy = merge::vec::overwrite_empty))] pub iglob_file: Vec, diff --git a/crates/core/src/backend/node.rs b/crates/core/src/backend/node.rs index 106cdd57..6115bd49 100644 --- a/crates/core/src/backend/node.rs +++ b/crates/core/src/backend/node.rs @@ -3,16 +3,16 @@ use std::{ ffi::{OsStr, OsString}, fmt::Debug, path::Path, - str::FromStr, }; +#[cfg(windows)] +use std::str::FromStr; + #[cfg(not(windows))] use std::fmt::Write; -#[cfg(not(windows))] -use std::os::unix::ffi::OsStrExt; #[cfg(not(windows))] -use crate::RusticResult; +use std::os::unix::ffi::OsStrExt; use chrono::{DateTime, Local}; use derive_more::Constructor; @@ -25,9 +25,8 @@ use serde_with::{ serde_as, DeserializeAs, SerializeAs, }; -#[cfg(not(windows))] use crate::error::NodeErrorKind; - +use crate::error::RusticResult; use crate::id::Id; #[derive(Default, Serialize, Deserialize, Clone, Debug, PartialEq, Eq, Constructor)] diff --git a/crates/core/src/blob/packer.rs b/crates/core/src/blob/packer.rs index f838b95e..58a54369 100644 --- a/crates/core/src/blob/packer.rs +++ b/crates/core/src/blob/packer.rs @@ -1,7 +1,8 @@ use integer_sqrt::IntegerSquareRoot; -use log::warn; +use log::{error, warn}; use std::{ + fmt::Debug, num::NonZeroU32, sync::Arc, time::{Duration, SystemTime}, @@ -21,7 +22,7 @@ use crate::{ }, blob::BlobType, crypto::{hasher::hash, CryptoKey}, - error::{PackerErrorKind, RusticErrorKind, RusticResult}, + error::{MultiprocessingErrorKind, PackerErrorKind, RusticErrorKind, RusticResult}, id::Id, index::indexer::SharedIndexer, repofile::{ @@ -94,7 +95,6 @@ impl PackSizer { } /// Computes the size of the pack file. - #[must_use] // The cast actually shouldn't pose any problems. // `current_size` is `u64`, the maximum value is `2^64-1`. // `isqrt(2^64-1) = 2^32-1` which fits into a `u32`. (@aawsome) @@ -503,7 +503,8 @@ impl PackerStats { /// # Type Parameters /// /// * `BE` - The backend type. -#[allow(missing_debug_implementations, clippy::module_name_repetitions)] +#[allow(clippy::module_name_repetitions)] +#[derive(Clone, Debug)] pub(crate) struct RawPacker { /// The backend to write to. be: BE, @@ -772,9 +773,11 @@ impl FileWriterHandle { } // TODO: add documentation +#[derive(Debug, Clone)] pub(crate) struct Actor { /// The sender to send blobs to the raw packer. sender: Sender<(Bytes, IndexPack)>, + /// The receiver to receive the status from the raw packer. finish: Receiver>, } diff --git a/crates/core/src/blob/tree.rs b/crates/core/src/blob/tree.rs index 7aa63343..a91db905 100644 --- a/crates/core/src/blob/tree.rs +++ b/crates/core/src/blob/tree.rs @@ -5,6 +5,7 @@ use std::{ mem, path::{Component, Path, PathBuf, Prefix}, str, + thread::JoinHandle, }; use crossbeam_channel::{bounded, unbounded, Receiver, Sender}; @@ -13,6 +14,7 @@ use derive_setters::Setters; use ignore::overrides::{Override, OverrideBuilder}; use ignore::Match; +use log::error; use serde::{Deserialize, Deserializer}; use serde_derive::Serialize; @@ -22,11 +24,12 @@ use crate::{ node::{Metadata, Node, NodeType}, }, crypto::hasher::hash, - error::{RusticResult, TreeErrorKind}, + error::{MultiprocessingErrorKind, RusticResult, TreeErrorKind}, id::Id, index::ReadGlobalIndex, progress::Progress, repofile::snapshotfile::SnapshotSummary, + RusticError, }; pub(super) mod constants { diff --git a/crates/core/src/cdc/rolling_hash.rs b/crates/core/src/cdc/rolling_hash.rs index 08ec97ee..81f67c72 100644 --- a/crates/core/src/cdc/rolling_hash.rs +++ b/crates/core/src/cdc/rolling_hash.rs @@ -35,7 +35,7 @@ pub(crate) trait RollingHash64 { } /// A rolling hash implementation for 64 bit polynoms from Rabin. -#[derive(Clone)] +#[derive(Clone, Debug)] pub(crate) struct Rabin64 { // Configuration /// Window size. diff --git a/crates/core/src/commands/check.rs b/crates/core/src/commands/check.rs index 18f7f73c..3ee35095 100644 --- a/crates/core/src/commands/check.rs +++ b/crates/core/src/commands/check.rs @@ -12,7 +12,7 @@ use crate::{ backend::{cache::Cache, decrypt::DecryptReadBackend, node::NodeType, FileType, ReadBackend}, blob::{tree::TreeStreamerOnce, BlobType}, crypto::hasher::hash, - error::{RusticErrorKind, RusticResult}, + error::{CheckErrorKind, RusticErrorKind, RusticResult}, id::Id, index::{ binarysorted::{IndexCollector, IndexType}, @@ -51,7 +51,11 @@ impl CheckOptions { /// /// # Errors /// + /// [`IndexStillInUse`] if the index is still in use + /// /// If the repository is corrupted + /// + /// [`IndexStillInUse`]: crate::error::IndexErrorKind::IndexStillInUse pub(crate) fn run(self, repo: &Repository) -> RusticResult<()> { let be = repo.dbe(); let cache = repo.cache(); diff --git a/crates/core/src/commands/copy.rs b/crates/core/src/commands/copy.rs index 1adfe29d..23d406fd 100644 --- a/crates/core/src/commands/copy.rs +++ b/crates/core/src/commands/copy.rs @@ -6,7 +6,7 @@ use rayon::prelude::{IntoParallelRefIterator, ParallelBridge, ParallelIterator}; use crate::{ backend::{decrypt::DecryptWriteBackend, node::NodeType}, blob::{packer::Packer, tree::TreeStreamerOnce, BlobType}, - error::RusticResult, + error::{CommandErrorKind, RusticResult}, index::{indexer::Indexer, ReadIndex}, progress::{Progress, ProgressBars}, repofile::SnapshotFile, diff --git a/crates/core/src/commands/forget.rs b/crates/core/src/commands/forget.rs index c2de82cd..4b15375e 100644 --- a/crates/core/src/commands/forget.rs +++ b/crates/core/src/commands/forget.rs @@ -7,7 +7,7 @@ use serde_derive::{Deserialize, Serialize}; use serde_with::{serde_as, DisplayFromStr}; use crate::{ - error::RusticResult, + error::{CommandErrorKind, RusticResult}, id::Id, progress::ProgressBars, repofile::{ @@ -530,7 +530,7 @@ impl KeepOptions { /// # Returns /// /// The list of snapshots with the attribute `keep` set to `true` if the snapshot should be kept and - /// `reasons` set to the list of reasons why the snapshot should be kept + /// `reasons` set to the list of reasons why the snapshot should be kept. pub fn apply( &self, mut snapshots: Vec, diff --git a/crates/core/src/commands/prune.rs b/crates/core/src/commands/prune.rs index 39a1e9e4..2e1ae603 100644 --- a/crates/core/src/commands/prune.rs +++ b/crates/core/src/commands/prune.rs @@ -327,7 +327,7 @@ impl DebugStats { } /// Statistics about what is deleted or kept within `prune` -#[derive(Default, Debug, Clone, Copy)] +#[derive(Default, Debug, Clone, Copy, Serialize)] pub struct DeleteStats { /// Number of blobs to remove pub remove: u64, @@ -343,7 +343,7 @@ impl DeleteStats { self.remove + self.recover + self.keep } } -#[derive(Debug, Default, Clone, Copy)] +#[derive(Debug, Default, Clone, Copy, Serialize)] /// Statistics about packs within `prune` pub struct PackStats { /// Number of used packs @@ -358,7 +358,7 @@ pub struct PackStats { pub keep: u64, } -#[derive(Debug, Default, Clone, Copy, Add)] +#[derive(Debug, Default, Clone, Copy, Add, Serialize)] /// Statistics about sizes within `prune` pub struct SizeStats { /// Number of used blobs @@ -391,7 +391,7 @@ impl SizeStats { } /// Statistics about a [`PrunePlan`] -#[derive(Default, Debug)] +#[derive(Default, Debug, Serialize)] pub struct PruneStats { /// Statistics about pack count pub packs_to_delete: DeleteStats, @@ -434,7 +434,7 @@ impl PruneStats { } // TODO: add documentation! -#[derive(Debug)] +#[derive(Debug, Serialize)] struct PruneIndex { /// The id of the index file id: Id, @@ -479,7 +479,7 @@ impl Default for PackToDo { } /// A pack which is to be pruned -#[derive(Debug)] +#[derive(Debug, Serialize)] struct PrunePack { /// The id of the pack id: Id, @@ -617,7 +617,7 @@ impl PrunePack { } /// Reasons why a pack should be repacked -#[derive(PartialEq, Eq, Debug)] +#[derive(PartialEq, Eq, Debug, Serialize)] enum RepackReason { /// The pack is partly used PartlyUsed, @@ -628,7 +628,7 @@ enum RepackReason { } /// A plan what should be repacked or removed by a `prune` run -#[derive(Debug)] +#[derive(Debug, Serialize)] pub struct PrunePlan { /// The time the plan was created time: DateTime, diff --git a/crates/core/src/commands/restore.rs b/crates/core/src/commands/restore.rs index dce8b1e5..f1273553 100644 --- a/crates/core/src/commands/restore.rs +++ b/crates/core/src/commands/restore.rs @@ -1,5 +1,6 @@ //! `restore` subcommand +use crossbeam_channel::unbounded; use derive_setters::Setters; use log::{debug, error, info, trace, warn}; @@ -14,7 +15,7 @@ use std::{ use chrono::{DateTime, Local, Utc}; use ignore::{DirEntry, WalkBuilder}; use itertools::Itertools; -use rayon::ThreadPoolBuilder; +use rayon::{Scope, ThreadPoolBuilder}; use crate::{ backend::{ @@ -24,10 +25,11 @@ use crate::{ FileType, ReadBackend, }, blob::BlobType, - error::{CommandErrorKind, RusticResult}, + error::{CommandErrorKind, IgnoreErrorKind, RusticErrorKind, RusticResult}, id::Id, progress::{Progress, ProgressBars}, repository::{IndexedFull, IndexedTree, Open, Repository}, + RusticError, }; pub(crate) mod constants { diff --git a/crates/core/src/index/binarysorted.rs b/crates/core/src/index/binarysorted.rs index 89ffbc39..0612fa2b 100644 --- a/crates/core/src/index/binarysorted.rs +++ b/crates/core/src/index/binarysorted.rs @@ -264,9 +264,10 @@ impl ReadIndex for Index { } #[cfg(test)] +#[allow(clippy::unwrap_used, clippy::expect_used)] mod tests { use super::*; - use crate::repofile::indexfile::IndexFile; + use crate::{repofile::indexfile::IndexFile, RusticResult}; const JSON_INDEX: &str = r#" {"packs":[{"id":"217f145b63fbc10267f5a686186689ea3389bed0d6a54b50ffc84d71f99eb7fa", diff --git a/crates/core/src/index/indexer.rs b/crates/core/src/index/indexer.rs index 0f2aaf47..52840816 100644 --- a/crates/core/src/index/indexer.rs +++ b/crates/core/src/index/indexer.rs @@ -27,7 +27,7 @@ pub(super) mod constants { pub(crate) type SharedIndexer = Arc>>; /// The `Indexer` is responsible for indexing blobs. -#[derive(Debug)] +#[derive(Debug, Clone)] pub struct Indexer where BE: DecryptWriteBackend, diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 71ddc293..59deb019 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -124,6 +124,7 @@ pub use crate::{ backend::{ decrypt::{compression_level_range, max_compression_level}, ignore::{LocalSource, LocalSourceFilterOptions, LocalSourceSaveOptions}, + in_memory::InMemoryBackend, local_destination::LocalDestination, node::last_modified_node, FileType, ReadBackend, ReadSource, ReadSourceEntry, ReadSourceOpen, RepositoryBackends, @@ -148,7 +149,7 @@ pub use crate::{ repofile::snapshotfile::{ PathList, SnapshotGroup, SnapshotGroupCriterion, SnapshotOptions, StringList, }, - repository::{IndexedFull, OpenStatus, Repository, RepositoryOptions}, + repository::{init_test_repository, IndexedFull, OpenStatus, Repository, RepositoryOptions}, }; #[cfg(feature = "merge")] diff --git a/crates/core/src/repofile/indexfile.rs b/crates/core/src/repofile/indexfile.rs index 5b73f979..d02277bb 100644 --- a/crates/core/src/repofile/indexfile.rs +++ b/crates/core/src/repofile/indexfile.rs @@ -14,7 +14,7 @@ use crate::{ /// Index files describe index information about multiple `pack` files. /// /// They are usually stored in the repository under `/index/` -#[derive(Serialize, Deserialize, Debug, Default)] +#[derive(Serialize, Deserialize, Debug, Default, Clone)] pub struct IndexFile { #[serde(skip_serializing_if = "Option::is_none")] /// which other index files are superseded by this (not actively used) From b70e8c071c85d32f6aaccff29ce1846640d99ab7 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:54:11 +0100 Subject: [PATCH 29/58] handle option in iter Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/ignore.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/core/src/backend/ignore.rs b/crates/core/src/backend/ignore.rs index 4f0a0aae..58131e51 100644 --- a/crates/core/src/backend/ignore.rs +++ b/crates/core/src/backend/ignore.rs @@ -305,7 +305,7 @@ impl Iterator for LocalSourceWalker { fn next(&mut self) -> Option { match self.walker.next() { // ignore root dir, i.e. an entry with depth 0 of type dir - Some(Ok(entry)) if entry.depth() == 0 && entry.file_type().unwrap().is_dir() => { + Some(Ok(entry)) if entry.depth() == 0 && entry.file_type()?.is_dir() => { self.walker.next() } item => item, From 63a6744f8b866b95e9bed19c905fb274f3e0c2d4 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:54:37 +0100 Subject: [PATCH 30/58] Don't panic on file not being able to be deleted. Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/decrypt.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/core/src/backend/decrypt.rs b/crates/core/src/backend/decrypt.rs index 5ffb66bd..0e6c113a 100644 --- a/crates/core/src/backend/decrypt.rs +++ b/crates/core/src/backend/decrypt.rs @@ -321,8 +321,8 @@ pub trait DecryptWriteBackend: WriteBackend + Clone + 'static { ) -> RusticResult<()> { p.set_length(list.len() as u64); list.par_bridge().try_for_each(|id| -> RusticResult<_> { - // TODO: Don't panic on file not being able to be deleted. - self.remove(tpe, id, cacheable).unwrap(); + self.remove(tpe, id, cacheable) + .map_err(RusticErrorKind::Backend)?; p.inc(1); Ok(()) })?; From 7ea2e37348751906519a211b19b41a5f6c6b345d Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:55:00 +0100 Subject: [PATCH 31/58] handle error in streamlist Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/decrypt.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/crates/core/src/backend/decrypt.rs b/crates/core/src/backend/decrypt.rs index 0e6c113a..5b5f9662 100644 --- a/crates/core/src/backend/decrypt.rs +++ b/crates/core/src/backend/decrypt.rs @@ -167,12 +167,18 @@ pub trait DecryptReadBackend: ReadBackend + Clone + 'static { p.set_length(list.len() as u64); let (tx, rx) = unbounded(); - list.into_par_iter() - .for_each_with((self, p, tx), |(be, p, tx), id| { + list.into_par_iter().try_for_each_with( + (self, p, tx), + |(be, p, tx), id| -> RusticResult<()> { let file = be.get_file::(&id).map(|file| (id, file)); p.inc(1); - tx.send(file).unwrap(); - }); + if tx.send(file).is_err() { + error!("receiver has been dropped unexpectedly."); + return Err(MultiprocessingErrorKind::ReceiverDropped.into()); + } + Ok(()) + }, + )?; Ok(rx) } } From efd19a4ea09af0d056c79c360a796d9ddd5eb8b9 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:55:32 +0100 Subject: [PATCH 32/58] handle option in gid and uid from name Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/local_destination.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/core/src/backend/local_destination.rs b/crates/core/src/backend/local_destination.rs index 285b214a..e26fc41e 100644 --- a/crates/core/src/backend/local_destination.rs +++ b/crates/core/src/backend/local_destination.rs @@ -44,14 +44,14 @@ pub struct LocalDestination { #[cfg(not(windows))] #[cached] fn uid_from_name(name: String) -> Option { - User::from_name(&name).unwrap().map(|u| u.uid) + User::from_name(&name).ok()?.map(|u| u.uid) } // Helper function to cache mapping group name -> gid #[cfg(not(windows))] #[cached] fn gid_from_name(name: String) -> Option { - Group::from_name(&name).unwrap().map(|g| g.gid) + Group::from_name(&name).ok()?.map(|g| g.gid) } impl LocalDestination { From 19516aac1fade958ec5a9b53f0d2d1044ccaab64 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:57:20 +0100 Subject: [PATCH 33/58] rename path to path of to make intent clear, because we pass in an item and otherwise it can be confused with a pure getter Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/local_destination.rs | 6 +++--- crates/core/src/commands/restore.rs | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/core/src/backend/local_destination.rs b/crates/core/src/backend/local_destination.rs index e26fc41e..f207f258 100644 --- a/crates/core/src/backend/local_destination.rs +++ b/crates/core/src/backend/local_destination.rs @@ -103,7 +103,7 @@ impl LocalDestination { /// /// * If the destination is a file, this will return the base path. /// * If the destination is a directory, this will return the base path joined with the item. - pub(crate) fn path(&self, item: impl AsRef) -> PathBuf { + pub(crate) fn path_of(&self, item: impl AsRef) -> PathBuf { if self.is_file { self.path.clone() } else { @@ -170,8 +170,8 @@ impl LocalDestination { /// /// [`LocalDestinationErrorKind::DirectoryCreationFailed`]: crate::error::LocalDestinationErrorKind::DirectoryCreationFailed pub fn create_dir(&self, item: impl AsRef) -> RusticResult<()> { - let dirname = self.path.join(item); - fs::create_dir_all(dirname).map_err(LocalDestinationErrorKind::DirectoryCreationFailed)?; + fs::create_dir_all(self.path_of(item)) + .map_err(LocalDestinationErrorKind::DirectoryCreationFailed)?; Ok(()) } diff --git a/crates/core/src/commands/restore.rs b/crates/core/src/commands/restore.rs index f1273553..aa5712d9 100644 --- a/crates/core/src/commands/restore.rs +++ b/crates/core/src/commands/restore.rs @@ -159,7 +159,7 @@ impl RestoreOptions { dry_run: bool, ) -> RusticResult { let p = repo.pb.progress_spinner("collecting file information..."); - let dest_path = dest.path(""); + let dest_path = dest.path_of(""); let mut stats = RestoreStats::default(); let mut restore_infos = RestorePlan::default(); @@ -285,7 +285,7 @@ impl RestoreOptions { next_dst = dst_iter.next(); } (Some(destination), Some((path, node))) => { - match destination.path().cmp(&dest.path(path)) { + match destination.path().cmp(&dest.path_of(path)) { Ordering::Less => { process_existing(destination)?; next_dst = dst_iter.next(); From 01db57165ad94796aec0f225c09a3786d0879d0a Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:58:01 +0100 Subject: [PATCH 34/58] use from Impl for readability Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/local_destination.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/core/src/backend/local_destination.rs b/crates/core/src/backend/local_destination.rs index f207f258..c42a3b4d 100644 --- a/crates/core/src/backend/local_destination.rs +++ b/crates/core/src/backend/local_destination.rs @@ -68,10 +68,9 @@ impl LocalDestination { /// * [`LocalDestinationErrorKind::DirectoryCreationFailed`] - If the directory could not be created. /// /// [`LocalDestinationErrorKind::DirectoryCreationFailed`]: crate::error::LocalDestinationErrorKind::DirectoryCreationFailed - // TODO: We should use `impl Into` here. we even use it in the body! pub fn new(path: &str, create: bool, expect_file: bool) -> RusticResult { let is_dir = path.ends_with('/'); - let path: PathBuf = path.into(); + let path = PathBuf::from(path); let is_file = path.is_file() || (!path.is_dir() && !is_dir && expect_file); if create { From d1896bd7e04bf7dea0cb991692b149b1b65891cd Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 13:59:07 +0100 Subject: [PATCH 35/58] add serde feature for enum-map for insta tests, uncomment lints and cleanup, add cfg_if for tests and os dependent stuff Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- Cargo.toml | 7 ++++--- crates/core/Cargo.toml | 1 + 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 0893b2fb..c3b42cee 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,6 +15,7 @@ rust-version = "1.72.1" aho-corasick = "1.1.2" anyhow = "1.0.81" bytes = "1.5.0" +enum-map = { version = "2.7.3", features = ["serde"] } parking_lot = { version = "0.12.1", features = ["deadlock_detection", "arc_lock"] } rustic_backend = { path = "crates/backend" } rustic_core = { path = "crates/core" } @@ -74,7 +75,7 @@ trivial_casts = "warn" unused_lifetimes = "warn" unused_qualifications = "warn" bad_style = "warn" -dead_code = "allow" # TODO: "warn" +dead_code = "warn" improper_ctypes = "warn" missing_copy_implementations = "warn" missing_debug_implementations = "warn" @@ -99,8 +100,8 @@ unreachable_pub = "allow" redundant_pub_crate = "allow" pedantic = "warn" nursery = "warn" -# expect_used = "warn" # TODO! -# unwrap_used = "warn" # TODO! +expect_used = "warn" +unwrap_used = "warn" enum_glob_use = "warn" correctness = "warn" suspicious = "warn" diff --git a/crates/core/Cargo.toml b/crates/core/Cargo.toml index f2e3154c..2c9223e8 100644 --- a/crates/core/Cargo.toml +++ b/crates/core/Cargo.toml @@ -106,6 +106,7 @@ runtime-format = "0.1.3" anyhow = { workspace = true } bytes = { workspace = true } bytesize = "1.3.0" +cfg-if = "1.0.0" chrono = { version = "0.4.35", default-features = false, features = ["clock", "serde"] } enum-map = { workspace = true } enum-map-derive = "0.17.0" From 37e72be42b28420ebeb9e47ab033b445bb9ab65e Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:00:24 +0100 Subject: [PATCH 36/58] handle err in ignore + node Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/ignore.rs | 18 ++++++++++++++---- crates/core/src/backend/node.rs | 31 ++++++++++++++++++++----------- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/crates/core/src/backend/ignore.rs b/crates/core/src/backend/ignore.rs index 58131e51..bcaf6169 100644 --- a/crates/core/src/backend/ignore.rs +++ b/crates/core/src/backend/ignore.rs @@ -403,7 +403,11 @@ fn map_entry( let path = entry.into_path(); let open = Some(OpenFile(path.clone())); - Ok(ReadSourceEntry { path, node, open }) + Ok(ReadSourceEntry { + path, + node: node?, + open, + }) } /// Get the user name for the given uid. @@ -520,9 +524,11 @@ fn map_entry( .map(|name| { Ok(ExtendedAttribute { name: name.to_string_lossy().to_string(), - value: xattr::get(path, name) + value: xattr::get(path, name.clone()) .map_err(IgnoreErrorKind::FromIoError)? - .unwrap(), + .ok_or({ + IgnoreErrorKind::XattrNotFound(name.to_string_lossy().to_string()) + })?, }) }) .collect::>()? @@ -566,7 +572,11 @@ fn map_entry( }; let path = entry.into_path(); let open = Some(OpenFile(path.clone())); - Ok(ReadSourceEntry { path, node, open }) + Ok(ReadSourceEntry { + path, + node: node?, + open, + }) } #[cfg(not(windows))] diff --git a/crates/core/src/backend/node.rs b/crates/core/src/backend/node.rs index 6115bd49..1f9e8cb1 100644 --- a/crates/core/src/backend/node.rs +++ b/crates/core/src/backend/node.rs @@ -148,8 +148,7 @@ impl NodeType { /// /// The link target as `Path` #[cfg(not(windows))] - #[must_use] - pub fn to_link(&self) -> &Path { + pub fn to_link(&self) -> RusticResult<&Path> { match self { Self::Symlink { link_target, @@ -157,8 +156,8 @@ impl NodeType { } => Ok(link_target_raw.as_ref().map_or_else( || Path::new(link_target), |t| Path::new(OsStr::from_bytes(t)), - ), - _ => panic!("called method to_link on non-symlink!"), + )), + _ => Err(NodeErrorKind::InvalidLinkTarget.into()), } } @@ -174,11 +173,10 @@ impl NodeType { /// The link target as `Path` // TODO: Implement non-unicode link targets correctly for windows #[cfg(windows)] - #[must_use] - pub fn to_link(&self) -> &Path { + pub fn to_link(&self) -> RusticResult<&Path> { match self { Self::Symlink { link_target, .. } => Ok(Path::new(link_target)), - _ => panic!("called method to_link on non-symlink!"), + _ => Err(NodeErrorKind::InvalidLinkTarget.into()), } } } @@ -333,6 +331,7 @@ impl Node { /// /// The name of the node pub fn name(&self) -> OsString { + #[allow(clippy::expect_used)] unescape_file_name(&self.name).expect("unescaping should be infallible") } } @@ -357,6 +356,10 @@ pub fn last_modified_node(n1: &Node, n2: &Node) -> Ordering { // doesn't treat file names which need and escape (like `\`, `"`, ...) correctly #[cfg(windows)] fn escape_file_name(name: &OsStr) -> RusticResult { + Ok(name + .to_str() + .ok_or_else(|| NodeErrorKind::InvalidFileName(name.to_os_string()))? + .to_string()) } /// Unescape a file name @@ -408,23 +411,29 @@ fn escape_file_name(name: &OsStr) -> RusticResult { } Err(error) => { let (valid, after_valid) = input.split_at(error.valid_up_to()); - push(&mut s, std::str::from_utf8(valid).unwrap()); + push( + &mut s, + std::str::from_utf8(valid) + .map_err(|e| NodeErrorKind::FromUtf8Error(e.to_string()))?, + ); if let Some(invalid_sequence_length) = error.error_len() { for b in &after_valid[..invalid_sequence_length] { - write!(s, "\\x{b:02x}").unwrap(); + write!(s, "\\x{b:02x}") + .map_err(|e| NodeErrorKind::SignWriteError(e.to_string()))?; } input = &after_valid[invalid_sequence_length..]; } else { for b in after_valid { - write!(s, "\\x{b:02x}").unwrap(); + write!(s, "\\x{b:02x}") + .map_err(|e| NodeErrorKind::SignWriteError(e.to_string()))?; } break; } } } } - s + Ok(s) } #[cfg(not(windows))] From 3b93861f2fd9f901e1695f5e25df0c6bf1034e7a Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:00:59 +0100 Subject: [PATCH 37/58] differentiate between seen and successfully setting attribs Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/local_destination.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/crates/core/src/backend/local_destination.rs b/crates/core/src/backend/local_destination.rs index c42a3b4d..209c28b7 100644 --- a/crates/core/src/backend/local_destination.rs +++ b/crates/core/src/backend/local_destination.rs @@ -388,6 +388,8 @@ impl LocalDestination { extended_attributes: &[ExtendedAttribute], ) -> RusticResult<()> { let file_name = self.path_of(item); + let mut successful = vec![false; extended_attributes.len()]; + let mut seen = vec![false; extended_attributes.len()]; for curr_name in xattr::list(&file_name) .map_err(|err| LocalDestinationErrorKind::ListingXattrsFailed(err, file_name.clone()))? @@ -411,6 +413,14 @@ impl LocalDestination { source: err, } })?; + } + // We have seen this xattr, but we haven't changed anything + seen[index] = true; + // and we changed something + successful[index] = true; + } + // We have seen this xattr, but we haven't changed anything + seen[index] = true; } None => { if let Err(err) = xattr::remove(&file_name, &curr_name) { From c082ea1878e6d85658e2209b20024a267acda475 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:01:13 +0100 Subject: [PATCH 38/58] handle err in stdin Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend/stdin.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/core/src/backend/stdin.rs b/crates/core/src/backend/stdin.rs index cf254967..165d971c 100644 --- a/crates/core/src/backend/stdin.rs +++ b/crates/core/src/backend/stdin.rs @@ -70,10 +70,11 @@ impl Iterator for StdinSource { Some(Ok(ReadSourceEntry { path: self.path.clone(), node: Node::from_type_and_metadata( - self.path.file_name().unwrap(), + self.path.file_name()?, NodeType::File, Metadata::default(), - ), + ) + .ok()?, open: Some(OpenStdin()), })) } From ab9905179cc623aacddb9291cc7fcb2c79635748 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:02:37 +0100 Subject: [PATCH 39/58] handle errors in size calc in packer + deal with overflows Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/blob/packer.rs | 58 ++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 16 deletions(-) diff --git a/crates/core/src/blob/packer.rs b/crates/core/src/blob/packer.rs index 58a54369..2c288530 100644 --- a/crates/core/src/blob/packer.rs +++ b/crates/core/src/blob/packer.rs @@ -123,6 +123,8 @@ impl PackSizer { /// /// `True` if the given size is not too small _and_ too large, /// `False` otherwise + pub fn size_ok(&self, size: u32) -> RusticResult { + Ok(!self.is_too_small(size)? && !self.is_too_large(size)?) } /// Evaluates whether the given size is too small @@ -130,12 +132,20 @@ impl PackSizer { /// # Arguments /// /// * `size` - The size to check - #[must_use] - pub fn is_too_small(&self, size: u32) -> bool { - let target_size = self.pack_size(); + pub fn is_too_small(&self, size: u32) -> RusticResult { + let target_size = self.pack_size()?; // Note: we cast to u64 so that no overflow can occur in the multiplications - u64::from(size) * 100 - < u64::from(target_size) * u64::from(self.min_packsize_tolerate_percent) + Ok(u64::from(size).checked_mul(100).ok_or( + PackerErrorKind::IntConversionFailedInPackSizeCalculation { + value: u64::from(size), + comment: "overflow".into(), + }, + )? < u64::from(target_size) + .checked_mul(u64::from(self.min_packsize_tolerate_percent)) + .ok_or(PackerErrorKind::IntConversionFailedInPackSizeCalculation { + value: u64::from(target_size), + comment: "overflow".into(), + })?) } /// Evaluates whether the given size is too large @@ -143,12 +153,20 @@ impl PackSizer { /// # Arguments /// /// * `size` - The size to check - #[must_use] - pub fn is_too_large(&self, size: u32) -> bool { - let target_size = self.pack_size(); + pub fn is_too_large(&self, size: u32) -> RusticResult { + let target_size = self.pack_size()?; // Note: we cast to u64 so that no overflow can occur in the multiplications - u64::from(size) * 100 - > u64::from(target_size) * u64::from(self.max_packsize_tolerate_percent) + Ok(u64::from(size).checked_mul(100).ok_or( + PackerErrorKind::IntConversionFailedInPackSizeCalculation { + value: u64::from(size), + comment: "overflow".into(), + }, + )? > u64::from(target_size) + .checked_mul(u64::from(self.max_packsize_tolerate_percent)) + .ok_or(PackerErrorKind::IntConversionFailedInPackSizeCalculation { + value: u64::from(target_size), + comment: "overflow".into(), + })?) } /// Adds the given size to the current size. @@ -156,12 +174,20 @@ impl PackSizer { /// # Arguments /// /// * `added` - The size to add - /// - /// # Panics - /// - /// If the size is too large - fn add_size(&mut self, added: u32) { - self.current_size += u64::from(added); + fn add_size(&mut self, added: u32) -> RusticResult<()> { + if added > self.size_limit { + return Err(PackerErrorKind::SizeLimitExceeded(added).into()); + } + if let Some(value) = self.current_size.checked_add(u64::from(added)) { + self.current_size = value; + } else { + return Err(PackerErrorKind::AddingSizeToCurrentSizeFailed { + current_size: self.current_size, + to_be_added: added, + } + .into()); + } + Ok(()) } } From 3bae8c5e4bb4e0abdb467c38a2818a46f5264188 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:04:04 +0100 Subject: [PATCH 40/58] multiprocessing errs Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/blob/packer.rs | 108 ++++++++++++++++++++++++--------- 1 file changed, 79 insertions(+), 29 deletions(-) diff --git a/crates/core/src/blob/packer.rs b/crates/core/src/blob/packer.rs index 2c288530..4dc20bd5 100644 --- a/crates/core/src/blob/packer.rs +++ b/crates/core/src/blob/packer.rs @@ -259,7 +259,7 @@ impl Packer { indexer.clone(), config, total_size, - ))); + )?)); let (tx, rx) = bounded(0); let (finish_tx, finish_rx) = bounded::>(0); @@ -270,8 +270,9 @@ impl Packer { finish: finish_rx, }; - let _join_handle = std::thread::spawn(move || { - scope(|scope| { + // TODO!: store join handle? + let _join_handle = std::thread::spawn(move || -> RusticResult<()> { + let scoped_work = scope(|scope| -> RusticResult<()> { let status = rx .into_iter() .readahead_scoped(scope) @@ -296,6 +297,7 @@ impl Packer { // check again if id is already contained // TODO: We may still save duplicate blobs - the indexer is only updated when the packfile write has completed .filter(|res| { + res.as_ref() .map_or_else(|_| true, |(_, id, _, _, _)| !indexer.read().has(id)) }) .try_for_each(|item: RusticResult<_>| { @@ -305,6 +307,19 @@ impl Packer { .add_raw(&data, &id, data_len, ul, size_limit) }) .and_then(|()| raw_packer.write().finalize()); + if finish_tx.send(status).is_err() { + error!("receiver has been dropped unexpectedly."); + return Err(MultiprocessingErrorKind::ReceiverDropped.into()); + } + Ok(()) + }); + if scoped_work.is_err() { + Err(MultiprocessingErrorKind::SendingCrossbeamMessageFailed.into()) + } else if let Ok(Err(err)) = scoped_work { + Err(err) + } else { + Ok(()) + } }); Ok(packer) @@ -341,9 +356,10 @@ impl Packer { /// /// [`PackerErrorKind::SendingCrossbeamMessageFailed`]: crate::error::PackerErrorKind::SendingCrossbeamMessageFailed fn add_with_sizelimit(&self, data: Bytes, id: Id, size_limit: Option) -> RusticResult<()> { - self.sender - .send((data, id, size_limit)) - .map_err(PackerErrorKind::SendingCrossbeamMessageFailed)?; + if self.sender.send((data, id, size_limit)).is_err() { + error!("receiver has been dropped unexpectedly."); + return Err(MultiprocessingErrorKind::ReceiverDropped.into()); + } Ok(()) } @@ -574,7 +590,7 @@ impl RawPacker { indexer: SharedIndexer, config: &ConfigFile, total_size: u64, - ) -> Self { + ) -> RusticResult { let file_writer = Some(Actor::new( FileWriterHandle { be: be.clone(), @@ -583,11 +599,11 @@ impl RawPacker { }, 1, 1, - )); + )?); let pack_sizer = PackSizer::from_config(config, blob_type, total_size); - Self { + Ok(Self { be, blob_type, file: BytesMut::new(), @@ -598,7 +614,7 @@ impl RawPacker { file_writer, pack_sizer, stats: PackerStats::default(), - } + }) } /// Saves the packfile and returns the stats @@ -608,7 +624,10 @@ impl RawPacker { /// If the packfile could not be saved fn finalize(&mut self) -> RusticResult { self.save()?; - self.file_writer.take().unwrap().finalize()?; + self.file_writer + .take() + .ok_or(PackerErrorKind::ActorHandleNotPresent)? + .finalize()?; Ok(std::mem::take(&mut self.stats)) } @@ -665,7 +684,11 @@ impl RawPacker { .map_err(PackerErrorKind::IntConversionFailed)?; self.stats.data_packed += data_len_packed; - let size_limit = size_limit.unwrap_or_else(|| self.pack_sizer.pack_size()); + let size_limit = if let Some(size_limit) = size_limit { + size_limit + } else { + self.pack_sizer.pack_size()? + }; let offset = self.size; let len = self.write_data(data)?; self.index @@ -681,7 +704,7 @@ impl RawPacker { || self.size >= size_limit || elapsed >= constants::MAX_AGE { - self.pack_sizer.add_size(self.index.pack_size()); + self.pack_sizer.add_size(self.index.pack_size())?; self.save()?; self.size = 0; self.count = 0; @@ -741,10 +764,16 @@ impl RawPacker { // write file to backend let index = std::mem::take(&mut self.index); let file = std::mem::replace(&mut self.file, BytesMut::new()); - self.file_writer + if self + .file_writer .as_ref() - .unwrap() - .send((file.into(), index))?; + .ok_or(PackerErrorKind::FileWriterHandleNotPresent)? + .send((file.into(), index)) + .is_err() + { + error!("receiver has been dropped unexpectedly."); + return Err(MultiprocessingErrorKind::ReceiverDropped.into()); + } Ok(()) } @@ -833,12 +862,13 @@ impl Actor { fwh: FileWriterHandle, queue_len: usize, _par: usize, - ) -> Self { + ) -> RusticResult { let (tx, rx) = bounded(queue_len); let (finish_tx, finish_rx) = bounded::>(0); - let _join_handle = std::thread::spawn(move || { - scope(|scope| { + // TODO!: store join handle? + let _join_handle = std::thread::spawn(move || -> RusticResult<()> { + let scoped_work = scope(|scope| -> RusticResult<()> { let status = rx .into_iter() .readahead_scoped(scope) @@ -850,15 +880,28 @@ impl Actor { .map(|load| fwh.process(load)) .readahead_scoped(scope) .try_for_each(|index| fwh.index(index?)); - _ = finish_tx.send(status); - }) - .unwrap(); + + if finish_tx.send(status).is_err() { + error!("receiver has been dropped unexpectedly."); + return Err(MultiprocessingErrorKind::ReceiverDropped.into()); + }; + + Ok(()) + }); + + if scoped_work.is_err() { + Err(MultiprocessingErrorKind::SendingCrossbeamMessageFailed.into()) + } else if let Ok(Err(err)) = scoped_work { + Err(err) + } else { + Ok(()) + } }); - Self { + Ok(Self { sender: tx, finish: finish_rx, - } + }) } /// Sends the given data to the actor. @@ -875,9 +918,11 @@ impl Actor { /// /// Ok if the message was sent successfully fn send(&self, load: (Bytes, IndexPack)) -> RusticResult<()> { - self.sender - .send(load) - .map_err(PackerErrorKind::SendingCrossbeamMessageFailedForIndexPack)?; + if self.sender.send(load).is_err() { + error!("receiver has been dropped unexpectedly."); + return Err(MultiprocessingErrorKind::ReceiverDropped.into()); + } + Ok(()) } @@ -893,8 +938,13 @@ impl Actor { fn finalize(self) -> RusticResult<()> { // cancel channel drop(self.sender); + // wait for items in channel to be processed - self.finish.recv().unwrap() + if self.finish.recv().is_err() { + error!("sender has been dropped unexpectedly."); + return Err(MultiprocessingErrorKind::SenderDropped.into()); + } + Ok(()) } } @@ -942,7 +992,7 @@ impl Repacker { total_size: u64, ) -> RusticResult { let packer = Packer::new(be.clone(), blob_type, indexer, config, total_size)?; - let size_limit = PackSizer::from_config(config, blob_type, total_size).pack_size(); + let size_limit = PackSizer::from_config(config, blob_type, total_size).pack_size()?; Ok(Self { be, packer, From 2e1d12da444e6d145020149265fa8cf0962db300 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:04:22 +0100 Subject: [PATCH 41/58] multiprocessing errs Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/blob/tree.rs | 85 ++++++++++++++++++++++-------------- 1 file changed, 52 insertions(+), 33 deletions(-) diff --git a/crates/core/src/blob/tree.rs b/crates/core/src/blob/tree.rs index a91db905..9ea71116 100644 --- a/crates/core/src/blob/tree.rs +++ b/crates/core/src/blob/tree.rs @@ -317,9 +317,10 @@ where recursive: bool, ) -> RusticResult { let inner = if node.is_dir() { - Tree::from_backend(&be, index, node.subtree.unwrap())? - .nodes - .into_iter() + let Some(id) = node.subtree else { + return Err(TreeErrorKind::BlobIdNotFoundForNode(node.name()).into()); + }; + Tree::from_backend(&be, index, id)?.nodes.into_iter() } else { vec![node.clone()].into_iter() }; @@ -504,19 +505,28 @@ impl TreeStreamerOnce

{ let (out_tx, out_rx) = bounded(constants::MAX_TREE_LOADER); let (in_tx, in_rx) = unbounded(); - for _ in 0..constants::MAX_TREE_LOADER { - let be = be.clone(); - let index = index.clone(); - let in_rx = in_rx.clone(); - let out_tx = out_tx.clone(); - let _join_handle = std::thread::spawn(move || { - for (path, id, count) in in_rx { - out_tx - .send(Tree::from_backend(&be, &index, id).map(|tree| (path, tree, count))) - .unwrap(); - } - }); - } + let _join_handles = (0..constants::MAX_TREE_LOADER) + .map(|_| -> JoinHandle> { + let be = be.clone(); + let index = index.clone(); + let in_rx = in_rx.clone(); + let out_tx = out_tx.clone(); + std::thread::spawn(move || -> RusticResult<()> { + for (path, id, count) in in_rx { + if out_tx + .send( + Tree::from_backend(&be, &index, id).map(|tree| (path, tree, count)), + ) + .is_err() + { + error!("receiver has been dropped unexpectedly."); + return Err(MultiprocessingErrorKind::ReceiverDropped.into()); + } + } + Ok(()) + }) + }) + .collect::>>>(); let counter = vec![0; ids.len()]; let mut streamer = Self { @@ -559,11 +569,16 @@ impl TreeStreamerOnce

{ /// [`MultiprocessingErrorKind::QueueInNotAvailable`]: crate::error::MultiprocessingErrorKind::QueueInNotAvailable fn add_pending(&mut self, path: PathBuf, id: Id, count: usize) -> RusticResult { if self.visited.insert(id) { - self.queue_in + if self + .queue_in .as_ref() - .unwrap() + .ok_or(MultiprocessingErrorKind::QueueInNotAvailable)? .send((path, id, count)) - .map_err(TreeErrorKind::SendingCrossbeamMessageFailed)?; + .is_err() + { + error!("receiver has been dropped unexpectedly."); + return Err(MultiprocessingErrorKind::ReceiverDropped.into()); + } self.counter[count] += 1; Ok(true) } else { @@ -581,14 +596,15 @@ impl Iterator for TreeStreamerOnce

{ self.p.finish(); return None; } - let (path, tree, count) = match self.queue_out.recv() { - Ok(Ok(res)) => res, - Err(err) => { - return Some(Err( - TreeErrorKind::ReceivingCrossbreamMessageFailed(err).into() - )) - } - Ok(Err(err)) => return Some(Err(err)), + + // We don't print an error, because it is a progress bar and it is unexpected to + // show an error message in a progress bar, hence we are just returning `None` in + // case of an error with the channel. + let (path, tree, count) = if let Ok(res) = self.queue_out.recv() { + res.ok()? + } else { + error!("sender has been dropped unexpectedly."); + return Some(Err(MultiprocessingErrorKind::SenderDropped.into())); }; for node in &tree.nodes { @@ -739,13 +755,16 @@ pub(crate) fn merge_nodes( save: &impl Fn(Tree) -> RusticResult<(Id, u64)>, summary: &mut SnapshotSummary, ) -> RusticResult { - let trees: Vec<_> = nodes + let trees = nodes .iter() - .filter(|node| node.is_dir()) - .map(|node| node.subtree.unwrap()) - .collect(); - - let mut node = nodes.into_iter().max_by(|n1, n2| cmp(n1, n2)).unwrap(); + .filter(|node| node.is_dir() && node.subtree.is_some()) + .filter_map(|node| node.subtree) + .collect::>(); + + let mut node = nodes + .into_iter() + .max_by(|n1, n2| cmp(n1, n2)) + .ok_or(TreeErrorKind::NoNodeInListToBeMerged)?; // if this is a dir, merge with all other dirs if node.is_dir() { From 3e2e3fe785674cf8e0970f819bb496980ad7648c Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:05:19 +0100 Subject: [PATCH 42/58] parking lot rwlock Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/commands/repair/snapshots.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/core/src/commands/repair/snapshots.rs b/crates/core/src/commands/repair/snapshots.rs index 449d4ba6..2fe69e5d 100644 --- a/crates/core/src/commands/repair/snapshots.rs +++ b/crates/core/src/commands/repair/snapshots.rs @@ -149,7 +149,7 @@ impl RepairSnapshotsOptions { if !dry_run { _ = packer.finalize()?; - indexer.write().unwrap().finalize()?; + indexer.write().finalize()?; } if self.delete { From 34b55e4c3d7cd3e730775557b1a2dc54f2a2c4da Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:06:25 +0100 Subject: [PATCH 43/58] handle unwrap in repair cmd Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/commands/repair/snapshots.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/core/src/commands/repair/snapshots.rs b/crates/core/src/commands/repair/snapshots.rs index 2fe69e5d..83acf750 100644 --- a/crates/core/src/commands/repair/snapshots.rs +++ b/crates/core/src/commands/repair/snapshots.rs @@ -221,13 +221,13 @@ impl RepairSnapshotsOptions { let mut file_changed = false; let mut new_content = Vec::new(); let mut new_size = 0; - for blob in node.content.take().unwrap() { - index.get_data(&blob).map_or_else( + for id in node.content.iter().flatten() { + index.get_data(id).map_or_else( || { file_changed = true; }, |ie| { - new_content.push(blob); + new_content.push(*id); new_size += u64::from(ie.data_length()); }, ); From 5c55690c16647db044363462a7bb10b165026c47 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:06:56 +0100 Subject: [PATCH 44/58] error collector Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/commands/check.rs | 79 ++++++++++++++++++++----------- 1 file changed, 51 insertions(+), 28 deletions(-) diff --git a/crates/core/src/commands/check.rs b/crates/core/src/commands/check.rs index 3ee35095..3b39df9e 100644 --- a/crates/core/src/commands/check.rs +++ b/crates/core/src/commands/check.rs @@ -119,18 +119,21 @@ impl CheckOptions { let p = pb.progress_bytes("reading pack data..."); p.set_length(total_pack_size); + // REVIEW: Behaviour could have changed here, check! index_be - .into_index() + .into_index()? .into_iter() .par_bridge() - .for_each(|pack| { + .map(|pack| -> RusticResult<()> { let id = pack.id; - let data = be.read_full(FileType::Pack, &id).unwrap(); - match check_pack(be, pack, data, &p) { - Ok(()) => {} - Err(err) => error!("Error reading pack {id} : {err}",), - } - }); + let data = be + .read_full(FileType::Pack, &id) + .map_err(RusticErrorKind::Backend)?; + + check_pack(be, pack, data, &p) + }) + .collect::>()?; + p.finish(); } Ok(()) @@ -389,38 +392,55 @@ fn check_snapshots( let p = pb.progress_counter("checking trees..."); let mut tree_streamer = TreeStreamerOnce::new(be, index, snap_trees, p)?; + let mut errors = vec![]; while let Some(item) = tree_streamer.next().transpose()? { let (path, tree) = item; for node in tree.nodes { match node.node_type { - NodeType::File => node.content.as_ref().map_or_else( - || { - error!("file {:?} doesn't have a content", path.join(node.name())); - }, - |content| { - for (i, id) in content.iter().enumerate() { - if id.is_null() { - error!("file {:?} blob {} has null ID", path.join(node.name()), i); - } - - if !index.has_data(id) { - error!( - "file {:?} blob {} is missing in index", - path.join(node.name()), - id - ); - } + NodeType::File => { + let Some(content) = node.content.as_ref() else { + error!("file {:?} doesn't have content", path.join(node.name())); + errors.push(CheckErrorKind::MissingContent { + path: path.join(node.name()), + }); + continue; + }; + for (i, id) in content.iter().enumerate() { + if id.is_null() { + error!("file {:?} blob {} has null ID", path.join(node.name()), i); + errors.push(CheckErrorKind::BlobHasNullId { + path: path.join(node.name()), + index: i, + }); } - }, - ), + if !index.has_data(id) { + error!( + "file {:?} blob {} is missing in index", + path.join(node.name()), + id + ); + errors.push(CheckErrorKind::MissingBlob { + path: path.join(node.name()), + id: *id, + index: i, + }); + } + } + } NodeType::Dir => { match node.subtree { None => { error!("dir {:?} subtree does not exist", path.join(node.name())); + errors.push(CheckErrorKind::MissingSubtree { + path: path.join(node.name()), + }); } Some(tree) if tree.is_null() => { error!("dir {:?} subtree has null ID", path.join(node.name())); + errors.push(CheckErrorKind::SubtreeHasNullId { + path: path.join(node.name()), + }); } _ => {} // subtree is ok } @@ -430,6 +450,9 @@ fn check_snapshots( } } } + if !errors.is_empty() { + return Err(CheckErrorKind::ErrorCollection(errors).into()); + } Ok(()) } @@ -501,7 +524,7 @@ fn check_pack( // TODO: this is identical to backend/decrypt.rs; unify these two parts! if let Some(length) = blob.uncompressed_length { - blob_data = decode_all(&*blob_data).unwrap(); + blob_data = decode_all(&*blob_data)?; if blob_data.len() != length.get() as usize { error!("pack {id}, blob {blob_id}: Actual uncompressed length does not fit saved uncompressed length"); return Ok(()); From 49a08efb3e552d575fd43b889f3c8c60abbcadcb Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:07:38 +0100 Subject: [PATCH 45/58] handle get_tree errs Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/commands/copy.rs | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/crates/core/src/commands/copy.rs b/crates/core/src/commands/copy.rs index 23d406fd..2bb8810b 100644 --- a/crates/core/src/commands/copy.rs +++ b/crates/core/src/commands/copy.rs @@ -82,7 +82,10 @@ pub(crate) fn copy<'a, Q, R: IndexedFull, P: ProgressBars, S: IndexedIds>( .try_for_each(|id| -> RusticResult<_> { trace!("copy tree blob {id}"); if !index_dest.has_tree(id) { - let data = index.get_tree(id).unwrap().read_data(be)?; + let data = index + .get_tree(id) + .ok_or(CommandErrorKind::TreeNotFound(id.to_string()))? + .read_data(be)?; p.inc(data.len() as u64); tree_packer.add(data, *id)?; } @@ -101,7 +104,10 @@ pub(crate) fn copy<'a, Q, R: IndexedFull, P: ProgressBars, S: IndexedIds>( |id| -> RusticResult<_> { trace!("copy data blob {id}"); if !index_dest.has_data(id) { - let data = index.get_data(id).unwrap().read_data(be)?; + let data = index + .get_data(id) + .ok_or(CommandErrorKind::DataBlobNotFound(id.to_string()))? + .read_data(be)?; p.inc(data.len() as u64); data_packer.add(data, *id)?; } @@ -111,10 +117,18 @@ pub(crate) fn copy<'a, Q, R: IndexedFull, P: ProgressBars, S: IndexedIds>( } NodeType::Dir => { - let id = node.subtree.unwrap(); + let Some(id) = node.subtree else { + return Err(CommandErrorKind::MissingSubtree { + path: node.name().into(), + } + .into()); + }; trace!("copy tree blob {id}"); if !index_dest.has_tree(&id) { - let data = index.get_tree(&id).unwrap().read_data(be)?; + let data = index + .get_tree(&id) + .ok_or(CommandErrorKind::TreeNotFound(id.to_string()))? + .read_data(be)?; p.inc(data.len() as u64); tree_packer.add(data, id)?; } @@ -128,7 +142,7 @@ pub(crate) fn copy<'a, Q, R: IndexedFull, P: ProgressBars, S: IndexedIds>( _ = data_packer.finalize()?; _ = tree_packer.finalize()?; - indexer.write().unwrap().finalize()?; + indexer.write().finalize()?; let p = pb.progress_counter("saving snapshots..."); be_dest.save_list(snaps.iter(), p)?; From 5e57105f02beea0361a24ecefd8fae2895db0f87 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:08:54 +0100 Subject: [PATCH 46/58] handle unwrap in loop Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/commands/dump.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/core/src/commands/dump.rs b/crates/core/src/commands/dump.rs index 8d9d204f..6d7afc50 100644 --- a/crates/core/src/commands/dump.rs +++ b/crates/core/src/commands/dump.rs @@ -34,7 +34,7 @@ pub(crate) fn dump( return Err(CommandErrorKind::DumpNotSupported(node.node_type.clone()).into()); } - for id in node.content.as_ref().unwrap() { + for id in node.content.iter().flatten() { let data = repo.get_blob_cached(id, BlobType::Data)?; w.write_all(&data)?; } From c283f0bc393423c8979279010492aab19f9b4f6a Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:10:15 +0100 Subject: [PATCH 47/58] handle errs in forget Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/commands/forget.rs | 49 ++++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/crates/core/src/commands/forget.rs b/crates/core/src/commands/forget.rs index 4b15375e..72c3411a 100644 --- a/crates/core/src/commands/forget.rs +++ b/crates/core/src/commands/forget.rs @@ -86,11 +86,13 @@ pub(crate) fn get_forget_snapshots( let groups = repo .get_snapshot_group(&[], group_by, filter)? .into_iter() - .map(|(group, snapshots)| ForgetGroup { - group, - snapshots: keep.apply(snapshots, now), + .map(|(group, snapshots)| -> RusticResult { + Ok(ForgetGroup { + group, + snapshots: keep.apply(snapshots, now)?, + }) }) - .collect(); + .collect::>>()?; Ok(ForgetGroups(groups)) } @@ -418,13 +420,14 @@ impl KeepOptions { /// # Returns /// /// The list of reasons why the snapshot should be kept + #[allow(clippy::too_many_lines)] fn matches( &mut self, sn: &SnapshotFile, last: Option<&SnapshotFile>, has_next: bool, latest_time: DateTime, - ) -> Vec<&str> { + ) -> RusticResult> { let mut reason = Vec::new(); let snapshot_id_hex = sn.id.to_hex(); @@ -500,19 +503,35 @@ impl KeepOptions { ]; for (check_fun, counter, reason1, within, reason2) in keep_checks { - if !has_next || last.is_none() || !check_fun(sn, last.unwrap()) { + if !has_next + || last.is_none() + || !check_fun( + sn, + last.ok_or(CommandErrorKind::NoLastSnapshot { + snapshot_id: sn.id.to_hex().as_str().to_string(), + })?, + ) + { if *counter != 0 { reason.push(reason1); if *counter > 0 { *counter -= 1; } } - if sn.time + Duration::from_std(*within).unwrap() > latest_time { + if sn.time + + Duration::from_std(*within).map_err(|src| { + CommandErrorKind::DurationError { + duration: within.to_string(), + source: src, + } + })? + > latest_time + { reason.push(reason2); } } } - reason + Ok(reason) } /// Apply the `[KeepOptions]` to the given list of [`SnapshotFile`]s returning the corresponding @@ -535,11 +554,11 @@ impl KeepOptions { &self, mut snapshots: Vec, now: DateTime, - ) -> Vec { + ) -> RusticResult> { let mut group_keep = self.clone(); let mut snaps = Vec::new(); if snapshots.is_empty() { - return snaps; + return Ok(snaps); } snapshots.sort_unstable_by(|sn1, sn2| sn1.cmp(sn2).reverse()); @@ -555,8 +574,12 @@ impl KeepOptions { } else if sn.must_delete(now) { (false, vec!["snapshot"]) } else { - let reasons = - group_keep.matches(&sn, last.as_ref(), iter.peek().is_some(), latest_time); + let reasons = group_keep.matches( + &sn, + last.as_ref(), + iter.peek().is_some(), + latest_time, + )?; let keep = !reasons.is_empty(); (keep, reasons) } @@ -569,6 +592,6 @@ impl KeepOptions { reasons: reasons.iter().map(ToString::to_string).collect(), }); } - snaps + Ok(snaps) } } From c1f95b2e8dfc34afd375d82803659d64f1fa1156 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:10:38 +0100 Subject: [PATCH 48/58] handle errs in prune Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/commands/prune.rs | 93 ++++++++++++++++++++----------- 1 file changed, 62 insertions(+), 31 deletions(-) diff --git a/crates/core/src/commands/prune.rs b/crates/core/src/commands/prune.rs index 2e1ae603..59a7ed39 100644 --- a/crates/core/src/commands/prune.rs +++ b/crates/core/src/commands/prune.rs @@ -235,7 +235,7 @@ impl PruneOptions { self.repack_uncompressed || self.repack_all, self.no_resize, &pack_sizer, - ); + )?; pruner.check_existing_packs()?; pruner.filter_index_files(self.instant_delete); @@ -808,11 +808,11 @@ impl PrunePlan { if to_compress { _ = status.insert(PackStatus::NotCompressed); } - let size_mismatch = !pack_sizer[pack.blob_type].size_ok(pack.size); - if pack_sizer[pack.blob_type].is_too_small(pack.size) { + let size_mismatch = !pack_sizer[pack.blob_type].size_ok(pack.size)?; + if pack_sizer[pack.blob_type].is_too_small(pack.size)? { _ = status.insert(PackStatus::TooSmall); } - if pack_sizer[pack.blob_type].is_too_large(pack.size) { + if pack_sizer[pack.blob_type].is_too_large(pack.size)? { _ = status.insert(PackStatus::TooLarge); } match (pack.delete_mark, pi.used_blobs, pi.unused_blobs) { @@ -934,7 +934,7 @@ impl PrunePlan { repack_uncompressed: bool, no_resize: bool, pack_sizer: &BlobTypeMap, - ) { + ) -> RusticResult<()> { let max_unused = match (repack_uncompressed, max_unused) { (true, _) => 0, (false, LimitOption::Unlimited) => u64::MAX, @@ -983,7 +983,7 @@ impl PrunePlan { // packs in resize_packs are only repacked if we anyway repack this blob type or // if the target pack size is reached for the blob type. let todo = if do_repack[blob_type] - || repack_size[blob_type] > u64::from(pack_sizer[blob_type].pack_size()) + || repack_size[blob_type] > u64::from(pack_sizer[blob_type].pack_size()?) { PackToDo::Repack } else { @@ -994,6 +994,7 @@ impl PrunePlan { pack.set_todo(todo, &pi, status, &mut self.stats); } } + Ok(()) } /// Checks if the existing packs are ok @@ -1170,7 +1171,9 @@ impl PrunePlan { } else { let p = pb.progress_counter("marking unneeded unindexed pack files for deletion..."); - p.set_length(self.existing_packs.len().try_into().unwrap()); + p.set_length(u64::try_from(self.existing_packs.len()).map_err(|_| { + CommandErrorKind::TooManyPacksToDelete(self.existing_packs.len()) + })?); for (id, size) in self.existing_packs { let pack = IndexPack { id, @@ -1202,11 +1205,25 @@ impl PrunePlan { let tree_packs_remove = Arc::new(Mutex::new(Vec::new())); let data_packs_remove = Arc::new(Mutex::new(Vec::new())); - let delete_pack = |pack: PrunePack| { + let delete_pack = |pack: PrunePack| -> RusticResult<()> { // delete pack match pack.blob_type { - BlobType::Data => data_packs_remove.lock().unwrap().push(pack.id), - BlobType::Tree => tree_packs_remove.lock().unwrap().push(pack.id), + BlobType::Data => { + if let Ok(mut lock) = data_packs_remove.lock() { + lock.push(pack.id); + Ok(()) + } else { + Err(CommandErrorKind::MutexLockFailed.into()) + } + } + BlobType::Tree => { + if let Ok(mut lock) = tree_packs_remove.lock() { + lock.push(pack.id); + Ok(()) + } else { + Err(CommandErrorKind::MutexLockFailed.into()) + } + } } }; @@ -1237,15 +1254,19 @@ impl PrunePlan { PackToDo::Keep => { // keep pack: add to new index let pack = pack.into_index_pack(); - indexer.write().unwrap().add(pack)?; + indexer.write().add(pack)?; } PackToDo::Repack => { // TODO: repack in parallel for blob in &pack.blobs { - if used_ids.lock().unwrap().remove(&blob.id).is_none() { - // don't save duplicate blobs - continue; - } + if let Ok(mut lock) = used_ids.lock() { + if lock.remove(&blob.id).is_none() { + // don't save duplicate blobs + continue; + } + } else { + return Err(CommandErrorKind::MutexLockFailed.into()); + }; let repacker = match blob.tpe { BlobType::Data => &data_repacker, @@ -1259,7 +1280,7 @@ impl PrunePlan { p.inc(u64::from(blob.length)); } if opts.instant_delete { - delete_pack(pack); + delete_pack(pack)?; } else { // mark pack for removal let pack = pack.into_index_pack_with_time(self.time); @@ -1268,7 +1289,7 @@ impl PrunePlan { } PackToDo::MarkDelete => { if opts.instant_delete { - delete_pack(pack); + delete_pack(pack)?; } else { // mark pack for removal let pack = pack.into_index_pack_with_time(self.time); @@ -1277,7 +1298,7 @@ impl PrunePlan { } PackToDo::KeepMarked | PackToDo::KeepMarkedAndCorrect => { if opts.instant_delete { - delete_pack(pack); + delete_pack(pack)?; } else { // keep pack: add to new index; keep the timestamp. // Note the timestap shouldn't be None here, however if it is not not set, use the current time to heal the entry! @@ -1291,7 +1312,7 @@ impl PrunePlan { let pack = pack.into_index_pack_with_time(self.time); indexer.write().add(pack)?; } - PackToDo::Delete => delete_pack(pack), + PackToDo::Delete => delete_pack(pack)?, } Ok(()) })?; @@ -1306,18 +1327,24 @@ impl PrunePlan { be.delete_list(FileType::Index, true, indexes_remove.iter(), p)?; } - // get variable out of Arc> - let data_packs_remove = data_packs_remove.lock().unwrap(); - if !data_packs_remove.is_empty() { - let p = pb.progress_counter("removing old data packs..."); - be.delete_list(FileType::Pack, false, data_packs_remove.iter(), p)?; + // remove old data packs + if let Ok(data_packs_remove) = data_packs_remove.lock() { + if !data_packs_remove.is_empty() { + let p = pb.progress_counter("removing old data packs..."); + be.delete_list(FileType::Pack, false, data_packs_remove.iter(), p)?; + } + } else { + return Err(CommandErrorKind::MutexLockFailed.into()); } - // get variable out of Arc> - let tree_packs_remove = tree_packs_remove.lock().unwrap(); - if !tree_packs_remove.is_empty() { - let p = pb.progress_counter("removing old tree packs..."); - be.delete_list(FileType::Pack, true, tree_packs_remove.iter(), p)?; + // remove old tree packs + if let Ok(tree_packs_remove) = tree_packs_remove.lock() { + if !tree_packs_remove.is_empty() { + let p = pb.progress_counter("removing old tree packs..."); + be.delete_list(FileType::Pack, true, tree_packs_remove.iter(), p)?; + } + } else { + return Err(CommandErrorKind::MutexLockFailed.into()); } Ok(()) @@ -1325,7 +1352,7 @@ impl PrunePlan { } /// `PackInfo` contains information about a pack which is needed to decide what to do with the pack. -#[derive(PartialEq, Eq, Clone, Copy, Debug)] +#[derive(PartialEq, Eq, Clone, Copy, Debug, Serialize)] struct PackInfo { /// What type of blobs are in the pack blob_type: BlobType, @@ -1485,7 +1512,11 @@ fn find_used_blobs( ids.extend(node.content.iter().flatten().map(|id| (*id, 0))); } NodeType::Dir => { - _ = ids.insert(node.subtree.unwrap(), 0); + let Some(id) = node.subtree else { + warn!("subtree not found in dir node!"); + continue; + }; + _ = ids.insert(id, 0); } _ => {} // nothing to do } From 55df35fcb062022ca52e3bdd1ae078d2cb215d3d Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:11:17 +0100 Subject: [PATCH 49/58] collect errs in set_metadata Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/commands/restore.rs | 63 +++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 16 deletions(-) diff --git a/crates/core/src/commands/restore.rs b/crates/core/src/commands/restore.rs index aa5712d9..ed9ef4d5 100644 --- a/crates/core/src/commands/restore.rs +++ b/crates/core/src/commands/restore.rs @@ -379,26 +379,57 @@ impl RestoreOptions { /// # Errors /// /// If the metadata could not be set. - // TODO: Return a result here, introduce errors and get rid of logging. - fn set_metadata(self, dest: &LocalDestination, path: &PathBuf, node: &Node) { + fn set_metadata( + self, + dest: &LocalDestination, + path: &PathBuf, + node: &Node, + ) -> RusticResult<()> { debug!("setting metadata for {:?}", path); - dest.create_special(path, node) - .unwrap_or_else(|_| warn!("restore {:?}: creating special file failed.", path)); + + let mut errors = vec![]; + + if let Err(err) = dest.create_special(path, node) { + warn!("restore {:?}: creating special file failed.", path); + errors.push(err); + } + match (self.no_ownership, self.numeric_id) { (true, _) => {} - (false, true) => dest - .set_uid_gid(path, &node.meta) - .unwrap_or_else(|_| warn!("restore {:?}: setting UID/GID failed.", path)), - (false, false) => dest - .set_user_group(path, &node.meta) - .unwrap_or_else(|_| warn!("restore {:?}: setting User/Group failed.", path)), + (false, true) => { + if let Err(err) = dest.set_uid_gid(path, &node.meta) { + warn!("restore {:?}: setting UID/GID failed.", path); + errors.push(err); + } + } + (false, false) => { + if let Err(err) = dest.set_user_group(path, &node.meta) { + warn!("restore {:?}: setting User/Group failed.", path); + errors.push(err); + } + } } - dest.set_permission(path, node) - .unwrap_or_else(|_| warn!("restore {:?}: chmod failed.", path)); - dest.set_extended_attributes(path, &node.meta.extended_attributes) - .unwrap_or_else(|_| warn!("restore {:?}: setting extended attributes failed.", path)); - dest.set_times(path, &node.meta) - .unwrap_or_else(|_| warn!("restore {:?}: setting file times failed.", path)); + + if let Err(err) = dest.set_permission(path, node) { + warn!("restore {:?}: chmod failed.", path); + errors.push(err); + }; + + if let Err(err) = dest.set_extended_attributes(path, &node.meta.extended_attributes) { + warn!("restore {:?}: setting extended attributes failed.", path); + errors.push(err); + }; + + if let Err(err) = dest.set_times(path, &node.meta) { + warn!("restore {:?}: setting file times failed.", path); + errors.push(err); + }; + + if !errors.is_empty() { + return Err(CommandErrorKind::ErrorSettingMetadata(path.clone(), errors).into()); + } + + Ok(()) } } From 8e01f893883932d28364ab58386a6d58cf9756bf Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:12:00 +0100 Subject: [PATCH 50/58] refactor match to if Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/commands/restore.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/crates/core/src/commands/restore.rs b/crates/core/src/commands/restore.rs index ed9ef4d5..f2d47b98 100644 --- a/crates/core/src/commands/restore.rs +++ b/crates/core/src/commands/restore.rs @@ -343,26 +343,26 @@ impl RestoreOptions { ) -> RusticResult<()> { let mut dir_stack = Vec::new(); while let Some((path, node)) = node_streamer.next().transpose()? { - match node.node_type { - NodeType::Dir => { - // set metadata for all non-parent paths in stack - while let Some((stackpath, _)) = dir_stack.last() { - if path.starts_with(stackpath) { - break; - } - let (path, node) = dir_stack.pop().unwrap(); - self.set_metadata(dest, &path, &node); + if node.node_type == NodeType::Dir { + // set metadata for all non-parent paths in stack + while let Some((stack_path, stack_node)) = dir_stack.last() { + if path.starts_with(stack_path) { + break; } - // push current path to the stack - dir_stack.push((path, node)); + self.set_metadata(dest, stack_path, stack_node)?; + _ = dir_stack.pop(); } - _ => self.set_metadata(dest, &path, &node), + + // push current path to the stack + dir_stack.push((path, node)); + } else { + self.set_metadata(dest, &path, &node)?; } } // empty dir stack and set metadata for (path, node) in dir_stack.into_iter().rev() { - self.set_metadata(dest, &path, &node); + self.set_metadata(dest, &path, &node)?; } Ok(()) From cc82a9e1d3832ab96691bb37e944d60e4bffbffc Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:12:44 +0100 Subject: [PATCH 51/58] multiprocessing errs in restore Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/commands/restore.rs | 151 +++++++++++++++++++++++----- 1 file changed, 125 insertions(+), 26 deletions(-) diff --git a/crates/core/src/commands/restore.rs b/crates/core/src/commands/restore.rs index f2d47b98..d974dee4 100644 --- a/crates/core/src/commands/restore.rs +++ b/crates/core/src/commands/restore.rs @@ -520,22 +520,62 @@ fn restore_contents( .num_threads(constants::MAX_READER_THREADS_NUM) .build() .map_err(CommandErrorKind::FromRayonError)?; - pool.in_place_scope(|s| { + let (error_tx, error_rx) = unbounded::(); + let error_sender_outer = error_tx.clone(); + let error_sender_inner = error_tx; + + // This is a workaround for the fact that we can't use the `?` operator in a closure + // Still we want to propagate the error to the main thread. + // Handle errors `in_place_scope` + // TODO!: store join handle? + let _join_handle = std::thread::spawn(move || { + let mut count = 0; + + while let Ok(err) = error_rx.recv() { + error!("Error during restore: {:?}", err); + count += 1; + } + + if count > 0 { + error!("{count} errors occurred during restore.",); + } + }); + + pool.in_place_scope(|outer_scope| -> RusticResult<()> { for (pack, offset, length, from_file, name_dests) in blobs { let p = &p; + let error_sender_outer_clone = error_sender_outer.clone(); + let error_sender_inner_clone = error_sender_inner.clone(); if !name_dests.is_empty() { - // TODO: error handling! - s.spawn(move |s1| { + outer_scope.spawn(move |inner_scope: &Scope<'_>| { let read_data = match &from_file { Some((file_idx, offset_file, length_file)) => { // read from existing file match dest.read_at(&file_names[*file_idx], *offset_file, *length_file) { + Ok(val) => val, + Err(err) => { + if error_sender_outer_clone.send(err).is_err() { + error!("receiver has been dropped unexpectedly."); + }; + return; + } + } } None => { // read needed part of the pack - be.read_partial(FileType::Pack, &pack, false, offset, length) - .unwrap() + match be + .read_partial(FileType::Pack, &pack, false, offset, length) + .map_err(RusticErrorKind::Backend) + { + Ok(val) => val, + Err(err) => { + if error_sender_outer_clone.send(err.into()).is_err() { + error!("receiver has been dropped unexpectedly."); + }; + return; + } + } } }; @@ -545,33 +585,91 @@ fn restore_contents( let data = if from_file.is_some() { read_data.clone() } else { - let start = usize::try_from(bl.offset - offset).unwrap(); - let end = usize::try_from(bl.offset + bl.length - offset).unwrap(); - be.read_encrypted_from_partial( + let start = match usize::try_from(bl.offset - offset) { + Ok(val) => val, + Err(err) => { + if error_sender_outer_clone + .send(CommandErrorKind::ConversionFromIntFailed(err).into()) + .is_err() + { + error!("receiver has been dropped unexpectedly."); + }; + return; + } + }; + let end = match usize::try_from(bl.offset + bl.length - offset) { + Ok(val) => val, + Err(err) => { + if error_sender_outer_clone + .send(CommandErrorKind::ConversionFromIntFailed(err).into()) + .is_err() + { + error!("receiver has been dropped unexpectedly."); + }; + return; + } + }; + + match be.read_encrypted_from_partial( &read_data[start..end], bl.uncompressed_length, - ) - .unwrap() + ) { + Ok(val) => val, + Err(err) => { + if error_sender_outer_clone.send(err).is_err() { + error!("receiver has been dropped unexpectedly."); + }; + return; + } + } }; + for (_, file_idx, start) in group { let data = data.clone(); + + let error_sender_inner_clone = error_sender_inner_clone.clone(); + + inner_scope.spawn(move |_| { let path = &file_names[file_idx]; + // Allocate file if it is not yet allocated - let mut sizes_guard = sizes.lock().unwrap(); - let filesize = sizes_guard[file_idx]; - if filesize > 0 { - dest.set_length(path, filesize) - .map_err(|err| { - CommandErrorKind::ErrorSettingLength( - path.clone(), - Box::new(err), - ) - }) - .unwrap(); - sizes_guard[file_idx] = 0; - } - drop(sizes_guard); - dest.write_at(path, start, &data).unwrap(); + if let Ok(mut sizes_guard) = sizes.lock() { + let filesize = sizes_guard[file_idx]; + if filesize > 0 { + match dest.set_length(path, filesize) { + Ok(val) => val, + Err(err) => { + if error_sender_inner_clone.send(err).is_err() { + error!( + "receiver has been dropped unexpectedly." + ); + }; + + return; + } + }; + + sizes_guard[file_idx] = 0; + } + } else { + if error_sender_inner_clone + .send(CommandErrorKind::MutexLockFailed.into()) + .is_err() + { + error!("receiver has been dropped unexpectedly."); + }; + + return; + }; + + if let Err(err) = dest.write_at(path, start, &data) { + if error_sender_inner_clone.send(err).is_err() { + error!("receiver has been dropped unexpectedly."); + }; + + return; + }; + p.inc(size); }); } @@ -579,7 +677,8 @@ fn restore_contents( }); } } - }); + Ok(()) + })?; p.finish(); From 84b459c54af2a4f8f2ec03c8acabcf5ed612ea0f Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:13:06 +0100 Subject: [PATCH 52/58] handling other errs in restore Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/commands/restore.rs | 33 +++++++++++++++++++++++++---- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/crates/core/src/commands/restore.rs b/crates/core/src/commands/restore.rs index d974dee4..d0bd6a91 100644 --- a/crates/core/src/commands/restore.rs +++ b/crates/core/src/commands/restore.rs @@ -173,12 +173,25 @@ impl RestoreOptions { } debug!("additional {:?}", entry.path()); - if entry.file_type().unwrap().is_dir() { + if entry + .file_type() + .ok_or_else(|| CommandErrorKind::ErrorReadingFileType(entry.path().to_path_buf()))? + .is_dir() + { stats.dirs.additional += 1; } else { stats.files.additional += 1; } - match (self.delete, dry_run, entry.file_type().unwrap().is_dir()) { + match ( + self.delete, + dry_run, + entry + .file_type() + .ok_or_else(|| { + CommandErrorKind::ErrorReadingFileType(entry.path().to_path_buf()) + })? + .is_dir(), + ) { (true, true, true) => { info!("would have removed the additional dir: {:?}", entry.path()); } @@ -292,8 +305,20 @@ impl RestoreOptions { } Ordering::Equal => { // process existing node - if (node.is_dir() && !destination.file_type().unwrap().is_dir()) - || (node.is_file() && !destination.metadata().unwrap().is_file()) + if (node.is_dir() + && !destination + .file_type() + .ok_or_else(|| { + CommandErrorKind::ErrorReadingFileType( + destination.path().to_path_buf(), + ) + })? + .is_dir()) + || (node.is_file() + && !destination + .metadata() + .map_err(IgnoreErrorKind::GenericError)? + .is_file()) || node.is_special() { // if types do not match, first remove the existing file From 7db23969879f4087c5e50bb776961b8380d98e08 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:13:26 +0100 Subject: [PATCH 53/58] multiprocessing errs in archiver Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/archiver.rs | 51 ++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 17 deletions(-) diff --git a/crates/core/src/archiver.rs b/crates/core/src/archiver.rs index 3c8b25bc..40fd7da5 100644 --- a/crates/core/src/archiver.rs +++ b/crates/core/src/archiver.rs @@ -136,7 +136,7 @@ impl<'a, BE: DecryptFullBackend, I: ReadGlobalIndex> Archiver<'a, BE, I> { ::Open: Send, ::Iter: Send, { - std::thread::scope(|s| -> RusticResult<_> { + std::thread::scope(|s| -> RusticResult<()> { // determine backup size in parallel to running backup let src_size_handle = s.spawn(|| { if !no_scan && !p.is_hidden() { @@ -156,30 +156,31 @@ impl<'a, BE: DecryptFullBackend, I: ReadGlobalIndex> Archiver<'a, BE, I> { } Ok(ReadSourceEntry { path, node, open }) => { let snapshot_path = if let Some(as_path) = as_path { - as_path - .clone() - .join(path.strip_prefix(backup_path).unwrap()) + as_path.clone().join(path.strip_prefix(backup_path).ok()?) } else { path }; Some(if node.is_dir() { (snapshot_path, node, open) } else { - ( - snapshot_path - .parent() - .expect("file path should have a parent!") - .to_path_buf(), - node, - open, - ) + let Some(parent) = snapshot_path.parent() else { + warn!( + "ignoring file {}: no parent found!", + snapshot_path.display() + ); + + return None; + }; + + (parent.to_path_buf(), node, open) }) } }); + // handle beginning and ending of trees let iter = TreeIterator::new(iter); - scope(|scope| -> RusticResult<_> { + let scoped_work = scope(|scope| -> RusticResult<()> { // use parent snapshot iter.filter_map( |item| match self.parent.process(&self.be, self.index, item) { @@ -201,10 +202,26 @@ impl<'a, BE: DecryptFullBackend, I: ReadGlobalIndex> Archiver<'a, BE, I> { } }) .try_for_each(|item| self.tree_archiver.add(item)) - }) - .unwrap()?; - src_size_handle.join().unwrap(); - Ok(()) + }); + if let Err(err) = scoped_work { + error!("thread panicked: {err:?}"); + return Err(MultiprocessingErrorKind::JoinError { + location: format!("archiver::archive, {}", line!()), + } + .into()); + } else if let Ok(Err(err)) = scoped_work { + return Err(err); + }; + if let Err(err) = src_size_handle.join() { + error!("error joining src_size_handle: {err:?}"); + + Err(MultiprocessingErrorKind::JoinError { + location: format!("archiver::archive::src_size_handle, {}", line!()), + } + .into()) + } else { + Ok(()) + } })?; let stats = self.file_archiver.finalize()?; From b56487ddd0d90f8375b00879f26bf4f18186e9b8 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:14:06 +0100 Subject: [PATCH 54/58] results for fallible location Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/backend.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/core/src/backend.rs b/crates/core/src/backend.rs index 5af0f3cd..88cd9f44 100644 --- a/crates/core/src/backend.rs +++ b/crates/core/src/backend.rs @@ -93,6 +93,7 @@ pub trait ReadBackend: Send + Sync + 'static { /// # Returns /// /// The location of the backend. + fn location(&self) -> Result; /// Lists all files with their size of the given type. /// @@ -352,7 +353,7 @@ mock! { Backend {} impl ReadBackend for Backend{ - fn location(&self) -> String; + fn location(&self) -> Result; fn list_with_size(&self, tpe: FileType) -> Result>; fn read_full(&self, tpe: FileType, id: &Id) -> Result; fn read_partial( From 35d585aa0ed2dfea78e3f6b2a6fc24ce99a32e97 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:14:21 +0100 Subject: [PATCH 55/58] doc changes Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/repository.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/core/src/repository.rs b/crates/core/src/repository.rs index 4f5cdd4d..f3a51d1e 100644 --- a/crates/core/src/repository.rs +++ b/crates/core/src/repository.rs @@ -58,9 +58,9 @@ use crate::{ snapshotfile::{SnapshotGroup, SnapshotGroupCriterion}, ConfigFile, PathList, RepoFile, SnapshotFile, SnapshotSummary, Tree, }, - repository::{warm_up::warm_up, warm_up::warm_up_wait}, + repository::warm_up::{warm_up, warm_up_wait}, vfs::OpenFile, - RepositoryBackends, RusticResult, + InMemoryBackend, RepositoryBackends, RusticResult, }; mod constants { @@ -635,7 +635,7 @@ impl Repository { if let Some(cache) = &cache { self.be = CachedBackend::from_backend(self.be.clone(), cache.clone()); - info!("using cache at {}", cache.location()); + info!("using cache at {cache}"); } else { info!("using no cache"); } @@ -1438,7 +1438,7 @@ impl Repository { /// // TODO: Document errors pub fn open_file(&self, node: &Node) -> RusticResult { - Ok(OpenFile::from_node(self, node)) + OpenFile::from_node(self, node) } /// Reads an opened file at the given position @@ -1632,7 +1632,7 @@ impl Repository { /// Merge the given trees. /// /// This method creates needed tree blobs within the repository. - /// Merge conflicts (identical filenames which do not match) will be resolved using the ordering given by `cmp`. + /// Merge conflicts (identical file names which do not match) will be resolved using the ordering given by `cmp`. /// /// # Arguments /// @@ -1659,7 +1659,7 @@ impl Repository { /// Merge the given snapshots. /// /// This method will create needed tree blobs within the repository. - /// Merge conflicts (identical filenames which do not match) will be resolved using the ordering given by `cmp`. + /// Merge conflicts (identical file names which do not match) will be resolved using the ordering given by `cmp`. /// /// # Arguments /// From 9362ab65ddfe33a3d90d14df16aa04110e03f6c9 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:14:53 +0100 Subject: [PATCH 56/58] make from_dir_node return option to bubble up subtree optional Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/vfs.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/core/src/vfs.rs b/crates/core/src/vfs.rs index cdf24266..5b2e79bb 100644 --- a/crates/core/src/vfs.rs +++ b/crates/core/src/vfs.rs @@ -195,9 +195,9 @@ impl Vfs { /// /// The created [`Vfs`] or `None` if the node has no subtree #[must_use] - pub fn from_dir_node(node: &Node) -> Self { - let tree = VfsTree::RusticTree(node.subtree.unwrap()); - Self { tree } + pub fn from_dir_node(node: &Node) -> Option { + let tree = VfsTree::RusticTree(node.subtree?); + Some(Self { tree }) } /// Create a new [`Vfs`] from a list of snapshots. From c87e9f66189e49230344adf6fbc56412f4ebd72e Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:15:19 +0100 Subject: [PATCH 57/58] handle errs in from_node Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/vfs.rs | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/crates/core/src/vfs.rs b/crates/core/src/vfs.rs index 5b2e79bb..aee07299 100644 --- a/crates/core/src/vfs.rs +++ b/crates/core/src/vfs.rs @@ -443,31 +443,43 @@ impl OpenFile { /// # Returns /// /// The created `OpenFile` - /// - /// # Panics - /// - /// Panics if the `Node` has no content - pub fn from_node(repo: &Repository, node: &Node) -> Self { + pub fn from_node( + repo: &Repository, + node: &Node, + ) -> RusticResult { let mut start = 0; - let mut content: Vec<_> = node + let mut content = node .content - .as_ref() - .unwrap() .iter() - .map(|id| { - let starts_at = start; - start += repo.index().get_data(id).unwrap().data_length() as usize; - BlobInfo { id: *id, starts_at } + .flatten() + .map(|id| -> RusticResult<_> { + let starts_at: usize = start; + let hex_id = id.to_hex(); + let hex_id = hex_id.as_str(); + let data_length = repo + .index() + .get_data(id) + .ok_or_else(|| VfsErrorKind::DataBlobNotFound(hex_id.to_string()))? + .data_length(); + let data_length = usize::try_from(data_length).map_err(|err| { + VfsErrorKind::ConversionFromU32ToUsizeFailed(err, hex_id.to_string()) + })?; + if let Some(val) = start.checked_add(data_length) { + start = val; + } else { + return Err(VfsErrorKind::DataBlobTooLarge(hex_id.to_string()).into()); + }; + Ok(BlobInfo { id: *id, starts_at }) }) - .collect(); + .collect::>>()?; - // content is assumed to be partioned, so we add a starts_at:MAX entry + // content is assumed to be partitioned, so we add a starts_at:MAX entry content.push(BlobInfo { id: Id::default(), starts_at: usize::MAX, }); - Self { content } + Ok(Self { content }) } /// Read the `OpenFile` at the given `offset` from the `repo`. From f1218c80f6a02347c91362007e924d5dcd451da1 Mon Sep 17 00:00:00 2001 From: simonsan <14062932+simonsan@users.noreply.github.com> Date: Sun, 17 Mar 2024 14:15:53 +0100 Subject: [PATCH 58/58] handle errs in dir_entries_from_path Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com> --- crates/core/src/vfs.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/core/src/vfs.rs b/crates/core/src/vfs.rs index aee07299..fa0ccc1b 100644 --- a/crates/core/src/vfs.rs +++ b/crates/core/src/vfs.rs @@ -364,8 +364,13 @@ impl Vfs { let result = match self.tree.get_path(path)? { VfsPath::RusticPath(tree_id, path) => { let node = repo.node_from_path(*tree_id, &path)?; - if node.is_dir() { - let tree = repo.get_tree(&node.subtree.unwrap())?; + if node.is_dir() && node.subtree.is_some() { + let Some(id) = node.subtree else { + return Err( + VfsErrorKind::NoDirectoryEntriesForSymlinkFound(path.into()).into() + ); + }; + let tree = repo.get_tree(&id)?; tree.nodes } else { Vec::new() @@ -373,14 +378,14 @@ impl Vfs { } VfsPath::VirtualTree(virtual_tree) => virtual_tree .iter() - .map(|(name, tree)| { + .map(|(name, tree)| -> RusticResult<_> { let node_type = match tree { VfsTree::Link(target) => NodeType::from_link(Path::new(target)), _ => NodeType::Dir, }; Node::from_type_and_metadata(name, node_type, Metadata::default()) }) - .collect(), + .collect::>>()?, VfsPath::Link(str) => { return Err(VfsErrorKind::NoDirectoryEntriesForSymlinkFound(str.clone()).into()); }