Attention is currently required from: Dhiren Serai.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67331 )
Change subject: fix malloc garbage values issue
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/67331/comment/e16c2e6d_ccb0a8c5
PS2, Line 13: to zero as default so it will not be uninitialized/garbage.
I have trouble to understand if this is silencing a false-positive,
fixing a bug or covering one up. What is the actual issue? Having
garbage values in memory is only a problem if we read/process them.
Would the zeros be read somewhere? If so is zero a reasonable value?
is there another issue maybe, that data is read that shouldn't be?
--
To view, visit https://review.coreboot.org/c/flashrom/+/67331
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id3390a40033408d827d89c3af4cd7b799e254271
Gerrit-Change-Number: 67331
Gerrit-PatchSet: 2
Gerrit-Owner: Dhiren Serai <dhirensr(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Dhiren Serai <dhirensr(a)gmail.com>
Gerrit-Comment-Date: Mon, 24 Oct 2022 08:26:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Evan Benn.
Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68739 )
Change subject: flashrom_tester: Use tempdir crate for test data
......................................................................
Patch Set 1: Code-Review+1
--
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: 1
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-Comment-Date: Mon, 24 Oct 2022 04:57:53 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons.
Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68643 )
Change subject: flashrom_tester: Use Path type for test data
......................................................................
Patch Set 2:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/68643/comment/a0a12a5b_b8007dfa
PS1, Line 10: Update many str to Path.
> Can this be done separately from the tempdir stuff?
Yes1
--
To view, visit https://review.coreboot.org/c/flashrom/+/68643
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4a0b25c8c70fa6dfd682fba3db7d3bf06b2eb7ac
Gerrit-Change-Number: 68643
Gerrit-PatchSet: 2
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Mon, 24 Oct 2022 04:11:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Dhiren Serai.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/67331
to look at the new patch set (#2).
Change subject: fix malloc garbage values issue
......................................................................
fix malloc garbage values issue
variables which are uninitialized and using malloc
by default has garbage values which is being shown by
scan build.
For fixing that I used calloc which sets allocated memory
to zero as default so it will not be uninitialized/garbage.
Change-Id: Id3390a40033408d827d89c3af4cd7b799e254271
Signed-off-by: dhirensr <dhirensr(a)gmail.com>
---
M sfdp.c
1 file changed, 18 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/31/67331/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/67331
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Id3390a40033408d827d89c3af4cd7b799e254271
Gerrit-Change-Number: 67331
Gerrit-PatchSet: 2
Gerrit-Owner: Dhiren Serai <dhirensr(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Felix Singer <felixsinger(a)posteo.net>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Dhiren Serai <dhirensr(a)gmail.com>
Gerrit-MessageType: newpatchset
Evan Benn has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68739 )
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
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, 31 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/39/68739/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/src/tester.rs b/util/flashrom_tester/src/tester.rs
index 6ce3889..8324ade 100644
--- a/util/flashrom_tester/src/tester.rs
+++ b/util/flashrom_tester/src/tester.rs
@@ -46,6 +46,7 @@
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>;
@@ -67,6 +68,8 @@
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> {
@@ -76,14 +79,16 @@
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(),
- layout_file: create_layout_file(rom_sz, Path::new("/tmp/"), print_layout),
+ 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");
diff --git a/util/flashrom_tester/src/tests.rs b/util/flashrom_tester/src/tests.rs
index af34208..94fe14d 100644
--- a/util/flashrom_tester/src/tests.rs
+++ b/util/flashrom_tester/src/tests.rs
@@ -43,8 +43,6 @@
use std::io::BufRead;
use std::sync::atomic::AtomicBool;
-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(∅);
@@ -223,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.as_ref(), 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());
}
--
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: 1
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: newchange
Attention is currently required from: Evan Benn.
Hello build bot (Jenkins), Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/68643
to look at the new patch set (#2).
Change subject: flashrom_tester: Use Path type for test data
......................................................................
flashrom_tester: Use Path type for test data
Update may 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(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, 127 insertions(+), 88 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/68643/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/68643
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I4a0b25c8c70fa6dfd682fba3db7d3bf06b2eb7ac
Gerrit-Change-Number: 68643
Gerrit-PatchSet: 2
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: newpatchset
Attention is currently required from: Felix Singer, Edward O'Callaghan, Angel Pons, Alexander Goncharov.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68434 )
Change subject: tree/: Replace NULL-case of programmer_delay() with internal_delay
......................................................................
Patch Set 1: Code-Review+1
(2 comments)
Patchset:
PS1:
I like this. The cherry on the top of the cake would be to rename `internal_delay` so that it won't look like a property of internal programmer. But not in this patch.
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/68434/comment/55ed01fa_b743911c
PS1, Line 282: else
else branch also needs braces as per coding style (because all previous if - else if conditions are mutiline and have braces).
--
To view, visit https://review.coreboot.org/c/flashrom/+/68434
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I1da230804d5e8f47a6e281feb66f381514dc6861
Gerrit-Change-Number: 68434
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net>
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: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Comment-Date: Sun, 23 Oct 2022 23:57:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Thomas Heijligen, Edward O'Callaghan.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67475 )
Change subject: flashrom.c: Confine is_internal checks under a symbol
......................................................................
Patch Set 5: Code-Review+1
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/67475/comment/2fe85fd4_4fc8679c
PS5, Line 7: Confine is_internal checks under a symbol
This is already done in some of the earlier patches, `is_internal_programmer()` already exists in flashrom.c. May I suggest a different commit message to reflect what is happening in the latest version of this patch:
flashrom.c: Pass is_internal as an argument to *_help_message functions
Providing is_internal as an argument allows *_help_message to be pure functions without side-effects, and to do one job: printing a message.
--
To view, visit https://review.coreboot.org/c/flashrom/+/67475
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I569811798bfe310c59b8b61afba359bef68969fb
Gerrit-Change-Number: 67475
Gerrit-PatchSet: 5
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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Sun, 23 Oct 2022 23:40:22 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Nico Huber, Thomas Heijligen, Edward O'Callaghan, Angel Pons.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67474 )
Change subject: flashrom.c: Drop emergency_help_message() after erasure
......................................................................
Patch Set 5:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/67474/comment/8933a303_bb8af04a
PS2, Line 10: agnostic
> The principle idea suggested here is not at all unreasonable and had crossed my mind already however […]
It seems to me there are two questions here:
1) do we need to show emergency message when erase failed? (and the FIXME from many years ago thinks No we don't need)
2) where should help messages live, in cli_classic or in flashrom.c?
If the answer to 1) is No, then this patch can go ahead, and 2) can be discussed and decided completely independently of this patch.
If the answer to 1) is Yes then... I made an observation that `flashrom_image_write` has 3 callsites of `emergency_help_message()`. So moving the messages to cli_classic would need to take care of `flashrom_image_write`.
I would start with answering 1)
Maybe instead of emergency message we can msg_gerr with the text similar to the text of FIXME?
--
To view, visit https://review.coreboot.org/c/flashrom/+/67474
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib0d390c44e7a851e684014925a25c8b259b810cd
Gerrit-Change-Number: 67474
Gerrit-PatchSet: 5
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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Comment-Date: Sun, 23 Oct 2022 23:18:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de>
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment