Attention is currently required from: Julius Werner, Jan Dabros. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56601 )
Change subject: tests: Add lib/cbfs-verification-test test case ......................................................................
Patch Set 8:
(9 comments)
File tests/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/56601/comment/2d209490_fb5424e8 PS7, Line 35: tests-y += cbfs-verification-no-sha512-test
Should explain the rationale for testing both cases in a comment somewhere.
Done
https://review.coreboot.org/c/coreboot/+/56601/comment/854c1222_e6f0c5a0 PS7, Line 188: cbfs-verification-sources := tests/lib/cbfs-verification-test.c \
I wonder if it makes sense to define a copy_test function for this sort of stuff, so that you could […]
Please see CB:56932
https://review.coreboot.org/c/coreboot/+/56601/comment/ff862803_e7ac7df1 PS7, Line 198: 3rdparty/vboot/firmware/include
Shouldn't we just put these into the default include path for tests? They are in the default include […]
Yes, those paths were missing. I added them in CB:56933
https://review.coreboot.org/c/coreboot/+/56601/comment/f3d37ebf_e0b58189 PS7, Line 219: CONFIG_CBFS_VERIFICATION=1
Wasn't the point that this should be 0 here?
Yes, it should. Copy-paste error...
File tests/lib/cbfs-verification-test.c:
https://review.coreboot.org/c/coreboot/+/56601/comment/b3b598fd_c044254d PS7, Line 147: {
Actually, I think it would be better to properly mock this too (comparing buf and size to expected v […]
Done
https://review.coreboot.org/c/coreboot/+/56601/comment/a56b367f_62bf100e PS7, Line 216: &file_valid_hash.attrs_and_data[HASH_ATTR_SIZE], HASH_ATTR_SIZE);
Uhhh... wait a second... does this test succeed? Because it shouldn't succeed. […]
That's because of a typo in the test configuration. Fixed :)
https://review.coreboot.org/c/coreboot/+/56601/comment/ac95683c_d960be25 PS7, Line 243: assert_null(mapping);
And this should return a valid mapping because verification is disabled. […]
Done
https://review.coreboot.org/c/coreboot/+/56601/comment/6124fce2_cfcadbbf PS7, Line 283: /* No cache, so no validation required */
(nit: comment still makes no sense here)
True. Removed
https://review.coreboot.org/c/coreboot/+/56601/comment/6843ecd1_072cf469 PS7, Line 288: has_mcache
Well, you can either define NO_CBFS_MCACHE, or you can test mcache stuff here. Not both.
Ack