Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68433 )
Change subject: tests: Add prefix to io_mock functions not to clash with macros
......................................................................
Patch Set 5:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/68433/comment/2d813b15_5fee52ea
PS5, Line 10: because the latter are allowed
: to be macros. Adding a prefix to flashrom mock functions avoids
: them being accidentally expanded. Standard I/O functions are
: expanded and flashrom mocks stays as they are
I don't understand this yet. I have some questions:
* Why is it a problem that standard IO functions are macros?
* How does the prefix prevent that the flashrom mocks are expanded? I don't understand the relation between that function pointer and the standard I/O functions.
* Shouldn't undefining _FORTIFY_SOURCE solve that problem? In my understanding standard I/O functions are not expanded when _FORTIFY_SOURCE is undefined.
https://review.coreboot.org/c/flashrom/+/68433/comment/2731a9ce_9ca9658f
PS5, Line 13: stays
typo: stay (without s)
File tests/io_mock.h:
https://review.coreboot.org/c/flashrom/+/68433/comment/4584ff5e_6e5b08c6
PS5, Line 73: /* Port I/O */
: void (*outb)(void *state, unsigned char value, unsigned short port);
: unsigned char (*inb)(void *state, unsigned short port);
:
: void (*outw)(void *state, unsigned short value, unsigned short port);
: unsigned short (*inw)(void *state, unsigned short port);
:
: void (*outl)(void *state, unsigned int value, unsigned short port);
: unsigned int (*inl)(void *state, unsigned short port);
:
: /* USB I/O */
: int (*libusb_init)(void *state, libusb_context **ctx);
: int (*libusb_control_transfer)(void *state,
: libusb_device_handle *devh,
: uint8_t bmRequestType,
: uint8_t bRequest,
: uint16_t wValue,
: uint16_t wIndex,
: unsigned char *data,
: uint16_t wLength,
: unsigned int timeout);
: ssize_t (*libusb_get_device_list)(void *state, libusb_context *, libusb_device ***list);
: void (*libusb_free_device_list)(void *state, libusb_device **list, int unref_devices);
: int (*libusb_get_device_descriptor)(void *state, libusb_device *, struct libusb_device_descriptor *);
: int (*libusb_get_config_descriptor)(void *state,
: libusb_device *,
: uint8_t config_index,
: struct libusb_config_descriptor **);
: void (*libusb_free_config_descriptor)(void *state, struct libusb_config_descriptor *);
I am wondering, should these functions be renamed as well?
--
To view, visit https://review.coreboot.org/c/flashrom/+/68433
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I7998a8fb1b9e65621e12adbfab5460a245d5606b
Gerrit-Change-Number: 68433
Gerrit-PatchSet: 5
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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 02 Nov 2022 04:22:02 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Evan Benn has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/68739 )
Change subject: flashrom_tester: Use tempdir crate for test data
......................................................................
Abandoned
--
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-MessageType: abandon
Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk.
Felix Singer 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 5: Code-Review+2
--
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: 5
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: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Wed, 02 Nov 2022 03:58:34 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Evan Benn.
Edward O'Callaghan 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 5: Code-Review-1
(1 comment)
File util/flashrom_tester/src/tester.rs:
https://review.coreboot.org/c/flashrom/+/68739/comment/c3fc7ef8_71e17e76
PS5, Line 87:
: original_flash_contents: tmp_dir.path().join("flashrom_tester_golden.bin"),
:
The critical issue with this patch is that it can leave a DUT irrecoverable should the test suite fail in some way or get stopped with no ability to inspect the pre-run state and use that to recover with.
The principle idea behind the patch is good but it needs more care to not regress the requirement of postmortem test analysis of artifacts.
--
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: Evan Benn <evanbenn(a)google.com>
Gerrit-Comment-Date: Wed, 02 Nov 2022 03:30:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Evan Benn.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69064 )
Change subject: flashrom_tester: Use path types for things that are paths
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69064/comment/33712aac_5bfa8074
PS2, Line 9: string
> See above, 'String' vs 'string' have very specific meanings. […]
Updated commit message no longer says anything about strings or Strings.
--
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: 3
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: 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: Wed, 02 Nov 2022 03:19:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: comment
Attention is currently required from: Angel Pons, Anastasia Klimchuk.
Hello build bot (Jenkins), Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69131
to look at the new patch set (#2).
Change subject: tree/: Convert flashchips db to use indirection for erase_block
......................................................................
tree/: Convert flashchips db to use indirection for erase_block
This paves the way to allow for the conversion of flashchip erase_block
func ptr to enumerate values. This change should be a NOP.
TEST=`diff -u <(objdump -D flashchips.o_bk) <(objdump -D flashchips.o)`.
Change-Id: I122295ec9add0fe0efd27273c9725e5d64f6dbe2
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M at45db.c
M atapromise.c
M flashchips.c
M flashrom.c
M include/flash.h
M sfdp.c
M spi25.c
M tests/chip.c
M tests/chip_wp.c
9 files changed, 2,022 insertions(+), 1,971 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/31/69131/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/69131
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I122295ec9add0fe0efd27273c9725e5d64f6dbe2
Gerrit-Change-Number: 69131
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Angel Pons, Anastasia Klimchuk.
Hello build bot (Jenkins), Angel Pons,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69133
to look at the new patch set (#2).
Change subject: tree/: Convert flashchip erase_block func ptr to enumerate [ALT]
......................................................................
tree/: Convert flashchip erase_block func ptr to enumerate [ALT]
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: I02ae7e4c67c5bf34ec2fd7ffe4af8a2aba6fd5e5
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M at45db.c
M flashrom.c
M include/chipdrivers.h
M include/flash.h
M sfdp.c
M spi25.c
M tests/chip.c
7 files changed, 135 insertions(+), 50 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/69133/2
--
To view, visit https://review.coreboot.org/c/flashrom/+/69133
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I02ae7e4c67c5bf34ec2fd7ffe4af8a2aba6fd5e5
Gerrit-Change-Number: 69133
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
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 5: Code-Review+1
--
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: 5
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: Wed, 02 Nov 2022 02:27:35 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Thomas Heijligen, Peter Marheine.
Anastasia Klimchuk 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 5:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/68432/comment/f1ea4fdb_40148912
PS3, Line 11: sence
> sense
Done
Patchset:
PS3:
> Should we go ahead and remove the existing _chk wraps as well?
Yes good idea, removed.
--
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: 5
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: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Wed, 02 Nov 2022 02:26:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Thomas Heijligen, Anastasia Klimchuk.
Hello Felix Singer, build bot (Jenkins), Thomas Heijligen, Angel Pons, Peter Marheine,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/68432
to look at the new patch set (#5).
Change subject: tests: Undefine _FORTIFY_SOURCE for unit tests to avoid _chk variants
......................................................................
tests: Undefine _FORTIFY_SOURCE for unit tests to avoid _chk variants
The option _FORTIFY_SOURCE, when enabled, can result in some functions
being expanded into _chk variants. For example, `fprintf` can get
expanded into `__fprintf_chk`. This makes sense for building a real
binary, but is not needed for unit tests.
In unit test environment all those functions are wrapped. In the
example above, both `fprintf` and `__fprintf_chk` needed to be mocked.
Disabling _FORTIFY_SOURCE avoids expanding functions into _chk
variants, without any loss of testing coverage because that would
be wrapped/mocked anyway.
This patch also removes two existing _chk wraps because they are not
needed anymore.
BUG=b:237606255
TEST=ninja test on
1) gcc 12.2.0 on Debian
2) clang 15.0 on Chromium OS
Ticket: https://ticket.coreboot.org/issues/411
Change-Id: I70cb1cd90d1f377ff4606acad3c1b514120ae4f7
Signed-off-by: Anastasia Klimchuk <aklm(a)chromium.org>
---
M tests/meson.build
M tests/tests.c
2 files changed, 31 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/32/68432/5
--
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: 5
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-MessageType: newpatchset