flashrom_tester: Use path types for things that are paths

Use Path and PathBuf for things that are paths.

BUG=b:243460685
BRANCH=None
TEST=/usr/bin/flashrom_tester --flashrom_binary /usr/sbin/flashrom host
TEST=/usr/bin/flashrom_tester --libflashrom host

Change-Id: I69531bec5436a60430eae975eeab02c8835962bf
Signed-off-by: Evan Benn <evanbenn@chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/69064
Tested-by: build bot (Jenkins) <no-reply@coreboot.org>
Reviewed-by: Angel Pons <th3fanbus@gmail.com>
Reviewed-by: Edward O'Callaghan <quasisec@chromium.org>

util/flashrom_tester/flashrom/src/cmd.rs

@@ -35,14 +35,18 @@
 
 use crate::{FlashChip, FlashromError, ROMWriteSpecifics};
 
-use std::process::Command;
+use std::{
+    ffi::{OsStr, OsString},
+    path::Path,
+    process::Command,
+};
 
 #[derive(Default)]
 pub struct FlashromOpt<'a> {
     pub wp_opt: WPOpt,
     pub io_opt: IOOpt<'a>,
 
-    pub layout: Option<&'a str>, // -l <file>
+    pub layout: Option<&'a Path>, // -l <file>
     pub image: Option<&'a str>,   // -i <name>
 
     pub flash_name: bool, // --flash-name
@@ -60,11 +64,11 @@ pub struct WPOpt {
 
 #[derive(Default)]
 pub struct IOOpt<'a> {
-    pub read: Option<&'a str>,              // -r <file>
+    pub read: Option<&'a Path>,              // -r <file>
-    pub write: Option<&'a str>,             // -w <file>
+    pub write: Option<&'a Path>,             // -w <file>
-    pub verify: Option<&'a str>,            // -v <file>
+    pub verify: Option<&'a Path>,            // -v <file>
     pub erase: bool,                         // -E
-    pub region: Option<(&'a str, &'a str)>, // --image
+    pub region: Option<(&'a str, &'a Path)>, // --image
 }
 
 #[derive(PartialEq, Eq, Debug)]
@@ -227,7 +231,7 @@ impl crate::Flashrom for FlashromCmd {
         }
     }
 
-    fn read_into_file(&self, path: &str) -> Result<(), FlashromError> {
+    fn read_into_file(&self, path: &Path) -> Result<(), FlashromError> {
         let opts = FlashromOpt {
             io_opt: IOOpt {
                 read: Some(path),
@@ -240,7 +244,7 @@ impl crate::Flashrom for FlashromCmd {
         Ok(())
     }
 
-    fn read_region_into_file(&self, path: &str, region: &str) -> Result<(), FlashromError> {
+    fn read_region_into_file(&self, path: &Path, region: &str) -> Result<(), FlashromError> {
         let opts = FlashromOpt {
             io_opt: IOOpt {
                 region: Some((region, path)),
@@ -253,7 +257,7 @@ impl crate::Flashrom for FlashromCmd {
         Ok(())
     }
 
-    fn write_from_file(&self, path: &str) -> Result<(), FlashromError> {
+    fn write_from_file(&self, path: &Path) -> Result<(), FlashromError> {
         let opts = FlashromOpt {
             io_opt: IOOpt {
                 write: Some(path),
@@ -266,7 +270,7 @@ impl crate::Flashrom for FlashromCmd {
         Ok(())
     }
 
-    fn verify_from_file(&self, path: &str) -> Result<(), FlashromError> {
+    fn verify_from_file(&self, path: &Path) -> Result<(), FlashromError> {
         let opts = FlashromOpt {
             io_opt: IOOpt {
                 verify: Some(path),
@@ -297,8 +301,8 @@ impl crate::Flashrom for FlashromCmd {
     }
 }
 
-fn flashrom_decode_opts(opts: FlashromOpt) -> Vec<String> {
+fn flashrom_decode_opts(opts: FlashromOpt) -> Vec<OsString> {
-    let mut params = Vec::<String>::new();
+    let mut params = Vec::<OsString>::new();
 
     // ------------ WARNING !!! ------------
     // each param must NOT contain spaces!
@@ -307,58 +311,62 @@ fn flashrom_decode_opts(opts: FlashromOpt) -> Vec<String> {
     // wp_opt
     if opts.wp_opt.range.is_some() {
         let (x0, x1) = opts.wp_opt.range.unwrap();
-        params.push("--wp-range".to_string());
+        params.push("--wp-range".into());
-        params.push(hex_range_string(x0, x1));
+        params.push(hex_range_string(x0, x1).into());
     }
     if opts.wp_opt.status {
-        params.push("--wp-status".to_string());
+        params.push("--wp-status".into());
     } else if opts.wp_opt.list {
-        params.push("--wp-list".to_string());
+        params.push("--wp-list".into());
     } else if opts.wp_opt.enable {
-        params.push("--wp-enable".to_string());
+        params.push("--wp-enable".into());
     } else if opts.wp_opt.disable {
-        params.push("--wp-disable".to_string());
+        params.push("--wp-disable".into());
     }
 
     // io_opt
     if let Some((region, path)) = opts.io_opt.region {
-        params.push("--image".to_string());
+        params.push("--image".into());
-        params.push(format!("{}:{}", region, path));
+        let mut p = OsString::new();
-        params.push("-r".to_string());
+        p.push(region);
+        p.push(":");
+        p.push(path);
+        params.push(p);
+        params.push("-r".into());
     } else if opts.io_opt.read.is_some() {
-        params.push("-r".to_string());
+        params.push("-r".into());
-        params.push(opts.io_opt.read.unwrap().to_string());
+        params.push(opts.io_opt.read.unwrap().into());
     } else if opts.io_opt.write.is_some() {
-        params.push("-w".to_string());
+        params.push("-w".into());
-        params.push(opts.io_opt.write.unwrap().to_string());
+        params.push(opts.io_opt.write.unwrap().into());
     } else if opts.io_opt.verify.is_some() {
-        params.push("-v".to_string());
+        params.push("-v".into());
-        params.push(opts.io_opt.verify.unwrap().to_string());
+        params.push(opts.io_opt.verify.unwrap().into());
     } else if opts.io_opt.erase {
-        params.push("-E".to_string());
+        params.push("-E".into());
     }
 
     // misc_opt
     if opts.layout.is_some() {
-        params.push("-l".to_string());
+        params.push("-l".into());
-        params.push(opts.layout.unwrap().to_string());
+        params.push(opts.layout.unwrap().into());
     }
     if opts.image.is_some() {
-        params.push("-i".to_string());
+        params.push("-i".into());
-        params.push(opts.image.unwrap().to_string());
+        params.push(opts.image.unwrap().into());
     }
 
     if opts.flash_name {
-        params.push("--flash-name".to_string());
+        params.push("--flash-name".into());
     }
     if opts.verbose {
-        params.push("-V".to_string());
+        params.push("-V".into());
     }
 
     params
 }
 
-fn flashrom_dispatch<S: AsRef<str>>(
+fn flashrom_dispatch<S: AsRef<OsStr>>(
     path: &str,
     params: &[S],
     fc: FlashChip,
@@ -366,7 +374,7 @@ fn flashrom_dispatch<S: AsRef<str>>(
 ) -> Result<(String, String), FlashromError> {
     // from man page:
     //  ' -p, --programmer <name>[:parameter[,parameter[,parameter]]] '
-    let mut args: Vec<&str> = vec!["-p", FlashChip::to(fc)];
+    let mut args: Vec<&OsStr> = vec![OsStr::new("-p"), OsStr::new(FlashChip::to(fc))];
     args.extend(params.iter().map(S::as_ref));
 
     info!("flashrom_dispatch() running: {} {:?}", path, args);
@@ -454,6 +462,8 @@ fn extract_flash_name(stdout: &str) -> Option<(&str, &str)> {
 
 #[cfg(test)]
 mod tests {
+    use std::path::Path;
+
     use super::flashrom_decode_opts;
     use super::{FlashromOpt, IOOpt, WPOpt};
 
@@ -515,21 +525,21 @@ mod tests {
 
         test_io_opt(
             IOOpt {
-                read: Some("foo.bin"),
+                read: Some(Path::new("foo.bin")),
                 ..Default::default()
             },
             &["-r", "foo.bin"],
         );
         test_io_opt(
             IOOpt {
-                write: Some("bar.bin"),
+                write: Some(Path::new("bar.bin")),
                 ..Default::default()
             },
             &["-w", "bar.bin"],
         );
         test_io_opt(
             IOOpt {
-                verify: Some("/tmp/baz.bin"),
+                verify: Some(Path::new("/tmp/baz.bin")),
                 ..Default::default()
             },
             &["-v", "/tmp/baz.bin"],
@@ -548,7 +558,7 @@ mod tests {
         //use Default::default;
         assert_eq!(
             flashrom_decode_opts(FlashromOpt {
-                layout: Some("TestLayout"),
+                layout: Some(Path::new("TestLayout")),
                 ..Default::default()
             }),
             &["-l", "TestLayout"]

util/flashrom_tester/flashrom/src/flashromlib.rs

@@ -34,7 +34,7 @@
 
 use libflashrom::{Chip, Programmer};
 
-use std::{cell::RefCell, convert::TryFrom, fs};
+use std::{cell::RefCell, convert::TryFrom, fs, path::Path};
 
 use crate::{FlashChip, FlashromError, ROMWriteSpecifics};
 
@@ -108,13 +108,13 @@ impl crate::Flashrom for FlashromLib {
         self.wp_range((0, self.get_size()?), en)
     }
 
-    fn read_into_file(&self, path: &str) -> Result<(), FlashromError> {
+    fn read_into_file(&self, path: &Path) -> Result<(), FlashromError> {
         let buf = self.flashrom.borrow_mut().image_read(None)?;
         fs::write(path, buf).map_err(|error| error.to_string())?;
         Ok(())
     }
 
-    fn read_region_into_file(&self, path: &str, region: &str) -> Result<(), FlashromError> {
+    fn read_region_into_file(&self, path: &Path, region: &str) -> Result<(), FlashromError> {
         let mut layout = self.flashrom.borrow_mut().layout_read_fmap_from_rom()?;
         layout.include_region(region)?;
         let range = layout.get_region_range(region)?;
@@ -123,7 +123,7 @@ impl crate::Flashrom for FlashromLib {
         Ok(())
     }
 
-    fn write_from_file(&self, path: &str) -> Result<(), FlashromError> {
+    fn write_from_file(&self, path: &Path) -> Result<(), FlashromError> {
         let mut buf = fs::read(path).map_err(|error| error.to_string())?;
         self.flashrom.borrow_mut().image_write(&mut buf, None)?;
         Ok(())
@@ -143,7 +143,7 @@ impl crate::Flashrom for FlashromLib {
         Ok(true)
     }
 
-    fn verify_from_file(&self, path: &str) -> Result<(), FlashromError> {
+    fn verify_from_file(&self, path: &Path) -> Result<(), FlashromError> {
         let buf = fs::read(path).map_err(|error| error.to_string())?;
         self.flashrom.borrow_mut().image_verify(&buf, None)?;
         Ok(())

util/flashrom_tester/flashrom/src/lib.rs

@@ -39,7 +39,7 @@ extern crate log;
 mod cmd;
 mod flashromlib;
 
-use std::{error, fmt};
+use std::{error, fmt, path::Path};
 
 pub use cmd::{dut_ctrl_toggle_wp, FlashromCmd};
 pub use flashromlib::FlashromLib;
@@ -118,8 +118,8 @@ where
 }
 
 pub struct ROMWriteSpecifics<'a> {
-    pub layout_file: Option<&'a str>,
+    pub layout_file: Option<&'a Path>,
-    pub write_file: Option<&'a str>,
+    pub write_file: Option<&'a Path>,
     pub name_file: Option<&'a str>,
 }
 
@@ -146,16 +146,16 @@ pub trait Flashrom {
     fn wp_toggle(&self, en: bool) -> Result<bool, FlashromError>;
 
     /// Read the whole flash to the file specified by `path`.
-    fn read_into_file(&self, path: &str) -> Result<(), FlashromError>;
+    fn read_into_file(&self, path: &Path) -> Result<(), FlashromError>;
 
     /// Read only a region of the flash.
-    fn read_region_into_file(&self, path: &str, region: &str) -> Result<(), FlashromError>;
+    fn read_region_into_file(&self, path: &Path, region: &str) -> Result<(), FlashromError>;
 
     /// Write the whole flash to the file specified by `path`.
-    fn write_from_file(&self, path: &str) -> Result<(), FlashromError>;
+    fn write_from_file(&self, path: &Path) -> Result<(), FlashromError>;
 
     /// Verify the whole flash against the file specified by `path`.
-    fn verify_from_file(&self, path: &str) -> Result<(), FlashromError>;
+    fn verify_from_file(&self, path: &Path) -> Result<(), FlashromError>;
 
     /// Erase the whole flash.
     fn erase(&self) -> Result<(), FlashromError>;

util/flashrom_tester/src/rand_util.rs

@@ -36,10 +36,11 @@
 use std::fs::File;
 use std::io::prelude::*;
 use std::io::BufWriter;
+use std::path::Path;
 
 use rand::prelude::*;
 
-pub fn gen_rand_testdata(path: &str, size: usize) -> std::io::Result<()> {
+pub fn gen_rand_testdata(path: &Path, size: usize) -> std::io::Result<()> {
     let mut buf = BufWriter::new(File::create(path)?);
 
     let mut a: Vec<u8> = vec![0; size];
@@ -58,8 +59,8 @@ mod tests {
     fn gen_rand_testdata() {
         use super::gen_rand_testdata;
 
-        let path0 = "/tmp/idk_test00";
+        let path0 = Path::new("/tmp/idk_test00");
-        let path1 = "/tmp/idk_test01";
+        let path1 = Path::new("/tmp/idk_test01");
         let sz = 1024;
 
         gen_rand_testdata(path0, sz).unwrap();

util/flashrom_tester/src/tester.rs

@@ -42,6 +42,8 @@ use serde_json::json;
 use std::fs::File;
 use std::io::Write;
 use std::mem::MaybeUninit;
+use std::path::Path;
+use std::path::PathBuf;
 use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::Mutex;
 
@@ -60,13 +62,11 @@ pub struct TestEnv<'a> {
 
     pub wp: WriteProtectState<'a, 'static>,
     /// The path to a file containing the flash contents at test start.
-    // TODO(pmarheine) migrate this to a PathBuf for clarity
+    original_flash_contents: PathBuf,
-    original_flash_contents: String,
     /// The path to a file containing flash-sized random data
-    // TODO(pmarheine) make this a PathBuf too
+    random_data: PathBuf,
-    random_data: String,
     /// The path to a file containing layout data.
-    pub layout_file: String,
+    pub layout_file: PathBuf,
 }
 
 impl<'a> TestEnv<'a> {
@@ -83,7 +83,7 @@ impl<'a> TestEnv<'a> {
             wp: WriteProtectState::from_hardware(cmd, chip_type)?,
             original_flash_contents: "/tmp/flashrom_tester_golden.bin".into(),
             random_data: "/tmp/random_content.bin".into(),
-            layout_file: create_layout_file(rom_sz, "/tmp/", print_layout),
+            layout_file: create_layout_file(rom_sz, Path::new("/tmp/"), print_layout),
         };
 
         info!("Stashing golden image for verification/recovery on completion");
@@ -122,7 +122,7 @@ impl<'a> TestEnv<'a> {
 
     /// Return the path to a file that contains random data and is the same size
     /// as the flash chip.
-    pub fn random_data_file(&self) -> &str {
+    pub fn random_data_file(&self) -> &Path {
         &self.random_data
     }
 
@@ -156,7 +156,7 @@ impl<'a> TestEnv<'a> {
     /// path.
     ///
     /// Returns Err if they are not the same.
-    pub fn verify(&self, contents_path: &str) -> Result<(), FlashromError> {
+    pub fn verify(&self, contents_path: &Path) -> Result<(), FlashromError> {
         self.cmd.verify_from_file(contents_path)?;
         Ok(())
     }
@@ -496,12 +496,11 @@ fn decode_test_result(res: TestResult, con: TestConclusion) -> (TestConclusion,
     }
 }
 
-fn create_layout_file(rom_sz: i64, tmp_dir: &str, print_layout: bool) -> String {
+fn create_layout_file(rom_sz: i64, tmp_dir: &Path, print_layout: bool) -> PathBuf {
     info!("Calculate ROM partition sizes & Create the layout file.");
     let layout_sizes = utils::get_layout_sizes(rom_sz).expect("Could not partition rom");
 
-    let mut layout_file = tmp_dir.to_string();
+    let layout_file = tmp_dir.join("layout.file");
-    layout_file.push_str("/layout.file");
     let mut f = File::create(&layout_file).expect("Could not create layout file");
     let mut buf: Vec<u8> = vec![];
     utils::construct_layout_file(&mut buf, &layout_sizes).expect("Could not construct layout file");

util/flashrom_tester/src/tests.rs

@@ -224,7 +224,7 @@ fn elog_sanity_test(env: &mut TestEnv) -> TestResult {
 
     const ELOG_RW_REGION_NAME: &str = "RW_ELOG";
     env.cmd
-        .read_region_into_file(ELOG_FILE, ELOG_RW_REGION_NAME)?;
+        .read_region_into_file(ELOG_FILE.as_ref(), ELOG_RW_REGION_NAME)?;
 
     // Just checking for the magic numer
     // TODO: improve this test to read the events