[M] Change in flashrom[master]: flashrom_tester: Use Path type and tempdir for test data
Evan Benn has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68643 )
Change subject: flashrom_tester: Use Path type and tempdir for test data ......................................................................
flashrom_tester: Use Path type and tempdir for test data
Use tempdir crate to create a tempdir that is deleted when the tester closes. Update many str to Path.
BUG=b:243460685 BRANCH=None TEST=/usr/bin/flashrom_tester --flashrom_binary /usr/sbin/flashrom host
Change-Id: I4a0b25c8c70fa6dfd682fba3db7d3bf06b2eb7ac Signed-off-by: Evan Benn evanbenn@chromium.org --- M util/flashrom_tester/Cargo.toml 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 7 files changed, 140 insertions(+), 94 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/68643/1
diff --git a/util/flashrom_tester/Cargo.toml b/util/flashrom_tester/Cargo.toml index b57f04e..364a223 100644 --- a/util/flashrom_tester/Cargo.toml +++ b/util/flashrom_tester/Cargo.toml @@ -23,6 +23,7 @@ rand = "0.6.4" serde_json = "1" sys-info = "0.9" +tempdir = "0.3.7"
[build-dependencies] built = { version = "0.5", features = ["chrono"] } 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 f2c3846..8324ade 100644 --- a/util/flashrom_tester/src/tester.rs +++ b/util/flashrom_tester/src/tester.rs @@ -39,9 +39,14 @@ use flashrom::FlashromError; use flashrom::{FlashChip, Flashrom}; 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; +use tempdir::TempDir;
// type-signature comes from the return type of lib.rs workers. type TestError = Box<dyn std::error::Error>; @@ -58,23 +63,32 @@
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: PathBuf, + /// A directory for temporary test data, including the above files. + pub tmp_dir: TempDir, }
impl<'a> TestEnv<'a> { - pub fn create(chip_type: FlashChip, cmd: &'a dyn Flashrom) -> Result<Self, FlashromError> { + pub fn create( + chip_type: FlashChip, + cmd: &'a dyn Flashrom, + print_layout: bool, + ) -> Result<Self, FlashromError> { let rom_sz = cmd.get_size()?; + let tmp_dir = TempDir::new("flashrom_tester").map_err(|e| e.to_string())?; let out = TestEnv { chip_type, cmd, layout: utils::get_layout_sizes(rom_sz)?, wp: WriteProtectState::from_hardware(cmd, chip_type)?, - original_flash_contents: "/tmp/flashrom_tester_golden.bin".into(), - random_data: "/tmp/random_content.bin".into(), + original_flash_contents: tmp_dir.path().join("flashrom_tester_golden.bin"), + random_data: tmp_dir.path().join("random_content.bin"), + layout_file: create_layout_file(rom_sz, tmp_dir.path(), print_layout), + tmp_dir, };
info!("Stashing golden image for verification/recovery on completion"); @@ -113,7 +127,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 }
@@ -147,7 +161,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(()) } @@ -487,17 +501,38 @@ } }
+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 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"); + + f.write_all(&buf).expect("Writing layout file failed"); + if print_layout { + info!( + "Dumping layout file as requested:\n{}", + String::from_utf8_lossy(&buf) + ); + } + layout_file +} + pub fn run_all_tests<T, TS>( chip: FlashChip, cmd: &dyn Flashrom, ts: TS, terminate_flag: Option<&AtomicBool>, + print_layout: bool, ) -> Vec<(String, (TestConclusion, Option<TestError>))> where T: TestCase + Copy, TS: IntoIterator<Item = T>, { - let mut env = TestEnv::create(chip, cmd).expect("Failed to set up test environment"); + let mut env = + TestEnv::create(chip, cmd, print_layout).expect("Failed to set up test environment");
let mut results = Vec::new(); for t in ts { diff --git a/util/flashrom_tester/src/tests.rs b/util/flashrom_tester/src/tests.rs index 1527a6e..94fe14d 100644 --- a/util/flashrom_tester/src/tests.rs +++ b/util/flashrom_tester/src/tests.rs @@ -40,12 +40,9 @@ use std::collections::{HashMap, HashSet}; use std::convert::TryInto; use std::fs::{self, File}; -use std::io::{BufRead, Write}; +use std::io::BufRead; use std::sync::atomic::AtomicBool;
-const LAYOUT_FILE: &str = "/tmp/layout.file"; -const ELOG_FILE: &str = "/tmp/elog.file"; - /// Iterate over tests, yielding only those tests with names matching filter_names. /// /// If filter_names is None, all tests will be run. None is distinct from Some(∅); @@ -91,23 +88,6 @@ ) -> Result<(), Box<dyn std::error::Error>> { utils::ac_power_warning();
- info!("Calculate ROM partition sizes & Create the layout file."); - let rom_sz: i64 = cmd.get_size()?; - let layout_sizes = utils::get_layout_sizes(rom_sz)?; - { - let mut f = File::create(LAYOUT_FILE)?; - let mut buf: Vec<u8> = vec![]; - utils::construct_layout_file(&mut buf, &layout_sizes)?; - - f.write_all(&buf)?; - if print_layout { - info!( - "Dumping layout file as requested:\n{}", - String::from_utf8_lossy(&buf) - ); - } - } - info!("Record crossystem information.\n{}", crossystem);
// Register tests to run: @@ -144,7 +124,7 @@
// ------------------------. // Run all the tests and collate the findings: - let results = tester::run_all_tests(fc, cmd, tests, terminate_flag); + let results = tester::run_all_tests(fc, cmd, tests, terminate_flag, print_layout);
// Any leftover filtered names were specified to be run but don't exist for leftover in filter_names.iter().flatten() { @@ -241,15 +221,17 @@ env.ensure_golden()?;
const ELOG_RW_REGION_NAME: &str = "RW_ELOG"; + let elog_file = env.tmp_dir.path().join("elog.file"); + env.cmd - .read_region_into_file(ELOG_FILE, ELOG_RW_REGION_NAME)?; + .read_region_into_file(&elog_file, ELOG_RW_REGION_NAME)?;
// Just checking for the magic numer // TODO: improve this test to read the events - if fs::metadata(ELOG_FILE)?.len() < 4 { + if fs::metadata(&elog_file)?.len() < 4 { return Err("ELOG contained no data".into()); } - let data = fs::read(ELOG_FILE)?; + let data = fs::read(&elog_file)?; if u32::from_le_bytes(data[0..4].try_into()?) != 0x474f4c45 { return Err("ELOG had bad magic number".into()); } @@ -295,7 +277,7 @@
// Check that we cannot write to the protected region. let rws = flashrom::ROMWriteSpecifics { - layout_file: Some(LAYOUT_FILE), + layout_file: Some(&env.layout_file), write_file: Some(env.random_data_file()), name_file: Some(wp_section_name), }; @@ -313,7 +295,7 @@ let (non_wp_section_name, _, _) = utils::layout_section(env.layout(), section.get_non_overlapping_section()); let rws = flashrom::ROMWriteSpecifics { - layout_file: Some(LAYOUT_FILE), + layout_file: Some(&env.layout_file), write_file: Some(env.random_data_file()), name_file: Some(non_wp_section_name), };
-
Evan Benn (Code Review)