[M] Change in flashrom[master]: flashrom_tester: Use path types for things that are paths

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69064 )

Change subject: flashrom_tester: Use path types for things that are paths
......................................................................

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 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/69064
Tested-by: build bot (Jenkins) 
Reviewed-by: Angel Pons 
Reviewed-by: Edward O'Callaghan 
---
M util/flashrom_tester/flashrom/src/cmd.rs
M util/flashrom_tester/flashrom/src/flashromlib.rs
M util/flashrom_tester/flashrom/src/lib.rs
M util/flashrom_tester/src/rand_util.rs
M util/flashrom_tester/src/tester.rs
M util/flashrom_tester/src/tests.rs
6 files changed, 100 insertions(+), 69 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Angel Pons: Looks good to me, approved
  Edward O'Callaghan: Looks good to me, approved

diff --git a/util/flashrom_tester/flashrom/src/cmd.rs b/util/flashrom_tester/flashrom/src/cmd.rs
index faf597d..4078910 100644
--- a/util/flashrom_tester/flashrom/src/cmd.rs
+++ b/util/flashrom_tester/flashrom/src/cmd.rs
@@ -35,15 +35,19 @@

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 image: Option<&'a str>,  // -i <name>
+    pub layout: Option<&'a Path>, // -l <file>
+    pub image: Option<&'a str>,   // -i <name>

pub flash_name: bool, // --flash-name
     pub verbose: bool,    // -V
@@ -60,11 +64,11 @@

#[derive(Default)]
 pub struct IOOpt<'a> {
-    pub read: Option<&'a str>,              // -r <file>
-    pub write: Option<&'a str>,             // -w <file>
-    pub verify: Option<&'a str>,            // -v <file>
-    pub erase: bool,                        // -E
-    pub region: Option<(&'a str, &'a str)>, // --image
+    pub read: Option<&'a Path>,              // -r <file>
+    pub write: Option<&'a Path>,             // -w <file>
+    pub verify: Option<&'a Path>,            // -v <file>
+    pub erase: bool,                         // -E
+    pub region: Option<(&'a str, &'a Path)>, // --image
 }

#[derive(PartialEq, Eq, Debug)]
@@ -227,7 +231,7 @@
         }
     }

-    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 @@
         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 @@
         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 @@
         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 @@
     }
 }

-fn flashrom_decode_opts(opts: FlashromOpt) -> Vec<String> {
-    let mut params = Vec::<String>::new();
+fn flashrom_decode_opts(opts: FlashromOpt) -> Vec<OsString> {
+    let mut params = Vec::<OsString>::new();

// ------------ WARNING !!! ------------
     // each param must NOT contain spaces!
@@ -307,58 +311,62 @@
     // 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(hex_range_string(x0, x1));
+        params.push("--wp-range".into());
+        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(format!("{}:{}", region, path));
-        params.push("-r".to_string());
+        params.push("--image".into());
+        let mut p = OsString::new();
+        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(opts.io_opt.read.unwrap().to_string());
+        params.push("-r".into());
+        params.push(opts.io_opt.read.unwrap().into());
     } else if opts.io_opt.write.is_some() {
-        params.push("-w".to_string());
-        params.push(opts.io_opt.write.unwrap().to_string());
+        params.push("-w".into());
+        params.push(opts.io_opt.write.unwrap().into());
     } else if opts.io_opt.verify.is_some() {
-        params.push("-v".to_string());
-        params.push(opts.io_opt.verify.unwrap().to_string());
+        params.push("-v".into());
+        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(opts.layout.unwrap().to_string());
+        params.push("-l".into());
+        params.push(opts.layout.unwrap().into());
     }
     if opts.image.is_some() {
-        params.push("-i".to_string());
-        params.push(opts.image.unwrap().to_string());
+        params.push("-i".into());
+        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 @@
 ) -> 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 @@

#[cfg(test)]
 mod tests {
+    use std::path::Path;
+
     use super::flashrom_decode_opts;
     use super::{FlashromOpt, IOOpt, WPOpt};

@@ -515,21 +525,21 @@

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 @@
         //use Default::default;
         assert_eq!(
             flashrom_decode_opts(FlashromOpt {
-                layout: Some("TestLayout"),
+                layout: Some(Path::new("TestLayout")),
                 ..Default::default()
             }),
             &["-l", "TestLayout"]
diff --git a/util/flashrom_tester/flashrom/src/flashromlib.rs b/util/flashrom_tester/flashrom/src/flashromlib.rs
index d20dbb2..3036394 100644
--- a/util/flashrom_tester/flashrom/src/flashromlib.rs
+++ b/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 @@
         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 @@
         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 @@
         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(())
diff --git a/util/flashrom_tester/flashrom/src/lib.rs b/util/flashrom_tester/flashrom/src/lib.rs
index 7e245ae..11874f9 100644
--- a/util/flashrom_tester/flashrom/src/lib.rs
+++ b/util/flashrom_tester/flashrom/src/lib.rs
@@ -39,7 +39,7 @@
 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 @@
 }

pub struct ROMWriteSpecifics<'a> {
-    pub layout_file: Option<&'a str>,
-    pub write_file: Option<&'a str>,
+    pub layout_file: Option<&'a Path>,
+    pub write_file: Option<&'a Path>,
     pub name_file: Option<&'a str>,
 }

@@ -146,16 +146,16 @@
     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>;
diff --git a/util/flashrom_tester/src/rand_util.rs b/util/flashrom_tester/src/rand_util.rs
index 1b0912b..a040c89 100644
--- a/util/flashrom_tester/src/rand_util.rs
+++ b/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 @@
     fn gen_rand_testdata() {
         use super::gen_rand_testdata;

-        let path0 = "/tmp/idk_test00";
-        let path1 = "/tmp/idk_test01";
+        let path0 = Path::new("/tmp/idk_test00");
+        let path1 = Path::new("/tmp/idk_test01");
         let sz = 1024;

gen_rand_testdata(path0, sz).unwrap();
diff --git a/util/flashrom_tester/src/tester.rs b/util/flashrom_tester/src/tester.rs
index 292fb78..6ce3889 100644
--- a/util/flashrom_tester/src/tester.rs
+++ b/util/flashrom_tester/src/tester.rs
@@ -42,6 +42,8 @@
 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 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: String,
+    original_flash_contents: PathBuf,
     /// The path to a file containing flash-sized random data
-    // TODO(pmarheine) make this a PathBuf too
-    random_data: String,
+    random_data: PathBuf,
     /// The path to a file containing layout data.
-    pub layout_file: String,
+    pub layout_file: PathBuf,
 }

impl<'a> TestEnv<'a> {
@@ -83,7 +83,7 @@
             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 @@

/// 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 @@
     /// 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 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();
-    layout_file.push_str("/layout.file");
+    let layout_file = tmp_dir.join("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");
diff --git a/util/flashrom_tester/src/tests.rs b/util/flashrom_tester/src/tests.rs
index 3e1737e..af34208 100644
--- a/util/flashrom_tester/src/tests.rs
+++ b/util/flashrom_tester/src/tests.rs
@@ -224,7 +224,7 @@

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