Attention is currently required from: Jakub Czapiga, Jan Dabros. Julius Werner 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 4:
(13 comments)
File tests/lib/cbfs-lookup-test.c:
https://review.coreboot.org/c/coreboot/+/56813/comment/bbb7f06a_5b4b8d55 PS4, Line 12: #define HEADER_INITIALIZER(ftype, attr_len, file_len) { \ If you're reusing components of this CBFS mocking stuff in multiple tests, please factor them out into a common header.
https://review.coreboot.org/c/coreboot/+/56813/comment/2fd5473d_f05e54c9 PS4, Line 115: BE32(CBFS_FILE_ATTR_TAG_UNUSED), What's this? This isn't valid. Either you have no attributes (and then attributes_offset should be 0), or all attributes need to at least have a valid attribute header (tag and size field).
https://review.coreboot.org/c/coreboot/+/56813/comment/dde01c6f_c9b68b4a PS4, Line 139: }, These are all good positive tests but I'd really like to have some negative tests for all the metadata parsing code as well (eventually, doesn't all need to be in the same CL). Things I can think of are:
- CBFS file header at a non-CBFS_ALIGNMENT boundary - CBFS file where the data length goes beyond the end of the rdev - CBFS file where the attribute part of the metadata goes beyond the end of the rdev - CBFS file where the filename part of the metadata goes beyond the end of the rdev - CBFS file where the initial struct cbfs_file part goes beyond the end of the rdev - all of the above but with file type CBFS_TYPE_NULL - CBFS files where either len, attributes_offset or offset are (uint32_t)-1 or something like that - CBFS file where filename isn't null-terminated - CBFS file where attributes_offset is larger than offset - CBFS file where attributes_offset is smaller than sizeof(struct cbfs_file) - CBFS file where offset is smaller than sizeof(struct cbfs_file) - CBFS file where either of the above are exactly equal to sizeof(struct cbfs_file) (which essentially means the filename size is 0) - CBFS file where an attribute's length exceeds the total space available for attributes (as delimited by the `offset` field)
https://review.coreboot.org/c/coreboot/+/56813/comment/a4b9f578_888f10ea PS4, Line 148: __aligned(CBFS_ALIGNMENT); nit: technically, I don't see why this should need to be aligned? The only access function is cbfs_dev_read(), which just copies raw bytes out and doesn't care about alignment. (The alignment of the files to the start of the CBFS is important, but not the alignment of this whole array in memory.)
In fact, it might be a good idea to intentionally unalign it to make sure there is no alignment restriction where there's not intended to be one?
https://review.coreboot.org/c/coreboot/+/56813/comment/5a0ebcc0_f3ca487b PS4, Line 153: __weak extern u8 _ecbfs_cache[]; Why not just #include <symbols.h> so you can use the same declarations as the actual coreboot code?
https://review.coreboot.org/c/coreboot/+/56813/comment/92f2bba7_a63e64e5 PS4, Line 154: TEST_REGION(cbfs_cache, TEST_CBFS_CACHE_SIZE); If you're planning to link mem_pool and have a real cbfs_cache in here, I'd suggest doing the pass-through mocking (with __real_...) for the mem_pool functions as well, so that we can test that they are called exactly when and as expected.
https://review.coreboot.org/c/coreboot/+/56813/comment/d3feb15c_56ef4f23 PS4, Line 159: int create_cbfs(const struct cbfs_test_file **files, size_t nfiles) One of the lesser-tested properties of CBFS is that there may be an arbitrary amount of space behind a CBFS file until the next one starts (as long as it still starts on a CBFS_ALIGNMENT boundary. It would maybe be nice to fit that in here by spacing a few of these more than one alignment boundary apart?
https://review.coreboot.org/c/coreboot/+/56813/comment/49582e30_94d2c9a7 PS4, Line 167: if (&data_ptr[file_size] > &cbfs_buffer[ARRAY_SIZE(cbfs_buffer) - 1]) nit: this is checking your test data, not the code under test, so should probably just assert()?
https://review.coreboot.org/c/coreboot/+/56813/comment/be234bde_42a9001b PS4, Line 173: if ((uintptr_t)data_ptr % CBFS_ALIGNMENT != 0) This would of course need to check for the alignment of (data_ptr - cbfs_buffer), rather than data_ptr directly, if cbfs_buffer wasn't aligned.
https://review.coreboot.org/c/coreboot/+/56813/comment/453f2d1f_9e77fd6d PS4, Line 189: function_called(); nit: I think technically you don't really need function_called() when you already have a check_expected() or a mock(). Cmocka should fail the moment it runs into one of those when they haven't been previously primed by the test harness. (Unless of course you use the count -1 thing to assign permanent defaults to all the parameters.)
https://review.coreboot.org/c/coreboot/+/56813/comment/079d81c5_f806f642 PS4, Line 240: cbd.mcache_size = TEST_MCACHE_SIZE; This isn't actually testing the mcache? If you want to test with mcache, you have to cbfs_mcache_build() it first.
https://review.coreboot.org/c/coreboot/+/56813/comment/dd0fa5d5_34bd8e52 PS4, Line 253: extern uintptr_t _cbmem_top_ptr; I'd honestly consider just mocking out the whole CBMEM part here, its internals are completely isolated from this code and we already have tests for that. Why not just mock cbmem_add() to check its arguments and return a static buffer?
https://review.coreboot.org/c/coreboot/+/56813/comment/2660b64a_9cc66283 PS4, Line 308: size_out Please also test cbfs_map("filename", NULL) somewhere.