From 80ae98bf87c57aa361a13c3dd925455fb67e3f03 Mon Sep 17 00:00:00 2001 From: Lars Wirzenius Date: Sat, 24 Apr 2021 08:18:56 +0300 Subject: feat: improve error messages All unclear error messages should now be clearer. For example, all the ones related to a file mention the file name and the attempted operation that failed. --- src/chunk.rs | 11 +++-- src/chunker.rs | 14 +++++-- src/client.rs | 117 +++++++++++++++++++++++++++++++++++------------------ src/cmd/restore.rs | 49 +++++++++++++++------- src/config.rs | 14 ++++--- src/fsentry.rs | 13 ++---- src/fsiter.rs | 12 +++--- src/generation.rs | 16 ++++---- src/server.rs | 12 +++--- 9 files changed, 161 insertions(+), 97 deletions(-) diff --git a/src/chunk.rs b/src/chunk.rs index a67ed8c..0eed38a 100644 --- a/src/chunk.rs +++ b/src/chunk.rs @@ -36,8 +36,11 @@ pub enum GenerationChunkError { #[error(transparent)] Utf8Error(#[from] std::str::Utf8Error), - #[error(transparent)] - SerdeJsonError(#[from] serde_json::Error), + #[error("failed to parse JSON: {0}")] + JsonParse(serde_json::Error), + + #[error("failed to serialize to JSON: {0}")] + JsonGenerate(serde_json::Error), } /// A result from a chunk operation. @@ -51,7 +54,7 @@ impl GenerationChunk { pub fn from_data_chunk(chunk: &DataChunk) -> GenerationChunkResult { let data = chunk.data(); let data = std::str::from_utf8(data)?; - Ok(serde_json::from_str(data)?) + serde_json::from_str(data).map_err(GenerationChunkError::JsonParse) } pub fn is_empty(&self) -> bool { @@ -67,7 +70,7 @@ impl GenerationChunk { } pub fn to_data_chunk(&self) -> GenerationChunkResult { - let json = serde_json::to_string(self)?; + let json = serde_json::to_string(self).map_err(GenerationChunkError::JsonGenerate)?; Ok(DataChunk::new(json.as_bytes().to_vec())) } } diff --git a/src/chunker.rs b/src/chunker.rs index f424833..eeeed8d 100644 --- a/src/chunker.rs +++ b/src/chunker.rs @@ -2,29 +2,32 @@ use crate::checksummer::sha256; use crate::chunk::DataChunk; use crate::chunkmeta::ChunkMeta; use std::io::prelude::*; +use std::path::{Path, PathBuf}; pub struct Chunker { chunk_size: usize, buf: Vec, + filename: PathBuf, handle: std::fs::File, } #[derive(Debug, thiserror::Error)] pub enum ChunkerError { - #[error(transparent)] - IoError(#[from] std::io::Error), + #[error("failed to read file {0}: {1}")] + FileRead(PathBuf, std::io::Error), } pub type ChunkerResult = Result; impl Chunker { - pub fn new(chunk_size: usize, handle: std::fs::File) -> Self { + pub fn new(chunk_size: usize, handle: std::fs::File, filename: &Path) -> Self { let mut buf = vec![]; buf.resize(chunk_size, 0); Self { chunk_size, buf, handle, + filename: filename.to_path_buf(), } } @@ -32,7 +35,10 @@ impl Chunker { let mut used = 0; loop { - let n = self.handle.read(&mut self.buf.as_mut_slice()[used..])?; + let n = self + .handle + .read(&mut self.buf.as_mut_slice()[used..]) + .map_err(|err| ChunkerError::FileRead(self.filename.to_path_buf(), err))?; used += n; if n == 0 || used == self.chunk_size { break; diff --git a/src/client.rs b/src/client.rs index 1b33372..0f8a72f 100644 --- a/src/client.rs +++ b/src/client.rs @@ -15,11 +15,11 @@ use reqwest::blocking::Client; use std::collections::HashMap; use std::fs::File; use std::io::prelude::*; -use std::path::Path; +use std::path::{Path, PathBuf}; #[derive(Debug, thiserror::Error)] pub enum ClientError { - #[error("Server successful response to creating chunk lacked chunk id")] + #[error("Server response claimed it had created a chunk, but lacked chunk id")] NoCreatedChunkId, #[error("Server does not have chunk {0}")] @@ -28,6 +28,12 @@ pub enum ClientError { #[error("Server does not have generation {0}")] GenerationNotFound(String), + #[error("Server response did not have a 'chunk-meta' header for chunk {0}")] + NoChunkMeta(ChunkId), + + #[error("Wrong checksum for chunk {0}, got {1}, expected {2}")] + WrongChecksum(ChunkId, String, String), + #[error(transparent)] GenerationChunkError(#[from] GenerationChunkError), @@ -37,26 +43,32 @@ pub enum ClientError { #[error(transparent)] ChunkerError(#[from] ChunkerError), - #[error("Server response did not have a 'chunk-meta' header for chunk {0}")] - NoChunkMeta(ChunkId), + #[error("couldn't convert response chunk-meta header to string: {0}")] + MetaHeaderToString(reqwest::header::ToStrError), - #[error("Wrong checksum for chunk {0}, got {1}, expected {2}")] - WrongChecksum(ChunkId, String, String), + #[error("error from reqwest library: {0}")] + ReqwestError(reqwest::Error), - #[error(transparent)] - ReqwestError(#[from] reqwest::Error), + #[error("lookup by chunk checksum failed: {0}")] + ChunkExists(reqwest::Error), - #[error(transparent)] - ReqwestToStrError(#[from] reqwest::header::ToStrError), + #[error("failed to parse JSON: {0}")] + JsonParse(serde_json::Error), - #[error(transparent)] - SerdeJsonError(#[from] serde_json::Error), + #[error("failed to generate JSON: {0}")] + JsonGenerate(serde_json::Error), - #[error(transparent)] - SerdeYamlError(#[from] serde_yaml::Error), + #[error("failed to parse YAML: {0}")] + YamlParse(serde_yaml::Error), - #[error(transparent)] - IoError(#[from] std::io::Error), + #[error("failed to open file {0}: {1}")] + FileOpen(PathBuf, std::io::Error), + + #[error("failed to create file {0}: {1}")] + FileCreate(PathBuf, std::io::Error), + + #[error("failed to write to file {0}: {1}")] + FileWrite(PathBuf, std::io::Error), } pub type ClientResult = Result; @@ -72,7 +84,8 @@ impl BackupClient { let config = config.config(); let client = Client::builder() .danger_accept_invalid_certs(!config.verify_tls_cert) - .build()?; + .build() + .map_err(ClientError::ReqwestError)?; Ok(Self { client, base_url: config.server_url.to_string(), @@ -110,8 +123,9 @@ impl BackupClient { fn read_file(&self, filename: &Path, size: usize) -> ClientResult> { info!("upload file {}", filename.display()); - let file = std::fs::File::open(filename)?; - let chunker = Chunker::new(size, file); + let file = std::fs::File::open(filename) + .map_err(|err| ClientError::FileOpen(filename.to_path_buf(), err))?; + let chunker = Chunker::new(size, file, filename); let chunk_ids = self.upload_new_file_chunks(chunker)?; Ok(chunk_ids) } @@ -130,17 +144,19 @@ impl BackupClient { .client .get(&self.chunks_url()) .query(&[("sha256", meta.sha256())]) - .build()?; + .build() + .map_err(ClientError::ReqwestError)?; - let res = self.client.execute(req)?; + let res = self.client.execute(req).map_err(ClientError::ChunkExists)?; debug!("has_chunk: status={}", res.status()); let has = if res.status() != 200 { debug!("has_chunk: error from server"); None } else { - let text = res.text()?; + let text = res.text().map_err(ClientError::ReqwestError)?; debug!("has_chunk: text={:?}", text); - let hits: HashMap = serde_json::from_str(&text)?; + let hits: HashMap = + serde_json::from_str(&text).map_err(ClientError::JsonParse)?; debug!("has_chunk: hits={:?}", hits); let mut iter = hits.iter(); if let Some((chunk_id, _)) = iter.next() { @@ -161,9 +177,10 @@ impl BackupClient { .post(&self.chunks_url()) .header("chunk-meta", meta.to_json()) .body(chunk.data().to_vec()) - .send()?; + .send() + .map_err(ClientError::ReqwestError)?; debug!("upload_chunk: res={:?}", res); - let res: HashMap = res.json()?; + let res: HashMap = res.json().map_err(ClientError::ReqwestError)?; let chunk_id = if let Some(chunk_id) = res.get("chunk_id") { debug!("upload_chunk: id={}", chunk_id); chunk_id.parse().unwrap() @@ -179,10 +196,11 @@ impl BackupClient { .client .post(&self.chunks_url()) .header("chunk-meta", meta.to_json()) - .body(serde_json::to_string(&gen)?) - .send()?; + .body(serde_json::to_string(&gen).map_err(ClientError::JsonGenerate)?) + .send() + .map_err(ClientError::ReqwestError)?; debug!("upload_chunk: res={:?}", res); - let res: HashMap = res.json()?; + let res: HashMap = res.json().map_err(ClientError::ReqwestError)?; let chunk_id = if let Some(chunk_id) = res.get("chunk_id") { debug!("upload_chunk: id={}", chunk_id); chunk_id.parse().unwrap() @@ -213,12 +231,20 @@ impl BackupClient { pub fn list_generations(&self) -> ClientResult { let url = format!("{}?generation=true", &self.chunks_url()); trace!("list_generations: url={:?}", url); - let req = self.client.get(&url).build()?; - let res = self.client.execute(req)?; + let req = self + .client + .get(&url) + .build() + .map_err(ClientError::ReqwestError)?; + let res = self + .client + .execute(req) + .map_err(ClientError::ReqwestError)?; debug!("list_generations: status={}", res.status()); - let body = res.bytes()?; + let body = res.bytes().map_err(ClientError::ReqwestError)?; debug!("list_generations: body={:?}", body); - let map: HashMap = serde_yaml::from_slice(&body)?; + let map: HashMap = + serde_yaml::from_slice(&body).map_err(ClientError::YamlParse)?; debug!("list_generations: map={:?}", map); let finished = map .iter() @@ -231,8 +257,15 @@ impl BackupClient { info!("fetch chunk {}", chunk_id); let url = format!("{}/{}", &self.chunks_url(), chunk_id); - let req = self.client.get(&url).build()?; - let res = self.client.execute(req)?; + let req = self + .client + .get(&url) + .build() + .map_err(ClientError::ReqwestError)?; + let res = self + .client + .execute(req) + .map_err(ClientError::ReqwestError)?; if res.status() != 200 { let err = ClientError::ChunkNotFound(chunk_id.to_string()); error!("fetching chunk {} failed: {}", chunk_id, err); @@ -246,12 +279,15 @@ impl BackupClient { error!("fetching chunk {} failed: {}", chunk_id, err); return Err(err); } - let meta = meta.unwrap().to_str()?; + let meta = meta + .unwrap() + .to_str() + .map_err(ClientError::MetaHeaderToString)?; debug!("fetching chunk {}: meta={:?}", chunk_id, meta); - let meta: ChunkMeta = serde_json::from_str(meta)?; + let meta: ChunkMeta = serde_json::from_str(meta).map_err(ClientError::JsonParse)?; debug!("fetching chunk {}: meta={:?}", chunk_id, meta); - let body = res.bytes()?; + let body = res.bytes().map_err(ClientError::ReqwestError)?; let body = body.to_vec(); let actual = sha256(&body); if actual != meta.sha256() { @@ -277,10 +313,13 @@ impl BackupClient { let gen = self.fetch_generation_chunk(gen_id)?; // Fetch the SQLite file, storing it in the named file. - let mut dbfile = File::create(&dbname)?; + let mut dbfile = File::create(&dbname) + .map_err(|err| ClientError::FileCreate(dbname.to_path_buf(), err))?; for id in gen.chunk_ids() { let chunk = self.fetch_chunk(id)?; - dbfile.write_all(chunk.data())?; + dbfile + .write_all(chunk.data()) + .map_err(|err| ClientError::FileWrite(dbname.to_path_buf(), err))?; } info!("downloaded generation to {}", dbname.display()); diff --git a/src/cmd/restore.rs b/src/cmd/restore.rs index 4de0830..01e96bf 100644 --- a/src/cmd/restore.rs +++ b/src/cmd/restore.rs @@ -96,14 +96,26 @@ pub enum RestoreError { #[error(transparent)] StripPrefixError(#[from] StripPrefixError), - #[error(transparent)] - IoError(#[from] std::io::Error), + #[error("failed to create directory {0}: {1}")] + CreateDirs(PathBuf, std::io::Error), - #[error(transparent)] - SerdeYamlError(#[from] serde_yaml::Error), + #[error("failed to create file {0}: {1}")] + CreateFile(PathBuf, std::io::Error), - #[error(transparent)] - NulError(#[from] std::ffi::NulError), + #[error("failed to write file {0}: {1}")] + WriteFile(PathBuf, std::io::Error), + + #[error("failed to create symbolic link {0}: {1}")] + Symlink(PathBuf, std::io::Error), + + #[error("failed to create UNIX domain socket {0}: {1}")] + UnixBind(PathBuf, std::io::Error), + + #[error("failed to set permissions for {0}: {1}")] + Chmod(PathBuf, std::io::Error), + + #[error("failed to set timestamp for {0}: {1}")] + SetTimestamp(PathBuf, std::io::Error), } pub type RestoreResult = Result; @@ -133,7 +145,8 @@ fn restore_generation( fn restore_directory(path: &Path) -> RestoreResult<()> { debug!("restoring directory {}", path.display()); - std::fs::create_dir_all(path)?; + std::fs::create_dir_all(path) + .map_err(|err| RestoreError::CreateDirs(path.to_path_buf(), err))?; Ok(()) } @@ -169,13 +182,16 @@ fn restore_regular( debug!("restoring regular {}", path.display()); let parent = path.parent().unwrap(); debug!(" mkdir {}", parent.display()); - std::fs::create_dir_all(parent)?; + std::fs::create_dir_all(parent) + .map_err(|err| RestoreError::CreateDirs(parent.to_path_buf(), err))?; { - let mut file = std::fs::File::create(path)?; + let mut file = std::fs::File::create(path) + .map_err(|err| RestoreError::CreateFile(path.to_path_buf(), err))?; for chunkid in gen.chunkids(fileid)?.iter()? { let chunkid = chunkid?; let chunk = client.fetch_chunk(&chunkid)?; - file.write_all(chunk.data())?; + file.write_all(chunk.data()) + .map_err(|err| RestoreError::WriteFile(path.to_path_buf(), err))?; } restore_metadata(path, entry)?; } @@ -188,16 +204,18 @@ fn restore_symlink(path: &Path, entry: &FilesystemEntry) -> RestoreResult<()> { let parent = path.parent().unwrap(); debug!(" mkdir {}", parent.display()); if !parent.exists() { - std::fs::create_dir_all(parent)?; + std::fs::create_dir_all(parent) + .map_err(|err| RestoreError::CreateDirs(parent.to_path_buf(), err))?; } - symlink(entry.symlink_target().unwrap(), path)?; + symlink(entry.symlink_target().unwrap(), path) + .map_err(|err| RestoreError::Symlink(path.to_path_buf(), err))?; debug!("restored symlink {}", path.display()); Ok(()) } fn restore_socket(path: &Path, entry: &FilesystemEntry) -> RestoreResult<()> { debug!("creating Unix domain socket {:?}", path); - UnixListener::bind(path)?; + UnixListener::bind(path).map_err(|err| RestoreError::UnixBind(path.to_path_buf(), err))?; restore_metadata(path, entry)?; Ok(()) } @@ -230,6 +248,7 @@ fn restore_metadata(path: &Path, entry: &FilesystemEntry) -> RestoreResult<()> { let times = [atime, mtime]; let times: *const timespec = ×[0]; + let pathbuf = path.to_path_buf(); let path = path_to_cstring(path); // We have to use unsafe here to be able call the libc functions @@ -239,14 +258,14 @@ fn restore_metadata(path: &Path, entry: &FilesystemEntry) -> RestoreResult<()> { if chmod(path.as_ptr(), entry.mode()) == -1 { let error = Error::last_os_error(); error!("chmod failed on {:?}", path); - return Err(error.into()); + return Err(RestoreError::Chmod(pathbuf, error)); } debug!("utimens {:?}", path); if utimensat(AT_FDCWD, path.as_ptr(), times, 0) == -1 { let error = Error::last_os_error(); error!("utimensat failed on {:?}", path); - return Err(error.into()); + return Err(RestoreError::SetTimestamp(pathbuf, error)); } } Ok(()) diff --git a/src/config.rs b/src/config.rs index b30cfa3..33e08a2 100644 --- a/src/config.rs +++ b/src/config.rs @@ -77,11 +77,11 @@ pub enum ClientConfigError { #[error("No passwords are set: you may need to run 'obnam init': {0}")] PasswordsMissing(PasswordError), - #[error(transparent)] - IoError(#[from] std::io::Error), + #[error("failed to read configuration file {0}: {1}")] + Read(PathBuf, std::io::Error), - #[error(transparent)] - SerdeYamlError(#[from] serde_yaml::Error), + #[error("failed to parse configuration file {0} as YAML: {1}")] + YamlParse(PathBuf, serde_yaml::Error), } pub type ClientConfigResult = Result; @@ -89,8 +89,10 @@ pub type ClientConfigResult = Result; impl ClientConfigWithoutPasswords { pub fn read_config(filename: &Path) -> ClientConfigResult { trace!("read_config: filename={:?}", filename); - let config = std::fs::read_to_string(filename)?; - let tentative: TentativeClientConfig = serde_yaml::from_str(&config)?; + let config = std::fs::read_to_string(filename) + .map_err(|err| ClientConfigError::Read(filename.to_path_buf(), err))?; + let tentative: TentativeClientConfig = serde_yaml::from_str(&config) + .map_err(|err| ClientConfigError::YamlParse(filename.to_path_buf(), err))?; let roots = tentative .roots .iter() diff --git a/src/fsentry.rs b/src/fsentry.rs index 35931ab..3f532cc 100644 --- a/src/fsentry.rs +++ b/src/fsentry.rs @@ -52,8 +52,8 @@ pub enum FsEntryError { #[error("Unknown file kind {0}")] UnknownFileKindCode(u8), - #[error(transparent)] - IoError(#[from] std::io::Error), + #[error("failed to read symbolic link target {0}: {1}")] + ReadLink(PathBuf, std::io::Error), } pub type FsEntryResult = Result; @@ -64,13 +64,8 @@ impl FilesystemEntry { let kind = FilesystemKind::from_file_type(meta.file_type()); let symlink_target = if kind == FilesystemKind::Symlink { debug!("reading symlink target for {:?}", path); - let target = match read_link(path) { - Ok(x) => x, - Err(err) => { - error!("read_link failed: {}", err); - return Err(err.into()); - } - }; + let target = + read_link(path).map_err(|err| FsEntryError::ReadLink(path.to_path_buf(), err))?; Some(target) } else { None diff --git a/src/fsiter.rs b/src/fsiter.rs index 6c18404..56630fa 100644 --- a/src/fsiter.rs +++ b/src/fsiter.rs @@ -10,11 +10,11 @@ pub struct FsIterator { #[derive(Debug, thiserror::Error)] pub enum FsIterError { - #[error(transparent)] - WalkError(#[from] walkdir::Error), + #[error("walkdir failed: {0}")] + WalkDir(walkdir::Error), - #[error("I/O error on {0}: {1}")] - IoError(PathBuf, #[source] std::io::Error), + #[error("failed to get file system metadata for {0}: {1}")] + Metadata(PathBuf, std::io::Error), #[error(transparent)] FsEntryError(#[from] FsEntryError), @@ -110,7 +110,7 @@ impl Iterator for SkipCachedirs { debug!("walkdir found: {:?}", next); match next { None => None, - Some(Err(err)) => Some(Err(err.into())), + Some(Err(err)) => Some(Err(FsIterError::WalkDir(err))), Some(Ok(entry)) => { self.try_enqueue_cachedir_tag(&entry); Some(new_entry(entry.path())) @@ -127,7 +127,7 @@ fn new_entry(path: &Path) -> FsIterResult { Ok(meta) => meta, Err(err) => { warn!("failed to get metadata for {}: {}", path.display(), err); - return Err(FsIterError::IoError(path.to_path_buf(), err)); + return Err(FsIterError::Metadata(path.to_path_buf(), err)); } }; let entry = FilesystemEntry::from_metadata(path, &meta)?; diff --git a/src/generation.rs b/src/generation.rs index 0055bfe..85af1f5 100644 --- a/src/generation.rs +++ b/src/generation.rs @@ -27,11 +27,11 @@ pub enum NascentError { #[error(transparent)] BackupError(#[from] BackupError), - #[error(transparent)] - RusqliteError(#[from] rusqlite::Error), + #[error("SQL transaction error: {0}")] + Transaction(rusqlite::Error), - #[error(transparent)] - IoError(#[from] std::io::Error), + #[error("SQL commit error: {0}")] + Commit(rusqlite::Error), } pub type NascentResult = Result; @@ -55,10 +55,10 @@ impl NascentGeneration { ids: &[ChunkId], reason: Reason, ) -> NascentResult<()> { - let t = self.conn.transaction()?; + let t = self.conn.transaction().map_err(NascentError::Transaction)?; self.fileno += 1; sql::insert_one(&t, e, self.fileno, ids, reason)?; - t.commit()?; + t.commit().map_err(NascentError::Commit)?; Ok(()) } @@ -66,7 +66,7 @@ impl NascentGeneration { &mut self, entries: impl Iterator, Reason)>>, ) -> NascentResult> { - let t = self.conn.transaction()?; + let t = self.conn.transaction().map_err(NascentError::Transaction)?; let mut warnings = vec![]; for r in entries { match r { @@ -80,7 +80,7 @@ impl NascentGeneration { } } } - t.commit()?; + t.commit().map_err(NascentError::Commit)?; Ok(warnings) } } diff --git a/src/server.rs b/src/server.rs index 6ea8ac4..3b0584f 100644 --- a/src/server.rs +++ b/src/server.rs @@ -29,20 +29,20 @@ pub enum ServerConfigError { #[error("server address can't be resolved")] BadServerAddress, - #[error("I/O error for {0}: {1}")] - IoError(PathBuf, #[source] std::io::Error), + #[error("failed to read configuration file {0}: {1}")] + Read(PathBuf, std::io::Error), - #[error(transparent)] - SerdeYamlError(#[from] serde_yaml::Error), + #[error("failed to parse configuration file as YAML: {0}")] + YamlParse(serde_yaml::Error), } impl ServerConfig { pub fn read_config(filename: &Path) -> Result { let config = match std::fs::read_to_string(filename) { Ok(config) => config, - Err(err) => return Err(ServerConfigError::IoError(filename.to_path_buf(), err)), + Err(err) => return Err(ServerConfigError::Read(filename.to_path_buf(), err)), }; - let config: Self = serde_yaml::from_str(&config)?; + let config: Self = serde_yaml::from_str(&config).map_err(ServerConfigError::YamlParse)?; config.check()?; Ok(config) } -- cgit v1.2.1