Attention is currently required from: Sam McNally, Daniel Campello, Anastasia Klimchuk.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63227 )
Change subject: tests: assert pathname and flags when calling open()
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/63227
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb
Gerrit-Change-Number: 63227
Gerrit-PatchSet: 9
Gerrit-Owner: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 05 Apr 2022 04:32:37 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Hung-Te Lin, Nico Huber, Subrata Banik, Edward O'Callaghan.
Daniel Campello has uploaded a new patch set (#20) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/62213 )
Change subject: flashrom.c: Implement file-based locking semantics
......................................................................
flashrom.c: Implement file-based locking semantics
This upstreams the ChromiumOS implementation of file-based locking
for multiple instances of flashrom that could be spawned either
from libflashrom (perhaps by fwupd for example) and user cli
as another example. Since flashrom is programming singleton state
of hardware from userspace there is no way to exclusively own
the address space and therefore a file-based locking semantic
is considered here.
BUG=b:217629892,b:215255210
BRANCH=none
TEST=nm -gD /build/brya/usr/lib64/libflashrom.so | grep "flock"
Test futility update with multiple instances of flashrom running.
Change-Id: I19cb4e3bf14caeb67c3e8100a20395b264c5113a
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M Makefile
A big_lock.c
A big_lock.h
A file_lock.c
M flashrom.c
A ipc_lock.h
M meson.build
M tests/chip.c
M tests/include/test.h
M tests/lifecycle.c
M tests/meson.build
M tests/tests.c
12 files changed, 492 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/13/62213/20
--
To view, visit https://review.coreboot.org/c/flashrom/+/62213
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I19cb4e3bf14caeb67c3e8100a20395b264c5113a
Gerrit-Change-Number: 62213
Gerrit-PatchSet: 20
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-CC: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Hung-Te Lin <hungte(a)chromium.org>
Gerrit-Attention: Nico Huber <nico.h(a)gmx.de>
Gerrit-Attention: Subrata Banik <subratabanik(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Sam McNally, Edward O'Callaghan, Anastasia Klimchuk.
Daniel Campello has uploaded a new patch set (#12) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/62320 )
Change subject: tests/: Add file path validation to open() calls
......................................................................
tests/: Add file path validation to open() calls
With this change we add path validation to many tests that do not call
open. Expected path is set to NULL, if the code indead calls open then
the assertion for non-NULL will make the test fail.
BUG=b:217629892,b:215255210
TEST=`ninja test`.
Change-Id: I892fa1ecee26ebce9640893edbb228fa9aa7b0b6
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Co-Author: Daniel Campello <campello(a)chromium.org>
Signed-off-by: Daniel Campello <campello(a)chromium.org>
---
M tests/chip.c
M tests/lifecycle.c
2 files changed, 121 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/20/62320/12
--
To view, visit https://review.coreboot.org/c/flashrom/+/62320
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I892fa1ecee26ebce9640893edbb228fa9aa7b0b6
Gerrit-Change-Number: 62320
Gerrit-PatchSet: 12
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Sam McNally, Anastasia Klimchuk.
Daniel Campello has uploaded a new patch set (#12) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/62319 )
Change subject: tests/linux_spi: Validate param file path
......................................................................
tests/linux_spi: Validate param file path
Add path validation for '/dev/null' to open operation.
BUG=b:217629892,b:215255210
TEST=`ninja test`.
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Co-Author: Daniel Campello <campello(a)chromium.org>
Signed-off-by: Daniel Campello <campello(a)chromium.org>
Change-Id: If5d24c65f291c53a35509fea5d2f5b3fdb51c306
---
M tests/lifecycle.c
1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/19/62319/12
--
To view, visit https://review.coreboot.org/c/flashrom/+/62319
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If5d24c65f291c53a35509fea5d2f5b3fdb51c306
Gerrit-Change-Number: 62319
Gerrit-PatchSet: 12
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Sam McNally, Anastasia Klimchuk.
Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/63227
to look at the new patch set (#9).
Change subject: tests: assert pathname and flags when calling open()
......................................................................
tests: assert pathname and flags when calling open()
With this change the wrappers for mock and friends are able to take an
optional io_mock_fallback_open_state struct to assert expected pathnames
and flags whenever an open operation is called.
Based partially on https://review.coreboot.org/c/flashrom/+/62319/5
BUG=b:227404721,b:217629892,b:215255210
TEST=./test_build.sh; FEATURES=test emerge-amd64-generic flashrom
BRANCH=none
Signed-off-by: Daniel Campello <campello(a)chromium.org>
Co-Author: Edward O'Callaghan <quasisec(a)google.com>
Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb
---
M tests/io_mock.c
M tests/io_mock.h
M tests/lifecycle.c
M tests/tests.c
4 files changed, 48 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/27/63227/9
--
To view, visit https://review.coreboot.org/c/flashrom/+/63227
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb
Gerrit-Change-Number: 63227
Gerrit-PatchSet: 9
Gerrit-Owner: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Sam McNally, Edward O'Callaghan, Anastasia Klimchuk.
Daniel Campello has uploaded a new patch set (#11) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/62320 )
Change subject: tests/: Add file path validation to open() calls
......................................................................
tests/: Add file path validation to open() calls
With this change we add path validation to many tests that do not call
open. Expected path is set to NULL, if the code indead calls open then
the assertion for non-NULL will make the test fail.
BUG=b:217629892,b:215255210
TEST=`ninja test`.
Change-Id: I892fa1ecee26ebce9640893edbb228fa9aa7b0b6
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Co-Author: Daniel Campello <campello(a)chromium.org>
Signed-off-by: Daniel Campello <campello(a)chromium.org>
---
M tests/chip.c
M tests/lifecycle.c
2 files changed, 121 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/20/62320/11
--
To view, visit https://review.coreboot.org/c/flashrom/+/62320
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I892fa1ecee26ebce9640893edbb228fa9aa7b0b6
Gerrit-Change-Number: 62320
Gerrit-PatchSet: 11
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Sam McNally, Anastasia Klimchuk.
Daniel Campello has uploaded a new patch set (#11) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/62319 )
Change subject: tests/linux_spi: Validate param file path
......................................................................
tests/linux_spi: Validate param file path
Add path validation for '/dev/null' to open operation.
BUG=b:217629892,b:215255210
TEST=`ninja test`.
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Co-Author: Daniel Campello <campello(a)chromium.org>
Signed-off-by: Daniel Campello <campello(a)chromium.org>
Change-Id: If5d24c65f291c53a35509fea5d2f5b3fdb51c306
---
M tests/lifecycle.c
1 file changed, 6 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/19/62319/11
--
To view, visit https://review.coreboot.org/c/flashrom/+/62319
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: If5d24c65f291c53a35509fea5d2f5b3fdb51c306
Gerrit-Change-Number: 62319
Gerrit-PatchSet: 11
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Sam McNally, Daniel Campello.
Hello Sam McNally, build bot (Jenkins), Edward O'Callaghan,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/63227
to look at the new patch set (#8).
Change subject: tests: assert pathname and flags when calling open()
......................................................................
tests: assert pathname and flags when calling open()
With this change the wrappers for mock and friends are able to take an
optional io_mock_open_state struct to assert expected pathnames and
flags whenever an open operation is called.
Based partially on https://review.coreboot.org/c/flashrom/+/62319/5
BUG=b:227404721,b:217629892,b:215255210
TEST=./test_build.sh; FEATURES=test emerge-amd64-generic flashrom
BRANCH=none
Signed-off-by: Daniel Campello <campello(a)chromium.org>
Co-Author: Edward O'Callaghan <quasisec(a)google.com>
Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb
---
M tests/io_mock.c
M tests/io_mock.h
M tests/lifecycle.c
M tests/tests.c
4 files changed, 48 insertions(+), 17 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/27/63227/8
--
To view, visit https://review.coreboot.org/c/flashrom/+/63227
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb
Gerrit-Change-Number: 63227
Gerrit-PatchSet: 8
Gerrit-Owner: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Sam McNally, Daniel Campello.
Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/63227 )
Change subject: tests: assert pathname and flags when calling open()
......................................................................
Patch Set 7:
(10 comments)
Patchset:
PS7:
Thanks!
File tests/io_mock.h:
https://review.coreboot.org/c/flashrom/+/63227/comment/31c81a69_f2ccfdf0
PS7, Line 63: /* Maximum number of open to mock. */
The number is probably arbitrary? If yes, you can add this clarification into a comment, "the number is arbitrary".
This way, if in future we need more calls, it will be easier to increase the number (no need to search history for the reasons why the number is this).
https://review.coreboot.org/c/flashrom/+/63227/comment/4466f4c5_bc827d07
PS7, Line 64: 3
If you are in a good mood, let's make this number power of two :) like 4 for example.
https://review.coreboot.org/c/flashrom/+/63227/comment/8fe149e6_7e375ab7
PS7, Line 118: open_state
Another thing: could you please add a comment above this line, to explain what that is, something like
"An alternative to custom open mock, a test can register one of: either its own mock open function or fallback_open".
https://review.coreboot.org/c/flashrom/+/63227/comment/52561be1_1ce4126d
PS7, Line 118: struct io_mock_open_state *open_state;
This is, as I understand from the code, an alternative to custom open mocking, and mutually exclusive (a test can either mock open function or setup open state). Let's have more descriptive naming for this.
For example:
struct fallback_open_state *fallback_open
I remember we agreed with Edward on "fallback" name in some earlier patch, let's use this name.
File tests/io_mock.c:
https://review.coreboot.org/c/flashrom/+/63227/comment/bf96023f_0039c312
PS7, Line 24: assert_true(io == NULL || io->open == NULL || io->open_state == NULL);
I would also add a comment above this line, to make it crystal clear:
"A test can register only one of: either custom mock for open function, or fallback open state".
File tests/tests.c:
https://review.coreboot.org/c/flashrom/+/63227/comment/d90d19f8_415e9955
PS7, Line 77: wrap_open_and_friends
Lets just call it "mock_open"
https://review.coreboot.org/c/flashrom/+/63227/comment/5e81318a_fdf67e4f
PS7, Line 81:
Could you please add new line between two conditionals? It would be easier to read.
https://review.coreboot.org/c/flashrom/+/63227/comment/5e1008fe_a5743e9c
PS7, Line 84:
Also here, please add new line between declaration of flags and asserts
https://review.coreboot.org/c/flashrom/+/63227/comment/f1e7592e_4acdbec9
PS7, Line 91:
Also here, please add new line before return statement.
--
To view, visit https://review.coreboot.org/c/flashrom/+/63227
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ib46ca5b854c8453ec02ae09f3151cd4d25f988eb
Gerrit-Change-Number: 63227
Gerrit-PatchSet: 7
Gerrit-Owner: Daniel Campello <campello(a)chromium.org>
Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Sam McNally <sammc(a)google.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org>
Gerrit-Attention: Sam McNally <sammc(a)google.com>
Gerrit-Attention: Daniel Campello <campello(a)chromium.org>
Gerrit-Comment-Date: Tue, 05 Apr 2022 03:58:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment