Attention is currently required from: Julius Werner, Jan Dabros. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56813 )
Change subject: tests: Add lib/cbfs-lookup-test test case ......................................................................
Patch Set 10:
(14 comments)
File tests/lib/cbfs-lookup-test.c:
https://review.coreboot.org/c/coreboot/+/56813/comment/5395b8bf_425b0964 PS7, Line 120: #if CONFIG(NO_CBFS_MCACHE)
... […]
Yes, we are compiling with -Os, but if you want to debug tests, then compiling it with -O0 is helpful :)
https://review.coreboot.org/c/coreboot/+/56813/comment/9ffbc8e3_3f6d158d PS7, Line 439: &s->buf[5]
CBFS area begins with garbage data but eventually there comes a valid, correctly aligned file
Done: - test_cbfs_garbage_data_before_aligned_file - test_cbfs_garbage_data_before_unaligned_file
https://review.coreboot.org/c/coreboot/+/56813/comment/27fa409c_a5a7f092 PS7, Line 757: setup_test_cbfs_aligned
That's okay but there's still a bunch of duplication. […]
I added these macros: - CBFS_LOOKUP_TEST_FAIL_BEYOND_RDEV - CBFS_LOOKUP_ALIGNED_TEST_FAIL_BEYOND_RDEV - CBFS_LOOKUP_UNALIGNED_TEST_FAIL_BEYOND_RDEV
File tests/lib/cbfs-lookup-test.c:
https://review.coreboot.org/c/coreboot/+/56813/comment/ad0f0a48_50771114 PS9, Line 32: assert
nit: I actually meant a Cmocka assert macro like assert_true(). […]
Done
https://review.coreboot.org/c/coreboot/+/56813/comment/c4246e56_eeff218c PS9, Line 464: ALIGN_UP(sizeof(test_file_1), CBFS_ALIGNMENT)
second_file_start?
Yes. I forgot to replace it
https://review.coreboot.org/c/coreboot/+/56813/comment/3a50aa7e_04ee4dde PS9, Line 466: assert
Again would probably be better as assert_true()
Done
https://review.coreboot.org/c/coreboot/+/56813/comment/1ce15786_65bc818f PS9, Line 526: termminator
nit: typo, and an unmatched parenthesis earlier in the sentence, and anyway why is this comment dupl […]
Done
https://review.coreboot.org/c/coreboot/+/56813/comment/47731286_ea2c3b1b PS9, Line 551: + strlen(TEST_DATA_1_FILENAME)) + 2;
I am confused what this test is doing? Isn't this valid? You make the filename field strlen() + 2 by […]
Yes, this test should succeed. I places `+ 2` outside of cpu_to_be32(), and that (partialy) is why it fails.
https://review.coreboot.org/c/coreboot/+/56813/comment/34fb4285_79b9ab33 PS9, Line 552: f->header.len -= 2;
I'm not really sure what this is supposed to do anyway, but please be aware that you're doing math o […]
True, I forgot that this field has to be big endian. Fixed :)
https://review.coreboot.org/c/coreboot/+/56813/comment/4f2efd4b_741886bf PS9, Line 554: assert
nit: more raw assert()s... […]
Done
https://review.coreboot.org/c/coreboot/+/56813/comment/e08c07e7_90657393 PS9, Line 627: attribute structure. However, file will be found, because offset is correct. */ Done. Also fixed in test_cbfs_attributes_offset_cut_off_at_len()
BTW, having a few more thorough tests for cbfs_find_attr() in isolation, maybe in another test unit, would also be nice at some point.
This is indeed an important part of CBFS. I will take care of this in the near future :)
https://review.coreboot.org/c/coreboot/+/56813/comment/f29d5620_425e927c PS9, Line 738: const struct cbfs_test_file test_file = {
nit: this isn't wrong, but it's somewhat confusing why you're suddenly switching from "copy in and m […]
Done. Also for other tests.
https://review.coreboot.org/c/coreboot/+/56813/comment/0e2ae99f_775ae7ad PS9, Line 840: .init_res = CONFIG(NO_CBFS_MCACHE) ? CB_SUCCESS : CB_CBFS_IO,
Ahhh... hmmm... hrrmmmm... funny. That's not really how I wanted this to work, I think. […]
CB:57271 does fix this issue. Thank you :)
https://review.coreboot.org/c/coreboot/+/56813/comment/6d4b0c13_ae1cd9c5 PS9, Line 843: CB_CBFS_MOCK_ASSERT_FAILURE
I think(?) as far as I can tell you're only hitting this in the cases where cbfs_mcache_build() returns an error, and then you're trying to use the resulting mcache anyway.
That was the case. I was convinced, that it should fail, when cbfs_init_boot_device() fails to build MCache correctly. It's nice, that this bug was revealed.
CB:57271 allowed to get rid of CB_CBFS_MOCK_ASSERT_FAILURE.