November 2022 - flashrom-gerrit

[XL] Change in flashrom[master]: tree/: Convert flashchip erase_block func ptr to enumerate
by Angel Pons (Code Review) 02 Nov '22

02 Nov '22

Attention is currently required from: Felix Singer, Edward O'Callaghan, Anastasia Klimchuk. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67240 ) Change subject: tree/: Convert flashchip erase_block func ptr to enumerate ...................................................................... Patch Set 15: Code-Review+1 (3 comments) Patchset: PS15: Besides the at45db changes, looks OK. [Food for thought] We could use reproducible builds and some temporary macros to easily verify that the bulk of changes (e.g. flashchips.c) doesn't change any behavior. One way to do this is to make a reproducible commit (i.e. one that doesn't change the resulting flashrom binary) preceding this one that replaces the function references with macros that have the same names as the enum values. For example: #define SPI_ERASE_AT45DB_SECTOR &spi_erase_at45db_sector The point is that (un)wrapping stuff in macros doesn't change the resulting binaries (if done correctly). In this case, the resulting source files after macro expanding (i.e. preprocessing) should already be equivalent. Yes, these macros are *cursed*, but they're *temporary*. Think of them as support structures in 3D printing or some other kind of "scaffolding": adding and removing them is work, but it makes the job substantially simpler and less error-prone. The big question is... Does flashrom's build system (or the cacophony that having two build systems is) support reproducible builds? File at45db.c: https://review.coreboot.org/c/flashrom/+/67240/comment/3b10412f_ca55de40 PS15, Line 82: for (i = 0; i < NUM_ERASEFUNCTIONS; i++) { : const struct block_eraser *const eraser = &flash->chip->block_erasers[i]; : erasefunc_t *erase_func = lookup_erase_func_ptr_(eraser); : if (erase_func == &spi_erase_at45db_sector) { : for (j = 0; j < NUM_ERASEREGIONS; j++) { : cnt += eraser->eraseblocks[j].count; : } : } : } Is it necessary to refactor this *now*, i.e. in this commit? It doesn't seem to be the case, and it could cause regressions. Could we go with the simple approach here, and consider refactor in a follow-up change? ``` for (i = 0; i < NUM_ERASEFUNCTIONS; i++) { if (flash->chip->block_erasers[i].block_erase == SPI_ERASE_AT45DB_SECTOR) { for (j = 0; j < NUM_ERASEREGIONS; j++) { cnt += flash->chip->block_erasers[i].eraseblocks[j].count; } } } ``` That is, just replace `&spi_erase_at45db_sector` with `SPI_ERASE_AT45DB_SECTOR`. File include/flash.h: https://review.coreboot.org/c/flashrom/+/67240/comment/6f99a0a2_b36cf2b9 PS15, Line 307: TEST_ERASE_INJECTOR, /* special case must come last. */ What does this mean? Looks like it's for testing the code, maybe add a "Used in tests" note to the comment? -- To view, visit https://review.coreboot.org/c/flashrom/+/67240 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I58415a4f2589812b26d284c96876e88f8d9acf52 Gerrit-Change-Number: 67240 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-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 02 Nov 2022 00:26:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: meson: Move programmer test sources into programmer definition
by Felix Singer (Code Review) 02 Nov '22

02 Nov '22

Attention is currently required from: Thomas Heijligen, Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Thomas Heijligen, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/68162 to look at the new patch set (#14). Change subject: meson: Move programmer test sources into programmer definition ...................................................................... meson: Move programmer test sources into programmer definition Move the definition of the programmer test source files into the dictionary defining the programmers itself. This way there is a better overview about which of the available programmers have tests and which don't. Also, to keep the tests working, iterate over all programmers and add their test source files to the list of sources that should be built. Signed-off-by: Felix Singer <felixsinger(a)posteo.net> Change-Id: I307faaf8a9f7ae3c54bd96e7d871a3abb8aadea3 --- M meson.build M tests/meson.build 2 files changed, 34 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/68162/14 -- To view, visit https://review.coreboot.org/c/flashrom/+/68162 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I307faaf8a9f7ae3c54bd96e7d871a3abb8aadea3 Gerrit-Change-Number: 68162 Gerrit-PatchSet: 14 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: tests/meson.build: Rename list of source files to `test_srcs`
by Felix Singer (Code Review) 01 Nov '22

01 Nov '22

Felix Singer has submitted this change. ( https://review.coreboot.org/c/flashrom/+/68228 ) Change subject: tests/meson.build: Rename list of source files to `test_srcs` ...................................................................... tests/meson.build: Rename list of source files to `test_srcs` Rename the list of source files to `test_srcs` so that there is less confusion with the variable `srcs` from the top-level meson.build file containing the flashrom source files. Signed-off-by: Felix Singer <felixsinger(a)posteo.net> Change-Id: Ica0fc3923070bff63323204bd58edb5276dc9493 Reviewed-on: https://review.coreboot.org/c/flashrom/+/68228 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> --- M tests/meson.build 1 file changed, 20 insertions(+), 3 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved diff --git a/tests/meson.build b/tests/meson.build index c62cc1b..b0375a1 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -11,7 +11,7 @@ # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the # GNU General Public License for more details. -srcs = files( +test_srcs = files( 'io_mock.c', 'tests.c', 'libusb_wraps.c', @@ -34,7 +34,7 @@ ) if not programmer.get('dummy').get('active') - srcs += programmer.get('dummy').get('srcs') + test_srcs += programmer.get('dummy').get('srcs') endif mocks = [ @@ -110,7 +110,7 @@ ] flashrom_tests = executable('flashrom_unit_tests', - srcs, + test_srcs, c_args : [ cargs, '-ffunction-sections', -- To view, visit https://review.coreboot.org/c/flashrom/+/68228 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ica0fc3923070bff63323204bd58edb5276dc9493 Gerrit-Change-Number: 68228 Gerrit-PatchSet: 6 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: 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-MessageType: merged

1 0

[S] Change in flashrom[master]: tests/meson.build: Rename list of source files to `test_srcs`
by Angel Pons (Code Review) 01 Nov '22

01 Nov '22

Attention is currently required from: Felix Singer. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68228 ) Change subject: tests/meson.build: Rename list of source files to `test_srcs` ...................................................................... Patch Set 5: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68228 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ica0fc3923070bff63323204bd58edb5276dc9493 Gerrit-Change-Number: 68228 Gerrit-PatchSet: 5 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: 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-Comment-Date: Tue, 01 Nov 2022 23:58:00 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: meson: Move programmer test sources into programmer definition
by Felix Singer (Code Review) 01 Nov '22

01 Nov '22

Attention is currently required from: Thomas Heijligen, Angel Pons, Anastasia Klimchuk. Hello build bot (Jenkins), Thomas Heijligen, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/68162 to look at the new patch set (#13). Change subject: meson: Move programmer test sources into programmer definition ...................................................................... meson: Move programmer test sources into programmer definition Move the definition of the programmer test source files into the dictionary defining the programmers itself. This way there is a better overview about which of the available programmers have tests and which don't. Also, to keep the tests working, iterate over all programmers and add their test source files to the list of sources that should be built. Signed-off-by: Felix Singer <felixsinger(a)posteo.net> Change-Id: I307faaf8a9f7ae3c54bd96e7d871a3abb8aadea3 --- M meson.build M tests/meson.build 2 files changed, 34 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/68162/13 -- To view, visit https://review.coreboot.org/c/flashrom/+/68162 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I307faaf8a9f7ae3c54bd96e7d871a3abb8aadea3 Gerrit-Change-Number: 68162 Gerrit-PatchSet: 13 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: tests/meson.build: Rename list of source files to `test_srcs`
by Felix Singer (Code Review) 01 Nov '22

01 Nov '22

Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68228 ) Change subject: tests/meson.build: Rename list of source files to `test_srcs` ...................................................................... Patch Set 5: (1 comment) Patchset: PS5: Rebased this patch on master so that CB:68162 doesn't block this one. -- To view, visit https://review.coreboot.org/c/flashrom/+/68228 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ica0fc3923070bff63323204bd58edb5276dc9493 Gerrit-Change-Number: 68228 Gerrit-PatchSet: 5 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: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Comment-Date: Tue, 01 Nov 2022 23:38:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests/meson.build: Rename list of source files to `test_srcs`
by Felix Singer (Code Review) 01 Nov '22

01 Nov '22

Attention is currently required from: Felix Singer. Hello build bot (Jenkins), Thomas Heijligen, Angel Pons, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/68228 to look at the new patch set (#5). Change subject: tests/meson.build: Rename list of source files to `test_srcs` ...................................................................... tests/meson.build: Rename list of source files to `test_srcs` Rename the list of source files to `test_srcs` so that there is less confusion with the variable `srcs` from the top-level meson.build file containing the flashrom source files. Signed-off-by: Felix Singer <felixsinger(a)posteo.net> Change-Id: Ica0fc3923070bff63323204bd58edb5276dc9493 --- M tests/meson.build 1 file changed, 17 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/28/68228/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/68228 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ica0fc3923070bff63323204bd58edb5276dc9493 Gerrit-Change-Number: 68228 Gerrit-PatchSet: 5 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: 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-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: tests: Undefine _FORTIFY_SOURCE for unit tests to avoid _chk variants
by Peter Marheine (Code Review) 01 Nov '22

01 Nov '22

Attention is currently required from: Felix Singer, Thomas Heijligen, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68432 ) Change subject: tests: Undefine _FORTIFY_SOURCE for unit tests to avoid _chk variants ...................................................................... Patch Set 3: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/68432/comment/041f4abf_d856e2c7 PS3, Line 11: sence sense -- To view, visit https://review.coreboot.org/c/flashrom/+/68432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I70cb1cd90d1f377ff4606acad3c1b514120ae4f7 Gerrit-Change-Number: 68432 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> 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-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 01 Nov 2022 22:36:37 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: Undefine _FORTIFY_SOURCE for unit tests to avoid _chk variants
by Peter Marheine (Code Review) 01 Nov '22

01 Nov '22

Attention is currently required from: Felix Singer, Thomas Heijligen, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68432 ) Change subject: tests: Undefine _FORTIFY_SOURCE for unit tests to avoid _chk variants ...................................................................... Patch Set 3: Code-Review+1 (1 comment) Patchset: PS3: Should we go ahead and remove the existing _chk wraps as well? -- To view, visit https://review.coreboot.org/c/flashrom/+/68432 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I70cb1cd90d1f377ff4606acad3c1b514120ae4f7 Gerrit-Change-Number: 68432 Gerrit-PatchSet: 3 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> 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-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 01 Nov 2022 22:35:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

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: Edward O'Callaghan, Evan Benn. Hello build bot (Jenkins), Edward O'Callaghan, Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/68739 to look at the new patch set (#5). 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/5 -- 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: 5 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit November 2022