November 2022 - flashrom-gerrit

[XL] Change in flashrom[master]: tree/: Convert flashchip read func ptr to enumerate
by Edward O'Callaghan (Code Review) 01 Nov '22

01 Nov '22

Attention is currently required from: Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66788 ) Change subject: tree/: Convert flashchip read func ptr to enumerate ...................................................................... Patch Set 16: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/66788/comment/0028b985_2602158f PS15, Line 14: Squash in, : flashchips.c: convert edi_chip_read() : flashchips.c: convert spi_chip_read() : flashchips.c: convert read_opaque() : flashchips.c: convert read_memmapped() : flashchips.c: convert spi_read_at45db_e8() : flashchips.c: convert spi_read_at45db() : flashchips.c: convert NULL cases > Not needed anymore Done -- To view, visit https://review.coreboot.org/c/flashrom/+/66788 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I612d46fefedf2b69e7e2064aa857fa0756efb4e7 Gerrit-Change-Number: 66788 Gerrit-PatchSet: 16 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 01 Nov 2022 01:08:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: comment

1 0

[XL] Change in flashrom[master]: tree/: Convert flashchip read func ptr to enumerate
by Edward O'Callaghan (Code Review) 01 Nov '22

01 Nov '22

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Hello Felix Singer, build bot (Jenkins), Angel Pons, Nikolai Artemiev, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/66788 to look at the new patch set (#16). Change subject: tree/: Convert flashchip read func ptr to enumerate ...................................................................... tree/: Convert flashchip read func ptr to enumerate This forges the way for flashchips.c to be pure declarative data and lookup functions for dispatch to be pure. This means that the flashchips data could be extracted out to be agnostic data of the flashrom code and algorithms. Change-Id: I612d46fefedf2b69e7e2064aa857fa0756efb4e7 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashchips.c M flashrom.c M fmap.c M include/flash.h M libflashrom.c M tests/chip.c M tests/chip_wp.c 7 files changed, 669 insertions(+), 607 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/88/66788/16 -- To view, visit https://review.coreboot.org/c/flashrom/+/66788 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I612d46fefedf2b69e7e2064aa857fa0756efb4e7 Gerrit-Change-Number: 66788 Gerrit-PatchSet: 16 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[XL] Change in flashrom[master]: tree/: Convert flashchip write func ptr to enumerate
by Edward O'Callaghan (Code Review) 01 Nov '22

01 Nov '22

Attention is currently required from: Angel Pons, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66782 ) Change subject: tree/: Convert flashchip write func ptr to enumerate ...................................................................... Patch Set 15: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/66782/comment/87a92fb2_01257ab0 PS14, Line 14: Squash in, : flashchips.c: convert spi_chip_write_256() : flashchips.c: convert write_jedec_1() : flashchips.c: convert write_jedec() : flashchips.c: convert write_opaque() : flashchips.c: convert spi_chip_write_1() : flashchips.c: convert spi_aai_write() : flashchips.c: convert edi_chip_write() : flashchips.c: convert write_en29lv640b() : flashchips.c: convert write_82802ab() : flashchips.c: convert spi_write_at45db() : flashchips.c: convert write_28sf040() : flashchips.c: convert NULL write > Not needed anymore Done -- To view, visit https://review.coreboot.org/c/flashrom/+/66782 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I80149de169464b204fb09f1424a86fc645b740fd Gerrit-Change-Number: 66782 Gerrit-PatchSet: 15 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 01 Nov 2022 01:08:04 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: comment

1 0

[XL] Change in flashrom[master]: tree/: Convert flashchip write func ptr to enumerate
by Edward O'Callaghan (Code Review) 01 Nov '22

01 Nov '22

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Hello Felix Singer, build bot (Jenkins), Angel Pons, Nikolai Artemiev, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/66782 to look at the new patch set (#15). Change subject: tree/: Convert flashchip write func ptr to enumerate ...................................................................... tree/: Convert flashchip write func ptr to enumerate This forges the way for flashchips.c to be pure declarative data and lookup functions for dispatch to be pure. This means that the flashchips data could be extracted out to be agnostic data of the flashrom code and algorithms. Change-Id: I80149de169464b204fb09f1424a86fc645b740fd Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashchips.c M flashrom.c M include/flash.h M sfdp.c M tests/chip.c M tests/chip_wp.c M tests/spi25.c 7 files changed, 671 insertions(+), 599 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/82/66782/15 -- To view, visit https://review.coreboot.org/c/flashrom/+/66782 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I80149de169464b204fb09f1424a86fc645b740fd Gerrit-Change-Number: 66782 Gerrit-PatchSet: 15 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[XL] Change in flashrom[master]: tree/: Convert flashchip probe func ptr to enumerate
by Edward O'Callaghan (Code Review) 01 Nov '22

01 Nov '22

Attention is currently required from: Angel Pons, Arthur Heymans, Anastasia Klimchuk. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/66781 ) Change subject: tree/: Convert flashchip probe func ptr to enumerate ...................................................................... Patch Set 14: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/66781/comment/d96ee2c6_4f556fd8 PS13, Line 14: squash in, : : flashchip.c: probe_jedec_29gl : flashchip.c: probe_82802ab : flashchip.c: probe_opaque : flashchip.c: probe_jedec : flashchip.c: probe_spi_res2 : flashchip.c: probe_spi_res1 : flashchip.c: probe_spi_rems : flashchip.c: probe_spi_at25f : flashchip.c: probe_spi_at45db : flashchip.c: probe_spi_st95 : flashchip.c: probe_spi_sfdp : flashchip.c: probe_spi_rdid{4} : flashchip.c: probe_w29ee011 : flashchip.c: probe_en29lv640b : flashchip.c: edi_probe_kb9012 : flashchip.c: probe_spi_big_spansion > I guess this is not needed anymore ;) Done -- To view, visit https://review.coreboot.org/c/flashrom/+/66781 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I00aaab9c83f305cd47e78c36d9c2867f2b73c396 Gerrit-Change-Number: 66781 Gerrit-PatchSet: 14 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 01 Nov 2022 01:05:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: comment

1 0

[XL] Change in flashrom[master]: tree/: Convert flashchip probe func ptr to enumerate
by Edward O'Callaghan (Code Review) 01 Nov '22

01 Nov '22

Attention is currently required from: Edward O'Callaghan, Angel Pons, Arthur Heymans, Anastasia Klimchuk. Hello Felix Singer, build bot (Jenkins), Angel Pons, Nikolai Artemiev, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/66781 to look at the new patch set (#14). Change subject: tree/: Convert flashchip probe func ptr to enumerate ...................................................................... tree/: Convert flashchip probe func ptr to enumerate This forges the way for flashchips.c to be pure declarative data and lookup functions for dispatch to be pure. This means that the flashchips data could be extracted out to be agnostic data of the flashrom code and algorithms. Change-Id: I00aaab9c83f305cd47e78c36d9c2867f2b73c396 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashchips.c M flashrom.c M include/flash.h M tests/spi25.c 4 files changed, 665 insertions(+), 596 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/81/66781/14 -- To view, visit https://review.coreboot.org/c/flashrom/+/66781 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I00aaab9c83f305cd47e78c36d9c2867f2b73c396 Gerrit-Change-Number: 66781 Gerrit-PatchSet: 14 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: flashrom_tester: Change String types to PathBuf where appropriate
by Evan Benn (Code Review) 01 Nov '22

01 Nov '22

Evan Benn has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/69064 ) Change subject: flashrom_tester: Change String types to PathBuf where appropriate ...................................................................... flashrom_tester: Change String types to PathBuf where appropriate String types were being used for paths to files. Change those to PathBuf and Path. 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(a)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, 97 insertions(+), 69 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/69064/1 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 -- To view, visit https://review.coreboot.org/c/flashrom/+/69064 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I69531bec5436a60430eae975eeab02c8835962bf Gerrit-Change-Number: 69064 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: flashrom_tester: Move layout_file to TestEnv
by Evan Benn (Code Review) 01 Nov '22

01 Nov '22

Evan Benn has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/69063 ) Change subject: flashrom_tester: Move layout_file to TestEnv ...................................................................... flashrom_tester: Move layout_file to TestEnv layout_file is part of the test environment, move it from a global to a member of the TestEnv struct. This allows layout to be part of the tempdir in a subsequent patch. 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: Ia7e8efeb4fbac0a46627f079956d671aed43f1c7 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M util/flashrom_tester/src/tester.rs M util/flashrom_tester/src/tests.rs 2 files changed, 56 insertions(+), 24 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/63/69063/1 diff --git a/util/flashrom_tester/src/tester.rs b/util/flashrom_tester/src/tester.rs index f2c3846..292fb78 100644 --- a/util/flashrom_tester/src/tester.rs +++ b/util/flashrom_tester/src/tester.rs @@ -39,6 +39,8 @@ 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::sync::atomic::{AtomicBool, Ordering}; use std::sync::Mutex; @@ -63,10 +65,16 @@ /// The path to a file containing flash-sized random data // TODO(pmarheine) make this a PathBuf too random_data: String, + /// The path to a file containing layout data. + pub layout_file: String, } 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 out = TestEnv { chip_type, @@ -75,6 +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), }; info!("Stashing golden image for verification/recovery on completion"); @@ -487,17 +496,39 @@ } } +fn create_layout_file(rom_sz: i64, tmp_dir: &str, print_layout: bool) -> String { + 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 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..3e1737e 100644 --- a/util/flashrom_tester/src/tests.rs +++ b/util/flashrom_tester/src/tests.rs @@ -40,10 +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. @@ -91,23 +90,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 +126,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() { @@ -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), }; -- To view, visit https://review.coreboot.org/c/flashrom/+/69063 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia7e8efeb4fbac0a46627f079956d671aed43f1c7 Gerrit-Change-Number: 69063 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: flashrom_tester: Use tempdir crate for test data
by Evan Benn (Code Review) 01 Nov '22

01 Nov '22

Attention is currently required from: Anastasia Klimchuk. Hello build bot (Jenkins), Anastasia Klimchuk, Peter Marheine, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/68739 to look at the new patch set (#3). Change subject: flashrom_tester: Use tempdir crate for test data ...................................................................... flashrom_tester: Use tempdir crate for test data Use tempdir crate to create a tempdir that is deleted when the tester closes. 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: I14089ef69c3f509cb6f01cc44c7924c244ab7d73 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M util/flashrom_tester/Cargo.toml M util/flashrom_tester/src/tester.rs M util/flashrom_tester/src/tests.rs 3 files changed, 32 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/39/68739/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/68739 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I14089ef69c3f509cb6f01cc44c7924c244ab7d73 Gerrit-Change-Number: 68739 Gerrit-PatchSet: 3 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: flashrom.c: Make 'chip_to_probe' a param to probe_flash()
by Felix Singer (Code Review) 01 Nov '22

01 Nov '22

Attention is currently required from: Edward O'Callaghan, Anastasia Klimchuk. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67091 ) Change subject: flashrom.c: Make 'chip_to_probe' a param to probe_flash() ...................................................................... Patch Set 13: Code-Review+2 (2 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/67091/comment/9e69b4d4_43df1e6b PS13, Line 868: goto notfound; Please a line after that File w29ee011.c: https://review.coreboot.org/c/flashrom/+/67091/comment/003650e6_4db81a3d PS13, Line 25: override_chip || strcmp(override_chip, chip_name) Just an idea, a nit: How about moving this to a variable `can_override`? This also would allow us having one path regarding the returns. On the other side this way it's more clear what is returned. Not sure what's better. Feel free to mark as resolved if you think it's fine this way 😊 -- To view, visit https://review.coreboot.org/c/flashrom/+/67091 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ifcdace07ea2135d83dea92cfa5c6bec8d7ddf05d Gerrit-Change-Number: 67091 Gerrit-PatchSet: 13 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 01 Nov 2022 00:50:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit November 2022