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>

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 @@ use std::{
 #[derive(Default)]
 pub struct FlashromOpt<'a> {
     pub wp_opt: WPOpt,
-    pub io_opt: IOOpt<'a>,
+    pub io_opt: Option<IOOpt<'a>>,
-
-    pub layout: Option<&'a Path>, // -l <file>
-    pub image: Option<&'a str>,   // -i <name>
 
     pub flash_name: bool, // --flash-name
     pub verbose: bool,    // -V
@@ -62,13 +59,28 @@ pub struct WPOpt {
     pub disable: bool,             // --wp-disable
 }
 
-#[derive(Default)]
+pub enum OperationArgs<'a> {
-pub struct IOOpt<'a> {
+    /// The file is the whole chip.
-    pub read: Option<&'a Path>,              // -r <file>
+    EntireChip(&'a Path),
-    pub write: Option<&'a Path>,             // -w <file>
+    /// File is the size of the full chip, limited to a single named region.
-    pub verify: Option<&'a Path>,            // -v <file>
+    ///
-    pub erase: bool,                         // -E
+    /// The required path is the file to use, and the optional path is a layout file
-    pub region: Option<(&'a str, &'a Path)>, // --image
+    /// 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 @@ impl crate::Flashrom for FlashromCmd {
 
     fn name(&self) -> Result<(String, String), FlashromError> {
         let opts = FlashromOpt {
-            io_opt: IOOpt {
-                ..Default::default()
-            },
-
             flash_name: true,
-
             ..Default::default()
         };
 
@@ -132,16 +139,18 @@ impl crate::Flashrom for FlashromCmd {
         }
     }
 
-    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 {
+            io_opt: Some(IOOpt::Write(OperationArgs::FullFileRegion(
-                write: rws.write_file,
+                region,
-                ..Default::default()
+                path,
-            },
+                Some(layout),
-
+            ))),
-            layout: rws.layout_file,
-            image: rws.name_file,
-
             ..Default::default()
         };
 
@@ -220,10 +229,7 @@ impl crate::Flashrom for FlashromCmd {
 
     fn read_into_file(&self, path: &Path) -> Result<(), FlashromError> {
         let opts = FlashromOpt {
-            io_opt: IOOpt {
+            io_opt: Some(IOOpt::Read(OperationArgs::EntireChip(path))),
-                read: Some(path),
-                ..Default::default()
-            },
             ..Default::default()
         };
 
@@ -233,10 +239,9 @@ impl crate::Flashrom for FlashromCmd {
 
     fn read_region_into_file(&self, path: &Path, region: &str) -> Result<(), FlashromError> {
         let opts = FlashromOpt {
-            io_opt: IOOpt {
+            io_opt: Some(IOOpt::Read(OperationArgs::RegionFileRegion(
-                region: Some((region, path)),
+                region, path, None,
-                ..Default::default()
+            ))),
-            },
             ..Default::default()
         };
 
@@ -246,10 +251,7 @@ impl crate::Flashrom for FlashromCmd {
 
     fn write_from_file(&self, path: &Path) -> Result<(), FlashromError> {
         let opts = FlashromOpt {
-            io_opt: IOOpt {
+            io_opt: Some(IOOpt::Write(OperationArgs::EntireChip(path))),
-                write: Some(path),
-                ..Default::default()
-            },
             ..Default::default()
         };
 
@@ -259,10 +261,7 @@ impl crate::Flashrom for FlashromCmd {
 
     fn verify_from_file(&self, path: &Path) -> Result<(), FlashromError> {
         let opts = FlashromOpt {
-            io_opt: IOOpt {
+            io_opt: Some(IOOpt::Verify(OperationArgs::EntireChip(path))),
-                verify: Some(path),
-                ..Default::default()
-            },
             ..Default::default()
         };
 
@@ -272,10 +271,7 @@ impl crate::Flashrom for FlashromCmd {
 
     fn erase(&self) -> Result<(), FlashromError> {
         let opts = FlashromOpt {
-            io_opt: IOOpt {
+            io_opt: Some(IOOpt::Erase),
-                erase: true,
-                ..Default::default()
-            },
             ..Default::default()
         };
 
@@ -312,37 +308,49 @@ fn flashrom_decode_opts(opts: FlashromOpt) -> Vec<OsString> {
     }
 
     // io_opt
-    if let Some((region, path)) = opts.io_opt.region {
+    fn add_operation_args(opts: OperationArgs, params: &mut Vec<OsString>) {
-        params.push("--image".into());
+        let (file, region, layout) = match opts {
-        let mut p = OsString::new();
+            OperationArgs::EntireChip(file) => (Some(file), None, None),
-        p.push(region);
+            OperationArgs::FullFileRegion(region, file, layout) => {
-        p.push(":");
+                (Some(file), Some(region.to_string()), layout)
-        p.push(path);
+            }
-        params.push(p);
+            OperationArgs::RegionFileRegion(region, file, layout) => (
-        params.push("-r".into());
+                None,
-    } else if opts.io_opt.read.is_some() {
+                Some(format!("{region}:{}", file.to_string_lossy())),
-        params.push("-r".into());
+                layout,
-        params.push(opts.io_opt.read.unwrap().into());
+            ),
-    } else if opts.io_opt.write.is_some() {
+        };
-        params.push("-w".into());
+        if let Some(file) = file {
-        params.push(opts.io_opt.write.unwrap().into());
+            params.push(file.into())
-    } else if opts.io_opt.verify.is_some() {
+        }
-        params.push("-v".into());
+        if let Some(region) = region {
-        params.push(opts.io_opt.verify.unwrap().into());
+            params.push("--include".into());
-    } else if opts.io_opt.erase {
+            params.push(region.into())
-        params.push("-E".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 @@ mod tests {
         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 @@ mod tests {
         }
 
         test_io_opt(
-            IOOpt {
+            IOOpt::Read(crate::cmd::OperationArgs::EntireChip(Path::new("foo.bin"))),
-                read: Some(Path::new("foo.bin")),
-                ..Default::default()
-            },
             &["-r", "foo.bin"],
         );
         test_io_opt(
-            IOOpt {
+            IOOpt::Write(crate::cmd::OperationArgs::EntireChip(Path::new("bar.bin"))),
-                write: Some(Path::new("bar.bin")),
-                ..Default::default()
-            },
             &["-w", "bar.bin"],
         );
         test_io_opt(
-            IOOpt {
+            IOOpt::Verify(crate::cmd::OperationArgs::EntireChip(Path::new("baz.bin"))),
-                verify: Some(Path::new("/tmp/baz.bin")),
+            &["-v", "baz.bin"],
-                ..Default::default()
-            },
-            &["-v", "/tmp/baz.bin"],
         );
+        test_io_opt(IOOpt::Erase, &["-E"]);
         test_io_opt(
-            IOOpt {
+            IOOpt::Read(crate::cmd::OperationArgs::FullFileRegion(
-                erase: true,
+                "RO",
-                ..Default::default()
+                Path::new("foo.bin"),
-            },
+                Some(Path::new("baz.bin")),
-            &["-E"],
+            )),
+            &["-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 {

util/flashrom_tester/flashrom/src/flashromlib.rs

@@ -36,7 +36,7 @@ use libflashrom::{Chip, Programmer};
 
 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 @@ impl crate::Flashrom for FlashromLib {
         Ok(())
     }
 
-    fn write_file_with_layout(&self, rws: &ROMWriteSpecifics) -> Result<bool, FlashromError> {
+    fn write_from_file_region(
-        let buf = fs::read(rws.layout_file.unwrap()).map_err(|error| error.to_string())?;
+        &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())?;
+        layout.include_region(region)?;
-        let mut buf = fs::read(rws.write_file.unwrap()).map_err(|error| error.to_string())?;
+        let mut buf = fs::read(path).map_err(|error| error.to_string())?;
         self.flashrom
             .borrow_mut()
             .image_write(&mut buf, Some(layout))?;

util/flashrom_tester/flashrom/src/lib.rs

@@ -107,12 +107,6 @@ where
     }
 }
 
-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 @@ pub trait Flashrom {
     /// 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 @@ pub trait Flashrom {
     /// 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>;
 

util/flashrom_tester/src/tests.rs

@@ -283,12 +283,11 @@ fn partial_lock_test(section: LayoutNames) -> impl Fn(&mut TestEnv) -> TestResul
         env.wp.set_hw(true)?;
 
         // Check that we cannot write to the protected region.
-        let rws = flashrom::ROMWriteSpecifics {
+        if env
-            layout_file: Some(&env.layout_file),
+            .cmd
-            write_file: Some(env.random_data_file()),
+            .write_from_file_region(env.random_data_file(), wp_section_name, &env.layout_file)
-            name_file: Some(wp_section_name),
+            .is_ok()
-        };
+        {
-        if env.cmd.write_file_with_layout(&rws).is_ok() {
             return Err(
                 "Section should be locked, should not have been overwritable with random data"
                     .into(),
@@ -301,12 +300,11 @@ fn partial_lock_test(section: LayoutNames) -> impl Fn(&mut TestEnv) -> TestResul
         // 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 {
+        env.cmd.write_from_file_region(
-            layout_file: Some(&env.layout_file),
+            env.random_data_file(),
-            write_file: Some(env.random_data_file()),
+            non_wp_section_name,
-            name_file: Some(non_wp_section_name),
+            &env.layout_file,
-        };
+        )?;
-        env.cmd.write_file_with_layout(&rws)?;
 
         Ok(())
     }