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 2:
(5 comments)
File tests/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/56601/comment/bb687adb_457cfbbc PS2, Line 198: CONFIG_CBFS_VERIFICATION=1 \
It would be great if you could build this test twice, once with CONFIG_CBFS_VERIFICATION=1 and once […]
Makes sense
File tests/lib/cbfs-verification-test.c:
https://review.coreboot.org/c/coreboot/+/56601/comment/a739f798_8af8ae7a PS2, Line 4: #define __noreturn
What's this? __noreturn comes from src/commonlib/bsd/include/commonlib/bsd/compiler. […]
This is here to block `__noreturn` effect on `die()` function, which falls `failed()` to signal an error in the code. Original `die()` has the `__noreturn` attribute, which can be pretty tricky to deal with if one wants to check if ode really fails using `expect_assert_failure()`. And, I see, what I did wrong in the `die()` implementation. `fail()` cannot be catched, but `(mock_)assert()` can.
https://review.coreboot.org/c/coreboot/+/56601/comment/ca869a7f_ecc8f5d9 PS2, Line 38: #define HASH_ATTR_SIZE (sizeof(struct cbfs_file_attr_hash))
Oh, this is the VB2_SUPPORT_SHA512 issue. Okay, yeah, this needs to be defined differently. […]
Looks good to me
https://review.coreboot.org/c/coreboot/+/56601/comment/169a0aff_44ba65b0 PS2, Line 121: {
These should also use the check...() functions to make sure they're only called when intended. […]
`ulzman()` and `ulz4fn()` will be called only for files with compression enabled and when the `cbfs_cache` is available (so, when CONFIG_ARCH_X86=0). Moreover the `cbfs_file_attr_compression` attribute is required.
I think, that this test does not aim to check if attributes (compression attr included) are correctly interpreted. In my opinion `fail()` is sufficient here. Compression will be tested in the `cbfs-lookup-test` (or similar name), which I am currently working on.
https://review.coreboot.org/c/coreboot/+/56601/comment/da7b643c_e91ba390 PS2, Line 167: assert_null(mapping);
I guess with the way the objcopy weaking mocks work now, you can't both mock a function and still call the underlying to make a little wrapper that checks arguments?
It is not possible. Weak symbol is dropped during linking. I tried swapping symbols using --redefine-sym, but it was not giving correct results.
Maybe using --add-symbol to define a (non-weak) __real_<functionname> symbol for the same address as each <functionname> that's marked as mockable?
The problem with --add-symbol is, we have to know, what address of <functionname> is. To do this, we have to scan all produced object files to find the exact file and address of that symbol.