Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48557 )
Change subject: tests: Add lib/fmap-test test case ......................................................................
Patch Set 2:
(11 comments)
https://review.coreboot.org/c/coreboot/+/48557/2/tests/include/lib/fmap/fmap... File tests/include/lib/fmap/fmap_config.h:
PS2: Always put a license line, even on generated files, just to be safe.
https://review.coreboot.org/c/coreboot/+/48557/2/tests/include/lib/fmap/fmap... PS2, Line 7: * FMAP_OFFSET was changed to zero for testing purposes. Doesn't look like it's zero below?
https://review.coreboot.org/c/coreboot/+/48557/2/tests/include/lib/fmap/fmap... PS2, Line 84: //#define FMAP_SECTION_UNUSED_HOLE_START 0xfff000 ??
https://review.coreboot.org/c/coreboot/+/48557/2/tests/include/lib/fmap/fmap... File tests/include/lib/fmap/fmap_data.h:
https://review.coreboot.org/c/coreboot/+/48557/2/tests/include/lib/fmap/fmap... PS2, Line 3: #ifndef TESTS_INCLUDE_LIB_FMAP_FMAP_DATA_H_ I don't think you should use tests/include/lib for this, because then all the headers in there clash paths with coreboot's normal include/lib/ (okay, I guess coreboot technically doesn't have an include/lib/ at the moment, but the name is still too generic). You should always use subdirectories that clearly indicate they're test headers and not normal parts of coreboot, like include/tests/ or include/stubs/ (I see this was already done for <lib/edid-test.h>... since that has the 'test' in the name it's not quite as bad, but I still think it should be moved to another subdirectory for consistency). (You can of course put it under include/tests/lib/ or something like that if you want to encode that it tests some of coreboot's lib/ code as well.)
https://review.coreboot.org/c/coreboot/+/48557/2/tests/include/lib/fmap/fmap... PS2, Line 145: 1568 nit: just use sizeof()?
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c File tests/lib/fmap-test.c:
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c@18 PS2, Line 18: static void *local_mmap(const struct region_device *rdev, size_t offset, size_t size) I think you could just use a mem_region_device instead of all of this?
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c@38 PS2, Line 38: size = MIN(size, flash_buffer_size - offset); Doesn't the check above make this unnecessary?
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c@84 PS2, Line 84: rdev_root = malloc(sizeof(rdev_new_root)); I think you can just write
rdev_root = (struct region_device)REGION_DEV_INIT(...);
instead of the copying? If not, you can also just assign the members manually, there's no rule that you *have* to use REGION_DEV_INIT() if it makes the code for confusing than open-coding it.
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c@146 PS2, Line 146: assert_int_not_equal(-1, fmap_locate_area_as_rdev("RO_VPD", rdev)); Please test for actual offsets, not just != -1.
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c@161 PS2, Line 161: } Test the returned rdev, too? Make sure you cannot rdev_writeat() the RO version?
https://review.coreboot.org/c/coreboot/+/48557/2/tests/lib/fmap-test.c@261 PS2, Line 261: FMAP_SECTION_FW_MAIN_A_SIZE Maybe make it smaller so you can also test that the remaining part of the region gets zeroed on overwrite?