From 10d078b84394543cddcd79f6aa649b7e51e07f52 Mon Sep 17 00:00:00 2001
From: Oliver Hamlet <oliver.hamlet@gmail.com>
Date: Sat, 6 Oct 2018 21:29:07 +0100
Subject: [PATCH] Improve content of I/O and PE parsing errors

---
 src/function/eval.rs | 12 ++++++++----
 src/lib.rs           | 25 +++++++++++++------------
 src/version.rs       | 25 ++++++++++++++++++++++---
 3 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/src/function/eval.rs b/src/function/eval.rs
index 78d1a16..0b9a6a8 100644
--- a/src/function/eval.rs
+++ b/src/function/eval.rs
@@ -59,7 +59,8 @@ fn evaluate_file_regex(state: &State, parent_path: &Path, regex: &Regex) -> Resu
     };
 
     for entry in dir_iterator {
-        if is_match(regex, &entry?.file_name()) {
+        let entry = entry.map_err(|e| Error::IoError(parent_path.to_path_buf(), e))?;
+        if is_match(regex, &entry.file_name()) {
             return Ok(true);
         }
     }
@@ -75,7 +76,8 @@ fn evaluate_many(state: &State, parent_path: &Path, regex: &Regex) -> Result<boo
 
     let mut found_one = false;
     for entry in dir_iterator {
-        if is_match(regex, &entry?.file_name()) {
+        let entry = entry.map_err(|e| Error::IoError(parent_path.to_path_buf(), e))?;
+        if is_match(regex, &entry.file_name()) {
             if found_one {
                 return Ok(true);
             } else {
@@ -132,11 +134,13 @@ fn evaluate_checksum(state: &State, file_path: &Path, crc: u32) -> Result<bool,
         return Ok(false);
     }
 
-    let reader = BufReader::new(File::open(path)?);
+    let file = File::open(path).map_err(|e| Error::IoError(file_path.to_path_buf(), e))?;
+    let reader = BufReader::new(file);
     let mut digest = crc32::Digest::new(crc32::IEEE);
 
     for byte in reader.bytes() {
-        digest.write_u8(byte?);
+        let byte = byte.map_err(|e| Error::IoError(file_path.to_path_buf(), e))?;
+        digest.write_u8(byte);
     }
 
     let calculated_crc = digest.sum32();
diff --git a/src/lib.rs b/src/lib.rs
index e14ad93..4db7a78 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -28,8 +28,8 @@ use function::Function;
 pub enum Error {
     ParsingIncomplete,
     ParsingError,
-    PeParsingError,
-    IoError(io::Error),
+    PeParsingError(PathBuf, Box<error::Error>),
+    IoError(PathBuf, io::Error),
 }
 
 impl<I> From<Err<I>> for Error {
@@ -41,12 +41,6 @@ impl<I> From<Err<I>> for Error {
     }
 }
 
-impl From<io::Error> for Error {
-    fn from(error: io::Error) -> Self {
-        Error::IoError(error)
-    }
-}
-
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
@@ -54,11 +48,18 @@ impl fmt::Display for Error {
             Error::ParsingError => {
                 write!(f, "An error was encountered while parsing the expression")
             }
-            Error::PeParsingError => write!(
+            Error::PeParsingError(p, e) => write!(
                 f,
-                "An error was encountered while reading the version of an executable"
+                "An error was encountered while reading the file version field of \"{}\": {}",
+                p.display(),
+                e
+            ),
+            Error::IoError(p, e) => write!(
+                f,
+                "An error was encountered while accessing the path \"{}\": {}",
+                p.display(),
+                e
             ),
-            Error::IoError(e) => e.fmt(f),
         }
     }
 }
@@ -66,7 +67,7 @@ impl fmt::Display for Error {
 impl error::Error for Error {
     fn cause(&self) -> Option<&error::Error> {
         match self {
-            Error::IoError(e) => Some(e),
+            Error::IoError(_, e) => Some(e),
             _ => None,
         }
     }
diff --git a/src/version.rs b/src/version.rs
index a13c2ef..d8e218c 100644
--- a/src/version.rs
+++ b/src/version.rs
@@ -29,9 +29,10 @@ pub struct Version {
 
 impl Version {
     pub fn read_file_version(file_path: &Path) -> Result<Self, Error> {
-        let file_map = FileMap::open(file_path)?;
-        let version_info =
-            get_pe_version_info(file_map.as_ref()).map_err(|_| Error::PeParsingError)?;
+        let file_map =
+            FileMap::open(file_path).map_err(|e| Error::IoError(file_path.to_path_buf(), e))?;
+        let version_info = get_pe_version_info(file_map.as_ref())
+            .map_err(|e| Error::PeParsingError(file_path.to_path_buf(), e.into()))?;
 
         if let Some(fixed_file_info) = version_info.fixed() {
             let version = format!(
@@ -181,6 +182,24 @@ mod tests {
             assert!(version.pre_release_ids.is_empty());
         }
 
+        #[test]
+        fn version_read_file_version_should_error_with_path_if_path_does_not_exist() {
+            let error = Version::read_file_version(Path::new("missing")).unwrap_err();
+
+            assert!(
+                error
+                    .to_string()
+                    .starts_with("An error was encountered while accessing the path \"missing\":")
+            );
+        }
+
+        #[test]
+        fn version_read_file_version_should_error_with_path_if_the_file_is_not_an_executable() {
+            let error = Version::read_file_version(Path::new("Cargo.toml")).unwrap_err();
+
+            assert_eq!("An error was encountered while reading the file version field of \"Cargo.toml\": bad magic", error.to_string());
+        }
+
         #[test]
         fn version_eq_an_empty_string_should_equal_an_empty_string() {
             assert_eq!(Version::from(""), Version::from(""));