Edward O'Callaghan submitted this change.
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(-)
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(())
}
To view, visit change 71973. To unsubscribe, or for help writing mail filters, visit settings.