[L] Change in flashrom[master]: flashrom_tester: Rewrite IOOpts to support more operations
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/71973 )
Change subject: flashrom_tester: Rewrite IOOpts to support more operations ......................................................................
flashrom_tester: Rewrite IOOpts to support more operations
flashrom cli supports include regions for all of read write and verify, as well as omitting the read/write/verify file if an include region with file is specified. Use an enum to allow only one operation at a time. Unify the read and write region implementations.
BUG=b:235916336 BRANCH=None TEST=None
Change-Id: I1cb46bb1b26949fd9c19949c43708a8b652e00da Signed-off-by: Evan Benn evanbenn@chromium.org Reviewed-on: https://review.coreboot.org/c/flashrom/+/71973 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Peter Marheine pmarheine@chromium.org 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/tests.rs 4 files changed, 156 insertions(+), 134 deletions(-)
Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Peter Marheine: Looks good to me, but someone else must approve
diff --git a/util/flashrom_tester/flashrom/src/cmd.rs b/util/flashrom_tester/flashrom/src/cmd.rs index f24a2c9..85cbb12 100644 --- a/util/flashrom_tester/flashrom/src/cmd.rs +++ b/util/flashrom_tester/flashrom/src/cmd.rs @@ -33,7 +33,7 @@ // Software Foundation. //
-use crate::{FlashChip, FlashromError, ROMWriteSpecifics}; +use crate::{FlashChip, FlashromError};
use std::{ ffi::{OsStr, OsString}, @@ -44,10 +44,7 @@ #[derive(Default)] pub struct FlashromOpt<'a> { pub wp_opt: WPOpt, - pub io_opt: IOOpt<'a>, - - pub layout: Option<&'a Path>, // -l <file> - pub image: Option<&'a str>, // -i <name> + pub io_opt: Option<IOOpt<'a>>,
pub flash_name: bool, // --flash-name pub verbose: bool, // -V @@ -62,13 +59,28 @@ pub disable: bool, // --wp-disable }
-#[derive(Default)] -pub struct IOOpt<'a> { - 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 +pub enum OperationArgs<'a> { + /// The file is the whole chip. + EntireChip(&'a Path), + /// File is the size of the full chip, limited to a single named region. + /// + /// The required path is the file to use, and the optional path is a layout file + /// specifying how to locate regions (if unspecified, flashrom will attempt + /// to discover the layout itself). + FullFileRegion(&'a str, &'a Path, Option<&'a Path>), + /// File is the size of the single named region only. + /// + /// The required path is the file to use, and the optional path is a layout file + /// specifying how to locate regions (if unspecified, flashrom will attempt + /// to discover the layout itself). + RegionFileRegion(&'a str, &'a Path, Option<&'a Path>), // The file contains only the region +} + +pub enum IOOpt<'a> { + Read(OperationArgs<'a>), // -r <file> + Write(OperationArgs<'a>), // -w <file> + Verify(OperationArgs<'a>), // -v <file> + Erase, // -E }
#[derive(PartialEq, Eq, Debug)] @@ -116,12 +128,7 @@
fn name(&self) -> Result<(String, String), FlashromError> { let opts = FlashromOpt { - io_opt: IOOpt { - ..Default::default() - }, - flash_name: true, - ..Default::default() };
@@ -132,16 +139,18 @@ } }
- fn write_file_with_layout(&self, rws: &ROMWriteSpecifics) -> Result<bool, FlashromError> { + fn write_from_file_region( + &self, + path: &Path, + region: &str, + layout: &Path, + ) -> Result<bool, FlashromError> { let opts = FlashromOpt { - io_opt: IOOpt { - write: rws.write_file, - ..Default::default() - }, - - layout: rws.layout_file, - image: rws.name_file, - + io_opt: Some(IOOpt::Write(OperationArgs::FullFileRegion( + region, + path, + Some(layout), + ))), ..Default::default() };
@@ -220,10 +229,7 @@
fn read_into_file(&self, path: &Path) -> Result<(), FlashromError> { let opts = FlashromOpt { - io_opt: IOOpt { - read: Some(path), - ..Default::default() - }, + io_opt: Some(IOOpt::Read(OperationArgs::EntireChip(path))), ..Default::default() };
@@ -233,10 +239,9 @@
fn read_region_into_file(&self, path: &Path, region: &str) -> Result<(), FlashromError> { let opts = FlashromOpt { - io_opt: IOOpt { - region: Some((region, path)), - ..Default::default() - }, + io_opt: Some(IOOpt::Read(OperationArgs::RegionFileRegion( + region, path, None, + ))), ..Default::default() };
@@ -246,10 +251,7 @@
fn write_from_file(&self, path: &Path) -> Result<(), FlashromError> { let opts = FlashromOpt { - io_opt: IOOpt { - write: Some(path), - ..Default::default() - }, + io_opt: Some(IOOpt::Write(OperationArgs::EntireChip(path))), ..Default::default() };
@@ -259,10 +261,7 @@
fn verify_from_file(&self, path: &Path) -> Result<(), FlashromError> { let opts = FlashromOpt { - io_opt: IOOpt { - verify: Some(path), - ..Default::default() - }, + io_opt: Some(IOOpt::Verify(OperationArgs::EntireChip(path))), ..Default::default() };
@@ -272,10 +271,7 @@
fn erase(&self) -> Result<(), FlashromError> { let opts = FlashromOpt { - io_opt: IOOpt { - erase: true, - ..Default::default() - }, + io_opt: Some(IOOpt::Erase), ..Default::default() };
@@ -312,37 +308,49 @@ }
// io_opt - if let Some((region, path)) = opts.io_opt.region { - 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".into()); - params.push(opts.io_opt.read.unwrap().into()); - } else if opts.io_opt.write.is_some() { - params.push("-w".into()); - params.push(opts.io_opt.write.unwrap().into()); - } else if opts.io_opt.verify.is_some() { - params.push("-v".into()); - params.push(opts.io_opt.verify.unwrap().into()); - } else if opts.io_opt.erase { - params.push("-E".into()); + fn add_operation_args(opts: OperationArgs, params: &mut Vec<OsString>) { + let (file, region, layout) = match opts { + OperationArgs::EntireChip(file) => (Some(file), None, None), + OperationArgs::FullFileRegion(region, file, layout) => { + (Some(file), Some(region.to_string()), layout) + } + OperationArgs::RegionFileRegion(region, file, layout) => ( + None, + Some(format!("{region}:{}", file.to_string_lossy())), + layout, + ), + }; + if let Some(file) = file { + params.push(file.into()) + } + if let Some(region) = region { + params.push("--include".into()); + params.push(region.into()) + } + if let Some(layout) = layout { + params.push("--layout".into()); + params.push(layout.into()) + } + } + if let Some(io) = opts.io_opt { + match io { + IOOpt::Read(args) => { + params.push("-r".into()); + add_operation_args(args, &mut params); + } + IOOpt::Write(args) => { + params.push("-w".into()); + add_operation_args(args, &mut params); + } + IOOpt::Verify(args) => { + params.push("-v".into()); + add_operation_args(args, &mut params); + } + IOOpt::Erase => params.push("-E".into()), + } }
// misc_opt - if opts.layout.is_some() { - params.push("-l".into()); - params.push(opts.layout.unwrap().into()); - } - if opts.image.is_some() { - params.push("-i".into()); - params.push(opts.image.unwrap().into()); - } - if opts.flash_name { params.push("--flash-name".into()); } @@ -474,7 +482,7 @@ fn test_io_opt(opts: IOOpt, expected: &[&str]) { assert_eq!( flashrom_decode_opts(FlashromOpt { - io_opt: opts, + io_opt: Some(opts), ..Default::default() }), expected @@ -482,53 +490,40 @@ }
test_io_opt( - IOOpt { - read: Some(Path::new("foo.bin")), - ..Default::default() - }, + IOOpt::Read(crate::cmd::OperationArgs::EntireChip(Path::new("foo.bin"))), &["-r", "foo.bin"], ); test_io_opt( - IOOpt { - write: Some(Path::new("bar.bin")), - ..Default::default() - }, + IOOpt::Write(crate::cmd::OperationArgs::EntireChip(Path::new("bar.bin"))), &["-w", "bar.bin"], ); test_io_opt( - IOOpt { - verify: Some(Path::new("/tmp/baz.bin")), - ..Default::default() - }, - &["-v", "/tmp/baz.bin"], + IOOpt::Verify(crate::cmd::OperationArgs::EntireChip(Path::new("baz.bin"))), + &["-v", "baz.bin"], ); + test_io_opt(IOOpt::Erase, &["-E"]); test_io_opt( - IOOpt { - erase: true, - ..Default::default() - }, - &["-E"], + IOOpt::Read(crate::cmd::OperationArgs::FullFileRegion( + "RO", + Path::new("foo.bin"), + Some(Path::new("baz.bin")), + )), + &["-r", "foo.bin", "--include", "RO", "--layout", "baz.bin"], ); + + test_io_opt( + IOOpt::Read(crate::cmd::OperationArgs::RegionFileRegion( + "foo", + Path::new("bar.bin"), + None, + )), + &["-r", "--include", "foo:bar.bin"], + ) }
#[test] fn decode_misc() { //use Default::default; - assert_eq!( - flashrom_decode_opts(FlashromOpt { - layout: Some(Path::new("TestLayout")), - ..Default::default() - }), - &["-l", "TestLayout"] - ); - - assert_eq!( - flashrom_decode_opts(FlashromOpt { - image: Some("TestImage"), - ..Default::default() - }), - &["-i", "TestImage"] - );
assert_eq!( flashrom_decode_opts(FlashromOpt { diff --git a/util/flashrom_tester/flashrom/src/flashromlib.rs b/util/flashrom_tester/flashrom/src/flashromlib.rs index 6cdd55d..bf09e6d 100644 --- a/util/flashrom_tester/flashrom/src/flashromlib.rs +++ b/util/flashrom_tester/flashrom/src/flashromlib.rs @@ -36,7 +36,7 @@
use std::{cell::RefCell, convert::TryFrom, fs, path::Path};
-use crate::{FlashChip, FlashromError, ROMWriteSpecifics}; +use crate::{FlashChip, FlashromError};
#[derive(Debug)] pub struct FlashromLib { @@ -127,14 +127,19 @@ Ok(()) }
- fn write_file_with_layout(&self, rws: &ROMWriteSpecifics) -> Result<bool, FlashromError> { - let buf = fs::read(rws.layout_file.unwrap()).map_err(|error| error.to_string())?; + fn write_from_file_region( + &self, + path: &Path, + region: &str, + layout: &Path, + ) -> Result<bool, FlashromError> { + let buf = fs::read(layout).map_err(|error| error.to_string())?; let buf = String::from_utf8(buf).unwrap(); let mut layout: libflashrom::Layout = buf .parse() .map_err(|e: Box<dyn std::error::Error>| e.to_string())?; - layout.include_region(rws.name_file.unwrap())?; - let mut buf = fs::read(rws.write_file.unwrap()).map_err(|error| error.to_string())?; + layout.include_region(region)?; + let mut buf = fs::read(path).map_err(|error| error.to_string())?; self.flashrom .borrow_mut() .image_write(&mut buf, Some(layout))?; diff --git a/util/flashrom_tester/flashrom/src/lib.rs b/util/flashrom_tester/flashrom/src/lib.rs index 6b0cca3..7c86649 100644 --- a/util/flashrom_tester/flashrom/src/lib.rs +++ b/util/flashrom_tester/flashrom/src/lib.rs @@ -107,12 +107,6 @@ } }
-pub struct ROMWriteSpecifics<'a> { - pub layout_file: Option<&'a Path>, - pub write_file: Option<&'a Path>, - pub name_file: Option<&'a str>, -} - pub trait Flashrom { /// Returns the size of the flash in bytes. fn get_size(&self) -> Result<i64, FlashromError>; @@ -120,9 +114,6 @@ /// Returns the vendor name and the flash name. fn name(&self) -> Result<(String, String), FlashromError>;
- /// Write only a region of the flash. - fn write_file_with_layout(&self, rws: &ROMWriteSpecifics) -> Result<bool, FlashromError>; - /// Set write protect status and range. fn wp_range(&self, range: (i64, i64), wp_enable: bool) -> Result<bool, FlashromError>;
@@ -148,6 +139,16 @@ /// Write the whole flash to the file specified by `path`. fn write_from_file(&self, path: &Path) -> Result<(), FlashromError>;
+ /// Write only a region of the flash. + /// `path` is a file of the size of the whole flash. + /// The `region` name corresponds to a region name in the `layout` file, not the flash. + fn write_from_file_region( + &self, + path: &Path, + region: &str, + layout: &Path, + ) -> Result<bool, FlashromError>; + /// Verify the whole flash against the file specified by `path`. fn verify_from_file(&self, path: &Path) -> Result<(), FlashromError>;
diff --git a/util/flashrom_tester/src/tests.rs b/util/flashrom_tester/src/tests.rs index 13ba050..847bfec 100644 --- a/util/flashrom_tester/src/tests.rs +++ b/util/flashrom_tester/src/tests.rs @@ -283,12 +283,11 @@ env.wp.set_hw(true)?;
// Check that we cannot write to the protected region. - let rws = flashrom::ROMWriteSpecifics { - layout_file: Some(&env.layout_file), - write_file: Some(env.random_data_file()), - name_file: Some(wp_section_name), - }; - if env.cmd.write_file_with_layout(&rws).is_ok() { + if env + .cmd + .write_from_file_region(env.random_data_file(), wp_section_name, &env.layout_file) + .is_ok() + { return Err( "Section should be locked, should not have been overwritable with random data" .into(), @@ -301,12 +300,11 @@ // Check that we can write to the non protected region. let (non_wp_section_name, _, _) = utils::layout_section(env.layout(), section.get_non_overlapping_section()); - let rws = flashrom::ROMWriteSpecifics { - layout_file: Some(&env.layout_file), - write_file: Some(env.random_data_file()), - name_file: Some(non_wp_section_name), - }; - env.cmd.write_file_with_layout(&rws)?; + env.cmd.write_from_file_region( + env.random_data_file(), + non_wp_section_name, + &env.layout_file, + )?;
Ok(()) }
-
Edward O'Callaghan (Code Review)