flashrom_tester: Refactor Error type

Use a type implementing Error instead of a string for errors. Error
implements Display so can be easily converted to a String. This will
allow libflashrom to be more easily integrated.

BUG=b:230545739
BRANCH=None
TEST=cargo test

Change-Id: Id166053c7edfd07576e7823692cfa0ea4d438948
Signed-off-by: Evan Benn <evanbenn@chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/65277
Reviewed-by: Peter Marheine <pmarheine@chromium.org>
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Edward O'Callaghan <quasisec@chromium.org>

util/flashrom_tester/flashrom/src/cmd.rs

@@ -86,10 +86,7 @@ fn flashrom_extract_size(stdout: &str) -> Result<i64, FlashromError> {
     {
         None => return Err("Found no purely-numeric lines in flashrom output".into()),
         Some(Err(e)) => {
-            return Err(format!(
-                "Failed to parse flashrom size output as integer: {}",
-                e
-            ))
+            return Err(format!("Failed to parse flashrom size output as integer: {}", e).into())
         }
         Some(Ok(sz)) => Ok(sz),
     }
@@ -246,7 +243,7 @@ impl crate::Flashrom for FlashromCmd {
                 info!("Successfully {}abled write-protect", status);
                 Ok(true)
             }
-            Err(e) => Err(format!("Cannot {}able write-protect: {}", status, e)),
+            Err(e) => Err(format!("Cannot {}able write-protect: {}", status, e).into()),
         }
     }
 
@@ -406,7 +403,7 @@ fn flashrom_dispatch<S: AsRef<str>>(
 
     let output = match Command::new(path).args(&args).output() {
         Ok(x) => x,
-        Err(e) => return Err(format!("Failed to run flashrom: {}", e)),
+        Err(e) => return Err(format!("Failed to run flashrom: {}", e).into()),
     };
     if !output.status.success() {
         // There is two cases on failure;
@@ -418,7 +415,8 @@ fn flashrom_dispatch<S: AsRef<str>>(
                     "{}\nExited with error code: {}",
                     String::from_utf8_lossy(&output.stderr),
                     code
-                ));
+                )
+                .into());
             }
             None => return Err("Process terminated by a signal".into()),
         }
@@ -444,7 +442,7 @@ pub fn dut_ctrl_servo_type() -> Result<(Vec<u8>, Vec<u8>), FlashromError> {
 fn dut_ctrl(args: &[&str]) -> Result<(Vec<u8>, Vec<u8>), FlashromError> {
     let output = match Command::new("dut-control").args(args).output() {
         Ok(x) => x,
-        Err(e) => return Err(format!("Failed to run dut-control: {}", e)),
+        Err(e) => return Err(format!("Failed to run dut-control: {}", e).into()),
     };
     if !output.status.success() {
         // There is two cases on failure;

util/flashrom_tester/flashrom/src/lib.rs

@@ -38,6 +38,8 @@ extern crate log;
 
 mod cmd;
 
+use std::{error, fmt};
+
 pub use cmd::{dut_ctrl_toggle_wp, FlashromCmd};
 
 #[derive(Copy, Clone, PartialEq, Debug)]
@@ -81,7 +83,27 @@ impl FlashChip {
     }
 }
 
-pub type FlashromError = String;
+#[derive(Debug, PartialEq)]
+pub struct FlashromError {
+    msg: String,
+}
+
+impl fmt::Display for FlashromError {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(f, "{}", self.msg)
+    }
+}
+
+impl error::Error for FlashromError {}
+
+impl<T> From<T> for FlashromError
+where
+    T: Into<String>,
+{
+    fn from(msg: T) -> Self {
+        FlashromError { msg: msg.into() }
+    }
+}
 
 pub struct ROMWriteSpecifics<'a> {
     pub layout_file: Option<&'a str>,

util/flashrom_tester/src/tester.rs

@@ -36,6 +36,7 @@
 use super::rand_util;
 use super::types;
 use super::utils::{self, LayoutSizes};
+use flashrom::FlashromError;
 use flashrom::{FlashChip, Flashrom};
 use serde_json::json;
 use std::mem::MaybeUninit;
@@ -65,7 +66,7 @@ pub struct TestEnv<'a> {
 }
 
 impl<'a> TestEnv<'a> {
-    pub fn create(chip_type: FlashChip, cmd: &'a dyn Flashrom) -> Result<Self, String> {
+    pub fn create(chip_type: FlashChip, cmd: &'a dyn Flashrom) -> Result<Self, FlashromError> {
         let rom_sz = cmd.get_size()?;
         let out = TestEnv {
             chip_type: chip_type,
@@ -128,22 +129,25 @@ impl<'a> TestEnv<'a> {
 
     /// Do whatever is necessary to make the current Flash contents the same as they
     /// were at the start of testing.
-    pub fn ensure_golden(&mut self) -> Result<(), String> {
+    pub fn ensure_golden(&mut self) -> Result<(), FlashromError> {
         self.wp.set_hw(false)?.set_sw(false)?;
-        self.cmd.write(&self.original_flash_contents)
+        self.cmd.write(&self.original_flash_contents)?;
+        Ok(())
     }
 
     /// Attempt to erase the flash.
-    pub fn erase(&self) -> Result<(), String> {
-        self.cmd.erase()
+    pub fn erase(&self) -> Result<(), FlashromError> {
+        self.cmd.erase()?;
+        Ok(())
     }
 
     /// Verify that the current Flash contents are the same as the file at the given
     /// path.
     ///
     /// Returns Err if they are not the same.
-    pub fn verify(&self, contents_path: &str) -> Result<(), String> {
-        self.cmd.verify(contents_path)
+    pub fn verify(&self, contents_path: &str) -> Result<(), FlashromError> {
+        self.cmd.verify(contents_path)?;
+        Ok(())
     }
 }
 
@@ -200,7 +204,7 @@ impl<'a> WriteProtectState<'a, 'static> {
     ///
     /// Panics if there is already a live state derived from hardware. In such a situation the
     /// new state must be derived from the live one, or the live one must be dropped first.
-    pub fn from_hardware(cmd: &'a dyn Flashrom, fc: FlashChip) -> Result<Self, String> {
+    pub fn from_hardware(cmd: &'a dyn Flashrom, fc: FlashChip) -> Result<Self, FlashromError> {
         let mut lock = Self::get_liveness_lock()
             .lock()
             .expect("Somebody panicked during WriteProtectState init from hardware");
@@ -232,8 +236,9 @@ impl<'a> WriteProtectState<'a, 'static> {
     }
 
     /// Get the actual software write protect state.
-    fn get_sw(cmd: &dyn Flashrom) -> Result<bool, String> {
-        cmd.wp_status(true)
+    fn get_sw(cmd: &dyn Flashrom) -> Result<bool, FlashromError> {
+        let b = cmd.wp_status(true)?;
+        Ok(b)
     }
 }
 
@@ -247,7 +252,7 @@ impl<'a, 'p> WriteProtectState<'a, 'p> {
     }
 
     /// Set the software write protect.
-    pub fn set_sw(&mut self, enable: bool) -> Result<&mut Self, String> {
+    pub fn set_sw(&mut self, enable: bool) -> Result<&mut Self, FlashromError> {
         info!("request={}, current={}", enable, self.current.1);
         if self.current.1 != enable {
             self.cmd.wp_toggle(/* en= */ enable)?;
@@ -277,7 +282,7 @@ impl<'a, 'p> WriteProtectState<'a, 'p> {
     /// This is useful if you need to temporarily make a change to write protection:
     ///
     /// ```no_run
-    /// # fn main() -> Result<(), String> {
+    /// # fn main() -> Result<(), Box<dyn std::error::Error>> {
     /// # let cmd: flashrom::FlashromCmd = unimplemented!();
     /// let wp = flashrom_tester::tester::WriteProtectState::from_hardware(&cmd, flashrom::FlashChip::SERVO)?;
     /// {