December 2022 - flashrom-gerrit

[M] Change in flashrom[master]: tree/: Move programmer_delay() out of programmer state machine
by Alexander Goncharov (Code Review) 06 Dec '22

06 Dec '22

Attention is currently required from: Felix Singer, Nico Huber, Thomas Heijligen, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67393 ) Change subject: tree/: Move programmer_delay() out of programmer state machine ...................................................................... Patch Set 9: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/67393 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id059abb58b31a066a408009073912da2b224d40c Gerrit-Change-Number: 67393 Gerrit-PatchSet: 9 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.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-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 06 Dec 2022 13:28:47 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: cbtables.c/search_lb_records: Drop unused variable `count`
by Angel Pons (Code Review) 06 Dec '22

06 Dec '22

Attention is currently required from: Felix Singer, David Hendricks. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70247 ) Change subject: cbtables.c/search_lb_records: Drop unused variable `count` ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/70247/comment/5853b68d_d22d0b46 PS3, Line 9: Clang 15 complains about it. Thanks for specifying that the unused variable makes some compilers complain, as it justifies why 1.3.x should also contain this commit. -- To view, visit https://review.coreboot.org/c/flashrom/+/70247 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I340208f513bed57a9cc2bba880a2400352c5cc42 Gerrit-Change-Number: 70247 Gerrit-PatchSet: 3 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Comment-Date: Tue, 06 Dec 2022 12:10:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: cbtables.c/search_lb_records: Drop unused variable `count`
by Angel Pons (Code Review) 06 Dec '22

06 Dec '22

Attention is currently required from: Felix Singer, David Hendricks. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70247 ) Change subject: cbtables.c/search_lb_records: Drop unused variable `count` ...................................................................... Patch Set 3: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/70247 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I340208f513bed57a9cc2bba880a2400352c5cc42 Gerrit-Change-Number: 70247 Gerrit-PatchSet: 3 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com> Gerrit-Comment-Date: Tue, 06 Dec 2022 12:07:56 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom.c: Position heap alloc along side check in compare_range()
by Angel Pons (Code Review) 06 Dec '22

06 Dec '22

Attention is currently required from: Felix Singer, Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69473 ) Change subject: flashrom.c: Position heap alloc along side check in compare_range() ...................................................................... Patch Set 1: Code-Review+2 (2 comments) Patchset: PS1: +2 because it doesn't really matter from a functional point of view, c.f. the other comment. It's mostly a matter of style and/or personal preference. File flashrom.c: https://review.coreboot.org/c/flashrom/+/69473/comment/1ac6d495_a5234214 PS1, Line 451: uint8_t *cmpbuf = malloc(len); Felix, from a functional standpoint, this change only swaps the order of two variable declarations (both of which were already initialised in-line). It shouldn't break anything w.r.t. declarations-before-code that wasn't already "broken". > Why the ANSI C89 requirement, isn't C99 closer to 2023, nearly a 34yo requirement of the toolchain. "The" toolchain? flashrom can be built with a variety of toolchains. Some of them, like https://en.wikipedia.org/wiki/DJGPP (to build flashrom for DOS), are quite unusual. It's a good idea to use util/manibuilder from time to time to do a more comprehensive build test. At the very least, we should use `util/manibuilder` to build test releases, but it'd be nice to build-test flashrom periodically (daily, or after a change is submitted). -- To view, visit https://review.coreboot.org/c/flashrom/+/69473 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0386ac4c09a541cb9a659b2410ce49c3292ecc6e Gerrit-Change-Number: 69473 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(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-Comment-Date: Tue, 06 Dec 2022 12:07:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom.c: Replace 'exit(1)' leaks with return codes on err paths
by Angel Pons (Code Review) 06 Dec '22

06 Dec '22

Attention is currently required from: Edward O'Callaghan. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69474 ) Change subject: flashrom.c: Replace 'exit(1)' leaks with return codes on err paths ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/69474/comment/96bce863_a544e211 PS3, Line 11: to occur. Sorry, what exactly leaks if the program terminates? If `malloc()` or similar failed, it's likely because of a bigger problem with the system that is outside of flashrom's control; it's possible that flashrom may not be able to gracefully handle the situation. That being said, getting rid of `exit()` calls makes sense for libflashrom users, as it prevents the callers from trying to handle errors. -- To view, visit https://review.coreboot.org/c/flashrom/+/69474 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4186a40071e9a7296d601582ff15ad7df09c70a Gerrit-Change-Number: 69474 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(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: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Tue, 06 Dec 2022 11:56:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: flashrom.c: Replace 'exit(1)' leaks with return codes on err paths
by Felix Singer (Code Review) 06 Dec '22

06 Dec '22

Felix Singer has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69474 ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: flashrom.c: Replace 'exit(1)' leaks with return codes on err paths ...................................................................... flashrom.c: Replace 'exit(1)' leaks with return codes on err paths Do not just exit in the middle of the process, rather return a value back up to the caller to allow proper resource cleanup's to occur. Change-Id: Ie4186a40071e9a7296d601582ff15ad7df09c70a Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69474 Reviewed-by: Felix Singer <felixsinger(a)posteo.net> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M flashrom.c 1 file changed, 20 insertions(+), 3 deletions(-) Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, approved diff --git a/flashrom.c b/flashrom.c index fde6d5a..9c22eb6 100644 --- a/flashrom.c +++ b/flashrom.c @@ -319,7 +319,7 @@ opt = malloc(optlen + 1); if (!opt) { msg_gerr("Out of memory!\n"); - exit(1); + return NULL; } strncpy(opt, opt_pos, optlen); opt[optlen] = '\0'; @@ -455,7 +455,7 @@ if (!cmpbuf) { msg_gerr("Out of memory!\n"); - exit(1); + return -1; } memset(cmpbuf, erased_value, len); ret = verify_range(flash, cmpbuf, start, len); @@ -897,7 +897,7 @@ flash->chip = calloc(1, sizeof(*flash->chip)); if (!flash->chip) { msg_gerr("Out of memory!\n"); - exit(1); + return -1; } *flash->chip = *chip; flash->mst = mst; -- To view, visit https://review.coreboot.org/c/flashrom/+/69474 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie4186a40071e9a7296d601582ff15ad7df09c70a Gerrit-Change-Number: 69474 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(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-MessageType: merged

1 0

[S] Change in flashrom[master]: tree/: Make heap alloc checks err msg consistent
by Felix Singer (Code Review) 06 Dec '22

06 Dec '22

Felix Singer has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69472 ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: tree/: Make heap alloc checks err msg consistent ...................................................................... tree/: Make heap alloc checks err msg consistent Change-Id: Id84a9f15c33781efc494ed36a1c7cec82a0333d6 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69472 Reviewed-by: Felix Singer <felixsinger(a)posteo.net> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M flashrom.c M layout.c 2 files changed, 16 insertions(+), 3 deletions(-) Approvals: build bot (Jenkins): Verified Felix Singer: Looks good to me, approved diff --git a/flashrom.c b/flashrom.c index 51a6b64..fde6d5a 100644 --- a/flashrom.c +++ b/flashrom.c @@ -454,7 +454,7 @@ const uint8_t erased_value = ERASED_VALUE(flash); if (!cmpbuf) { - msg_gerr("Could not allocate memory!\n"); + msg_gerr("Out of memory!\n"); exit(1); } memset(cmpbuf, erased_value, len); @@ -513,7 +513,7 @@ uint8_t *readbuf = malloc(len); if (!readbuf) { - msg_gerr("Could not allocate memory!\n"); + msg_gerr("Out of memory!\n"); return -1; } diff --git a/layout.c b/layout.c index be88428..2d18f83 100644 --- a/layout.c +++ b/layout.c @@ -145,7 +145,7 @@ tmp = malloc(sizeof(*tmp)); if (tmp == NULL) { - msg_gerr("Could not allocate memory"); + msg_gerr("Out of memory"); goto error; } -- To view, visit https://review.coreboot.org/c/flashrom/+/69472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id84a9f15c33781efc494ed36a1c7cec82a0333d6 Gerrit-Change-Number: 69472 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[master]: tree/: Make heap alloc checks err msg consistent
by Felix Singer (Code Review) 06 Dec '22

06 Dec '22

Attention is currently required from: Edward O'Callaghan. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69472 ) Change subject: tree/: Make heap alloc checks err msg consistent ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: > Yes probably, but you could do that as a separate change as that changes the control flow graph. Done -- To view, visit https://review.coreboot.org/c/flashrom/+/69472 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id84a9f15c33781efc494ed36a1c7cec82a0333d6 Gerrit-Change-Number: 69472 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Comment-Date: Tue, 06 Dec 2022 10:07:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: README: Add information about meson and link build instructions
by Anastasia Klimchuk (Code Review) 06 Dec '22

06 Dec '22

Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70176 ) Change subject: README: Add information about meson and link build instructions ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I am confused by 2 +1s :) Is there anything that you would like to improve here? (especially, Thomas?) Thank you! -- To view, visit https://review.coreboot.org/c/flashrom/+/70176 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2a27d8f2ba42e18be2485ae95bec1b4c874bb4f7 Gerrit-Change-Number: 70176 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 06 Dec 2022 06:12:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom: Skip read/write/erase/verify ops on inaccessable flash regions
by Nikolai Artemiev (Code Review) 06 Dec '22

06 Dec '22

Attention is currently required from: Edward O'Callaghan, Angel Pons. Nikolai Artemiev has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/70128 ) Change subject: flashrom: Skip read/write/erase/verify ops on inaccessable flash regions ...................................................................... Patch Set 9: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/70128/comment/c09b07a8_f360fc06 PS6, Line 11: platforms with active an CSME coprocessor. > Currently, the way to use flashrom to erase/write parts of a flash chip when some other regions are […] I agree, silently skipping regions could easily go unnoticed by users and probably isn't what most users would expect to happen in that case. I'm working on some patches to integrate this better with layout logic, but they're incremental changes over this implementation. I think we should disable region skipping by default (i.e. error out if there are any protected regions) and make it opt-in feature via a flashrom flag, similar to `force-i-want-a-brick`. -- To view, visit https://review.coreboot.org/c/flashrom/+/70128 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9b96fb04b863625d2c9f9a00b97c35b3ddb0871b Gerrit-Change-Number: 70128 Gerrit-PatchSet: 9 Gerrit-Owner: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Tue, 06 Dec 2022 04:03:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit December 2022