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