Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/56501 )
Change subject: tests: Add tests to erase a chip ......................................................................
Patch Set 5:
(6 comments)
Patchset:
PS2:
The newest test that I have added uses real spi_send_command, however some previous tests are mock […]
Yes, I remember that when function is wrapped, the real one is meant to be available as __real_function. Which would work if the function is explicitly called in test code, but in this case spi_send_command is called somewhere deep inside in the middle of do_erase operation, the test doesn't know when and how (and shouldn't know).
I did spend some time few months ago exploring wrap vs real situation, when I was writing my first tests. I never sent those very first tests though, because they were wrapping register_shutdown, and that was a conflict with init-shutdown tests that do real shutdown. When I was trying to alternate between wrap and mock in different tests (in the same executable), the code was quickly getting convoluted, but I couldn't make it work anyway :\ (searching the internet didn't help) I decided init-shutdown tests were more useful and chose them.
That was few months ago, when I was younger and less experienced (especially the latter), so maybe now I could make it work... but the code would have to be convoluted, I am worried about it a bit.
I am marking this comment unresolved, because there is a TODO for this patch one way or the other. Myself leaning to have two executables, but please tell me if you strongly disagree!
Patchset:
PS4:
Definitely split this like you suggested but the first test looks great now!
thanks!
File tests/chip.c:
https://review.coreboot.org/c/flashrom/+/56501/comment/d5062766_b7fd379b PS4, Line 25: int
`unsigned` unless negative unlocks encode something special?
Done
https://review.coreboot.org/c/flashrom/+/56501/comment/fe0f1521_98143b4f PS4, Line 52: memcpy(&g_chip_state.buf[start], buf, len);
validate len does not exceed the actual buffer length. […]
Done
https://review.coreboot.org/c/flashrom/+/56501/comment/cb0eca80_5dd8a180 PS4, Line 59: g_chip_state.unlock_calls++; : return 0;
this does depend on the chip I suppose but another possible behavior is the following: […]
I added this check, it makes sense to me. For the existing test, unlock only called once (we know because it is logged). If at some point in future this becomes an issue, we can have a look later then.
https://review.coreboot.org/c/flashrom/+/56501/comment/499a6fc9_f7449f95 PS4, Line 71: memset(&g_chip_state.buf[blockaddr], 0xff, blocklen);
ditto, ensure blocklen does not exceed the global buffer size.
Done
-
Anastasia Klimchuk (Code Review)