Commit 9caccb76 authored by SamwiseFilmore's avatar SamwiseFilmore
Browse files

Error cleanup

Most of this is just allowing "implicit" conversions between
Path/PathBuf and ErrorKind, enabling `.chain_err(|| path )` syntax as
opposed to `.chain_err(|| ErrorKind::Path(path.to_path_buf()) )`.
Reduced verbosity should help with readability, since that pattern is
used throughout the codebase.

Also took care of a couple TODOs
parent 0723ce95
......@@ -14,7 +14,6 @@ pub enum Error {
TryFromInt(core::num::TryFromIntError),
}
//TODO: Improve Error messages
impl Display for Error {
fn fmt(&self, f: &mut Formatter) -> Result {
use Error::*;
......
use std::io;
use std::path::PathBuf;
use std::path::{Path, PathBuf};
use error_chain::error_chain;
use user_error::UFE;
......@@ -47,44 +47,16 @@ error_chain! {
impl UFE for Error {}
/*
/// An error which includes path context and implements `UFE` for easy display.
#[derive(Debug, Error)]
#[error("File: {path}")]
pub struct Error {
#[source]
pub src: ErrorKind,
pub path: PathBuf,
// Allow .chain_err(|| path )
impl From<&Path> for ErrorKind {
fn from(path: &Path) -> ErrorKind {
ErrorKind::Path(path.to_path_buf())
}
}
/// The main error type that is used by this library internally. For additional
/// contextual information, most public routines use [`Error`](struct.Error.html).
#[non_exhaustive]
#[derive(Debug, Error)]
pub enum ErrorKind {
#[error("Io")]
Io(#[from] io::Error),
#[error("Key length invalid")]
KeyInvalid,
#[error("Public and secret keys do not match")]
KeyMismatch,
#[error("Invalid nonce length")]
NonceInvalid,
#[error("Incorrect passphrase")]
PassphraseIncorrect,
#[error("Passphrases did not match")]
PassphraseMismatch,
#[error("Serialization")]
Ser(#[from] toml::ser::Error),
#[error("Deserialization")]
Deser(#[from] toml::de::Error),
impl From<&PathBuf> for ErrorKind {
fn from(path: &PathBuf) -> ErrorKind {
ErrorKind::Path(path.clone())
}
}
*/
......@@ -85,10 +85,10 @@ impl PublicKeyFile {
/// Parse a `PublicKeyFile` from `file` (in toml format).
pub fn open(file: impl AsRef<Path>) -> Result<PublicKeyFile, Error> {
let content = fs::read_to_string(&file)
.chain_err(|| ErrorKind::Path(file.as_ref().to_path_buf()) )?;
.chain_err(|| file.as_ref() )?;
toml::from_str(&content)
.chain_err(|| ErrorKind::Path(file.as_ref().to_path_buf()) )
.chain_err(|| file.as_ref() )
}
/// Write `self` serialized as toml to `w`.
......@@ -101,8 +101,8 @@ impl PublicKeyFile {
pub fn save(&self, file: impl AsRef<Path>) -> Result<(), Error> {
self.write(
File::create(&file)
.chain_err(|| ErrorKind::Path(file.as_ref().to_path_buf()) )?
).chain_err(|| ErrorKind::Path(file.as_ref().to_path_buf()) )
.chain_err(|| file.as_ref() )?
).chain_err(|| file.as_ref() )
}
}
......@@ -206,10 +206,10 @@ impl SecretKeyFile {
/// Parse a `SecretKeyFile` from `file` (in toml format).
pub fn open(file: impl AsRef<Path>) -> Result<SecretKeyFile, Error> {
let content = fs::read_to_string(&file)
.chain_err(|| ErrorKind::Path(file.as_ref().to_path_buf()) )?;
.chain_err(|| file.as_ref() )?;
toml::from_str(&content)
.chain_err(|| ErrorKind::Path(file.as_ref().to_path_buf()) )
.chain_err(|| file.as_ref() )
}
/// Write `self` serialized as toml to `w`.
......@@ -229,8 +229,8 @@ impl SecretKeyFile {
.create(true)
.mode(0o600)
.open(&file)
.chain_err(|| ErrorKind::Path(file.as_ref().to_path_buf()) )?
).chain_err(|| ErrorKind::Path(file.as_ref().to_path_buf()) )
.chain_err(|| file.as_ref() )?
).chain_err(|| file.as_ref() )
}
/// Ensure that the internal state of this struct is encrypted.
......@@ -358,7 +358,7 @@ impl Eq for Passwd {}
/// directories will not be created.
pub fn gen_keypair(pkey_path: &Path, skey_path: &Path) -> Result<(PublicKeyFile, SecretKeyFile), Error> {
let passwd = Passwd::prompt_new()
.chain_err(|| ErrorKind::Path(skey_path.to_path_buf()) )?;
.chain_err(|| skey_path )?;
let (pkey_file, mut skey_file) = SecretKeyFile::new();
......@@ -376,9 +376,9 @@ fn prompt_skey(skey_path: &Path, prompt: impl AsRef<str>) -> Result<SecretKeyFil
if key_file.is_encrypted() {
let passwd = Passwd::prompt(&format!("{} {}: ", prompt.as_ref(), skey_path.display()))
.chain_err(|| ErrorKind::Path(skey_path.to_path_buf()) )?;
.chain_err(|| skey_path )?;
key_file.decrypt(passwd)
.chain_err(|| ErrorKind::Path(skey_path.to_path_buf()) )?;
.chain_err(|| skey_path )?;
}
Ok(key_file)
}
......@@ -395,7 +395,7 @@ pub fn re_encrypt(skey_path: &Path) -> Result<(), Error> {
let mut skey_file = prompt_skey(skey_path, "Old passphrase for")?;
let passwd = Passwd::prompt_new()
.chain_err(|| ErrorKind::Path(skey_path.to_path_buf()) )?;
.chain_err(|| skey_path )?;
skey_file.encrypt(passwd);
skey_file.save(skey_path)
......
use std::io;
use std::fs;
use std::path::PathBuf;
use std::path::{Path, PathBuf};
use std::process;
use clap::clap_app;
......@@ -54,7 +54,7 @@ fn cli() -> Result<i32, Error> {
"gen" => {
if let Some(keydir) = skey_path.parent() {
fs::create_dir_all(&keydir)
.chain_err(|| ErrorKind::Path(keydir.to_path_buf()) )?;
.chain_err(|| keydir )?;
}
if ! submatches.is_present("force") {
......@@ -62,7 +62,7 @@ fn cli() -> Result<i32, Error> {
return Err(Error::from_kind(ErrorKind::Io(
io::Error::from(io::ErrorKind::AlreadyExists)
)))
.chain_err(|| ErrorKind::Path(skey_path.to_path_buf()) );
.chain_err(|| &skey_path );
}
}
......@@ -87,7 +87,7 @@ fn cli() -> Result<i32, Error> {
pkey.save(file)?;
} else {
pkey.write(io::stdout().lock())
.chain_err(|| ErrorKind::Path(PathBuf::from("stdout")) )?;
.chain_err(|| Path::new("stdout") )?;
}
},
"rencrypt" => {
......
......@@ -94,12 +94,12 @@ pub fn create(
.create(true)
.truncate(true)
.open(&archive_path)
.chain_err(|| ErrorKind::Path(archive_path.as_ref().to_path_buf()) )?;
.chain_err(|| archive_path.as_ref() )?;
// Create a list of entries
let mut entries = Vec::new();
folder_entries(&folder, &folder, &mut entries)
.chain_err(|| ErrorKind::Path(folder.as_ref().to_path_buf()) )
.chain_err(|| folder.as_ref() )
.chain_err(|| "Recursing buildroot" )?;
// Create initial header
......@@ -117,13 +117,14 @@ pub fn create(
for entry in &mut entries {
entry.offset = data_size;
data_size = data_size.checked_add(entry.size)
.ok_or(pkgar_core::Error::Overflow)?;
//.chain_err(|| ErrorKind::Entry(*entry) )?;
.ok_or(pkgar_core::Error::Overflow)
.map_err(Error::from)
.chain_err(|| ErrorKind::Entry(*entry) )?;
}
let data_offset = header.total_size()?;
archive_file.seek(SeekFrom::Start(data_offset as u64))
.chain_err(|| ErrorKind::Path(archive_path.as_ref().to_path_buf()) )
.chain_err(|| archive_path.as_ref() )
.chain_err(|| format!("Seek to {} (data offset)", data_offset) )?;
//TODO: fallocate data_offset + data_size
......@@ -144,19 +145,19 @@ pub fn create(
let mut entry_file = fs::OpenOptions::new()
.read(true)
.open(&path)
.chain_err(|| ErrorKind::Path(path.to_path_buf()) )?;
.chain_err(|| &path )?;
copy_and_hash(&mut entry_file, &mut archive_file, &mut buf)
.chain_err(|| ErrorKind::Path(path.to_path_buf()) )
.chain_err(|| &path )
.chain_err(|| format!("Writing entry to archive: '{}'", relative.display()) )?
},
Mode::SYMLINK => {
let destination = fs::read_link(&path)
.chain_err(|| ErrorKind::Path(path.to_path_buf()) )?;
.chain_err(|| &path )?;
let mut data = destination.as_os_str().as_bytes();
copy_and_hash(&mut data, &mut archive_file, &mut buf)
.chain_err(|| ErrorKind::Path(path.to_path_buf()) )
.chain_err(|| &path )
.chain_err(|| format!("Writing entry to archive: '{}'", relative.display()) )?
},
_ => return Err(Error::from(
......@@ -182,12 +183,12 @@ pub fn create(
// Write archive header
archive_file.seek(SeekFrom::Start(0))
.chain_err(|| ErrorKind::Path(archive_path.as_ref().to_path_buf()) )?;
.chain_err(|| archive_path.as_ref() )?;
archive_file.write_all(unsafe {
plain::as_bytes(&header)
})
.chain_err(|| ErrorKind::Path(archive_path.as_ref().to_path_buf()) )?;
.chain_err(|| archive_path.as_ref() )?;
// Write each entry header
for entry in &entries {
......@@ -195,7 +196,7 @@ pub fn create(
archive_file.write_all(unsafe {
plain::as_bytes(entry)
})
.chain_err(|| ErrorKind::Path(archive_path.as_ref().to_path_buf()) )
.chain_err(|| archive_path.as_ref() )
.chain_err(|| format!("Write entry {}", checked_path.display()) )?;
}
......@@ -262,7 +263,7 @@ pub fn verify(
.join(entry.check_path()?);
let expected = File::open(&expected_path)
.chain_err(|| ErrorKind::Path(expected_path.to_path_buf()) )?;
.chain_err(|| &expected_path )?;
let (count, hash) = copy_and_hash(expected, io::sink(), &mut buf)?;
......
......@@ -67,7 +67,6 @@ pub trait PackageSrcExt
/// A reader that provides acess to one entry's data within a `PackageSrc`.
/// Use `PackageSrcExt::entry_reader` for construction
//TODO: Fix the types for this
pub struct EntryReader<'a, Src>
where Src: PackageSrc
{
......@@ -84,6 +83,8 @@ impl<Src, E> Read for EntryReader<'_, Src>
fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
let count = self.src.read_entry(self.entry, self.pos, buf)
// This is a little painful, since e is pkgar::Error...
// However, this is likely to be a very rarely triggered error
// condition.
.map_err(|err|
io::Error::new(io::ErrorKind::Other, err.to_string())
)?;
......
......@@ -67,6 +67,19 @@ error_chain! {
impl UFE for Error {}
// Allow .chain_err(|| path )
impl From<&Path> for ErrorKind {
fn from(path: &Path) -> ErrorKind {
ErrorKind::Path(path.to_path_buf())
}
}
impl From<&PathBuf> for ErrorKind {
fn from(path: &PathBuf) -> ErrorKind {
ErrorKind::Path(path.clone())
}
}
// Unfortunately error_chain does not handle types that don't implement
// std::error::Error very well.
impl From<pkgar_core::Error> for Error {
......@@ -75,11 +88,6 @@ impl From<pkgar_core::Error> for Error {
}
}
impl From<&Path> for ErrorKind {
fn from(path: &Path) -> ErrorKind {
ErrorKind::Path(path.to_path_buf())
}
}
/*
// Apparently this conflicts with the first implementation; Just use map_err
// with Error::from for chaining errors to a pkgar_core Result.
......@@ -94,154 +102,3 @@ impl<T> ResultExt<T> for Result<T, pkgar_core::Error> {
}
}*/
/*
/// This mimics the way std::io::Error works, to manage adding context in an
/// adequate manner, without too much boilderplate.
#[derive(Debug, Error)]
enum Repr {
/// pkgar_keys::Error is very high level and already contains path context
#[error(transparent)]
Keys(#[from] pkgar_keys::Error),
/// Ideally this should never make it all the way back to the user without
/// being converted into a Complex error.
#[error(transparent)]
Kind(ErrorKind),
#[error("{}", match (path, reason) {
(Some(path), Some(reason)) => format!("{}: {}", reason, path.display()),
(Some(path), None) => format!("File: {}", path.display()),
(None, Some(reason)) => reason.clone(),
(None, None) => String::new(),
})]
Complex {
path: Option<PathBuf>,
reason: Option<String>,
entry: Option<Entry>,
src: ErrorKind,
}
}
impl Repr {
fn as_complex(
self,
new_path: Option<PathBuf>,
new_reason: Option<String>,
new_entry: Option<Entry>,
) -> Repr {
match self {
Repr::Kind(src) => Repr::Complex {
path: new_path,
reason: new_reason,
entry: new_entry,
src,
},
Repr::Complex { path, reason, entry, src } => Repr::Complex {
path: new_path.or(path),
reason: new_reason.or(reason),
entry: new_entry.or(entry),
src,
},
_ => self,
}
}
}
/// Primary error type for pkgar. Provides optional path and reason context
/// for the underlying [`ErrorKind`](enum.ErrorKind.html).
#[derive(Debug, Error)]
#[error(transparent)]
pub struct Error {
#[from]
repr: Repr,
}
impl Error {
/// Set the path associated with this error. Calling `path()` multiple
/// times will only store the most recently added path.
///
/// Most pkgar APIs will have already called `path()`; therefore, it's
/// unlikely that consumers of the library will need to use this function.
pub fn path(mut self, path: impl AsRef<Path>) -> Error {
let path = Some(path.as_ref().to_path_buf());
self.repr = self.repr.as_complex(path, None, None);
self
}
/// Set the reason associated with this error. Calling `reason()` multiple
/// times will only store the most recently added reason.
///
/// Most pkgar APIs will have already called `reason()`; therefore, it's
/// unlikely that consumers of the library will need to use this function.
pub fn reason(mut self, reason: impl ToString) -> Error {
let reason = Some(reason.to_string());
self.repr = self.repr.as_complex(None, reason, None);
self
}
pub fn entry(mut self, entry: Entry) -> Error {
self.repr = self.repr.as_complex(None, None, Some(entry));
self
}
}
impl<K> From<K> for Error
where K: Into<ErrorKind>
{
fn from(e: K) -> Error {
Error::from(Repr::Kind(e.into()))
}
}
impl From<pkgar_keys::Error> for Error {
fn from(e: pkgar_keys::Error) -> Error {
Error::from(Repr::Keys(e))
}
}
/// Primary enumeration of error types producible by the pkgar crate. Most
/// interaction with this type will be via [`Error`](struct.Error.html).
#[derive(Debug, Error)]
#[non_exhaustive]
pub enum ErrorKind {
#[error("Io")]
Io(#[from] io::Error),
#[error("Failed to commit transaction. {changed} files changed, {remaining} files remaining")]
FailedCommit {
changed: usize,
remaining: usize,
#[source]
source: io::Error,
},
#[error("Package: {0}")]
Core(pkgar_core::Error),
#[error("Invalid path component: {0}")]
InvalidPathComponent(PathBuf),
#[error("Entry size mismatch: expected {expected}, got {actual}")]
LengthMismatch {
actual: u64,
expected: u64,
},
#[error("Invalid Mode Kind: {0:#o}")]
InvalidModeKind(Mode),
}
impl ErrorKind {
pub fn as_error(self) -> Error {
Error::from(self)
}
}
// Core::Error doesn't implement std::Error, so thiserror won't generate this impl
impl From<pkgar_core::Error> for ErrorKind {
fn from(err: pkgar_core::Error) -> ErrorKind {
ErrorKind::Core(err)
}
}
*/
......@@ -26,7 +26,7 @@ impl PackageFile {
let file = OpenOptions::new()
.read(true)
.open(&path)
.chain_err(|| path.as_path() )?;
.chain_err(|| &path )?;
let mut new = PackageFile {
path,
......
......@@ -46,11 +46,11 @@ fn temp_path(target_path: impl AsRef<Path>, entry_hash: Hash) -> Result<PathBuf,
hash_path
};
let parent = target_path.parent()
let parent_dir = target_path.parent()
.ok_or(ErrorKind::InvalidPathComponent(PathBuf::from("/")))?;
fs::create_dir_all(parent)
.chain_err(|| parent )?;
Ok(parent.join(tmp_name))
fs::create_dir_all(parent_dir)
.chain_err(|| parent_dir )?;
Ok(parent_dir.join(tmp_name))
}
enum Action {
......@@ -63,16 +63,16 @@ impl Action {
fn commit(&self) -> Result<(), Error> {
match self {
Action::Rename(tmp, target) => fs::rename(&tmp, target)
.chain_err(|| tmp.as_path() ),
.chain_err(|| tmp ),
Action::Remove(target) => fs::remove_file(&target)
.chain_err(|| target.as_path() ),
.chain_err(|| target ),
}
}
fn abort(&self) -> Result<(), Error> {
match self {
Action::Rename(tmp, _) => fs::remove_file(&tmp)
.chain_err(|| tmp.as_path() ),
.chain_err(|| tmp ),
Action::Remove(_) => Ok(()),
}
}
......@@ -122,13 +122,13 @@ impl Transaction {
.chain_err(|| tmp_path.as_path() )?;
copy_and_hash(src.entry_reader(entry), &mut tmp_file, &mut buf)
.chain_err(|| tmp_path.as_path() )
.chain_err(|| &tmp_path )
.chain_err(|| format!("Copying entry to tempfile: '{}'", relative_path.display()) )?
},
Mode::SYMLINK => {
let mut data = Vec::new();
let (size, hash) = copy_and_hash(src.entry_reader(entry), &mut data, &mut buf)
.chain_err(|| tmp_path.as_path() )
.chain_err(|| &tmp_path )
.chain_err(|| format!("Copying entry to tempfile: '{}'", relative_path.display()) )?;
let sym_target = Path::new(OsStr::from_bytes(&data));
......@@ -205,11 +205,11 @@ impl Transaction {
"target path was not in the base path");
let candidate = File::open(&target_path)
.chain_err(|| target_path.as_path() )?;
.chain_err(|| &target_path )?;
// Ensure that the deletion candidate on disk has not been modified
copy_and_hash(candidate, io::sink(), &mut buf)
.chain_err(|| target_path.as_path() )
.chain_err(|| &target_path )
.chain_err(|| format!("Hashing file for entry: '{}'", relative_path.display()) )?;
actions.push(Action::Remove(target_path));
......@@ -225,7 +225,9 @@ impl Transaction {
if let Err(err) = action.commit() {
// Should be possible to restart a failed transaction
self.actions.push(action);
return Err(err.chain_err(|| ErrorKind::FailedCommit(count, self.actions.len()) ));
return Err(err
.chain_err(|| ErrorKind::FailedCommit(count, self.actions.len()) )
);
}
count += 1;
}
......@@ -244,8 +246,10 @@ impl Transaction {
// This is inherently inefficent, no biggie
self.actions.insert(0, action);
if last_failed {
//TODO: Somehow indicate that this is a failed abort instead of a commit
return Err(err.chain_err(|| ErrorKind::FailedCommit(count, self.actions.len()) ));
return Err(err
.chain_err(|| ErrorKind::FailedCommit(count, self.actions.len()) )
.chain_err(|| "Abort triggered" )
);
} else {
last_failed = true;
}
......
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment