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 8:
(24 comments)
File tests/lib/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/56813/comment/9f463047_fec1397e PS7, Line 222: cbmem_top
Unused now?
Yes
https://review.coreboot.org/c/coreboot/+/56813/comment/6b51ede9_11932a63 PS7, Line 226: CONFIG_CBFS_VERIFICATION=0
This is the default anyway (and should remain so for the foreseeable future).
If that is so, then we can remove it :)
File tests/lib/cbfs-lookup-test.c:
https://review.coreboot.org/c/coreboot/+/56813/comment/71736a8e_8962630c PS7, Line 35: BE64
nit: as long as it's test data it doesn't really matter, but be aware that while CBFS header fields […]
Thanks for clarifying that. I was treating BE64() macro as "to-bytes" in this context. I added "LE32/LE64()" to conform with cbfstool convention when it comes to integer values.
https://review.coreboot.org/c/coreboot/+/56813/comment/b1e946a4_e6e42d89 PS7, Line 80: break By placing "break" here I wanted to ignore case when someone has entered too much "spaces" (NULLs) at the end of input files list. But yes, you are right. It should be an assert(). I wrongly assumed that this if will be triggered only for NULL at the end of input file list.
Could pull this assert() out below the if-else block, I think...
It would require another if-statement because of memcpy() call in the else-block. Moreover, I *think* that this repeated assert() can be actually helpful when debugging, because one can see if an error was caused by file or by alignment marker (NULL).
https://review.coreboot.org/c/coreboot/+/56813/comment/ba3faf7c_5667a725 PS7, Line 90: if (offset % CBFS_ALIGNMENT != 0)
nit: technically don't need to check first, ALIGN_UP() will do the right thing either way.
Oh, right. This is probably leftover from my manual alignment attempts (to make it work with aligned and unaligned buffer)
https://review.coreboot.org/c/coreboot/+/56813/comment/d93b96e8_ed344eeb PS7, Line 120: #if CONFIG(NO_CBFS_MCACHE)
Should be no need to only compile one of the two?
For default tests cflags there is no problem, and I think that it is actually good idea to compile both mocks. I had to add src/commonlib/bsd/cbfs_mcache.c to sources for case without MCache, for it to compile correctly (no stripping with -O0).
https://review.coreboot.org/c/coreboot/+/56813/comment/baf396cf_c8e114e3 PS7, Line 154: {
nit: might as well throw in an assert_ptr_equal(mp, _cbfs_cache) to both of these?
Done
https://review.coreboot.org/c/coreboot/+/56813/comment/1aab3be5_81504c13 PS7, Line 185: size_t sz;
nit: I'd maybe call these cbfs_buf and cbfs_size or img_buf and img_size, to make it more obvious in […]
Done
https://review.coreboot.org/c/coreboot/+/56813/comment/7cd05842_614d7000 PS7, Line 198: s->sz = sizeof(aligned_cbfs_buffer);
Maybe zero out s->ex just in case?
Done
https://review.coreboot.org/c/coreboot/+/56813/comment/6f2f211e_f1f955e7 PS7, Line 248: #define expect_lookup_result(res) \
nit: can't this just be a function?
It can :)
https://review.coreboot.org/c/coreboot/+/56813/comment/019e9b53_5b03cba7 PS7, Line 439: &s->buf[5]
So, this is okay but now you have an image that doesn't even start with a CBFS header, which could a […]
Oh, I thought that one case includes other, but you are right. I left this case, but renamed it to "test_cbfs_image_not_aligned". It might be useful too. Regarding unaligned file, I adjusted this test as follows: - memcpy valid file at the beginning of CBFS - memcpy valid file at invalid (misaligned) offset after previous file - first file should be found, second not
https://review.coreboot.org/c/coreboot/+/56813/comment/93f834ad_73d6b65e PS7, Line 472: assert_int_equal(0, size_out);
the caller should just assume size_out is undefined if the mapping fails, so tests shouldn't check it in that case.
Ok, I removed this check from other test cases as well. It is true, we should not assume, that size_out was not changed inside _cbfs_alloc()
https://review.coreboot.org/c/coreboot/+/56813/comment/f1a29c42_e5e75bec PS7, Line 475: static void test_cbfs_fail_beyond_rdev(void **state)
This whole construction is so complicated that I think you should build a success case for it as wel […]
Done. Success test case added. Now this test can be parametrized by: - file_type - file_length -> point in file, where to cut rdev - init_res -> result of cbfs_init_boot_device - lookup_res -> result of lookup function
https://review.coreboot.org/c/coreboot/+/56813/comment/988ad95e_597ef2fd PS7, Line 491: full_file_size
I believe you want sizeof(test_file_to_go_beyond) here, because full_file_size may be much larger (a […]
You are right. I have to be more careful.
https://review.coreboot.org/c/coreboot/+/56813/comment/fe15e968_e657f558 PS7, Line 492: test_file_1
Below you're looking for TEST_DATA_2_FILENAME, so I think you want &test_file_2?
Oh, yeah. Typo
https://review.coreboot.org/c/coreboot/+/56813/comment/29f9424a_e21ac0f7 PS7, Line 526: mapping = cbfs_map("abcdefghijklmnop", &size_out);
This is a good test to have, but it could still fail to check a problem where the code just strcmp() […]
Done: - \0 not included in filename limited by file offset - \0 after offset - \0 filename shorter than limited by offset (I know, that other cases test it, but this test can explicitly show if there is an error with this particular case)
https://review.coreboot.org/c/coreboot/+/56813/comment/10b91acd_e3b34cf9 PS7, Line 537: const struct cbfs_test_file test_file = {
nit: maybe easier to copy in one of your pre-existing files and then just change one field?
Done. Now test requires test_data_2 to have `header.attributes_offset != 0`. I *think* that it won't be a problem. If one will change test data, then all tests have to be adjusted as well.
https://review.coreboot.org/c/coreboot/+/56813/comment/af621969_5a79e245 PS7, Line 545: .offset = cpu_to_be32(sizeof(struct cbfs_file) + FILENAME_SIZE),
Now that I think of it it would also be good to have another test where the attribute is cut off in […]
Done: - test_cbfs_attributes_offset_cut_off_at_len() - test_cbfs_attributes_offset_cut_off_at_data()
https://review.coreboot.org/c/coreboot/+/56813/comment/45259523_171a43a7 PS7, Line 617: .attributes_offset = cpu_to_be32(
This should have no attributes so it doesn't overlap so much with the other test above.
Ack
https://review.coreboot.org/c/coreboot/+/56813/comment/807c4475_2d235d43 PS7, Line 730: static void test_cbfs_length_uint32max(void **state)
Would be nice to have this for attributes_offset as well.
Done
https://review.coreboot.org/c/coreboot/+/56813/comment/06deab72_76c018b8 PS7, Line 757: setup_test_cbfs_aligned
can you use the group_setup/group_teardown stuff from cmocka_run_group_tests() to cover this?
I can but it will still require memset-zero on buffer and mcache, so it would just increate amount of code. To reduce repeating code I created macros: - CBFS_LOOKUP_TEST - CBFS_LOOKUP_ALIGNED_NAME_PRESTATE_TEST - CBFS_LOOKUP_ALIGNED_NAME_TEST - CBFS_LOOKUP_ALIGNED_TEST (same for unaligned)
https://review.coreboot.org/c/coreboot/+/56813/comment/33d4a036_e26b54ce PS7, Line 851: attrs_and_data
e.g. either 4 or 8 bytes further in, to see if having only the attribute tag or the tag and size throw it off somehow.
Done
I designed this test incorrectly, so I had to rewrite it (including adding new parameters in test state extension struct). I had to cut cbfs rdev to (&second_file + offset) to make it work correctly. It might look a bit ugly, because I am changing region size by hand at this point. If you see any way to improve it, please let me know in comment in `test_cbfs_fail_beyond_rdev`
https://review.coreboot.org/c/coreboot/+/56813/comment/880e8eb1_b90024d0 PS7, Line 852: CBFS_TYPE_NULL
You say that but your code doesn't do it.
True. Fixed
https://review.coreboot.org/c/coreboot/+/56813/comment/e92f57a6_0364ff85 PS7, Line 866: filename
Same here, testing when the cutoff is a few bytes further in (but before the null terminator) would […]
Done
-
Jakub Czapiga (Code Review)