View Change

11 comments:

• File tests/include/lib/fmap/fmap_config.h:

  • Patch Set #2:

    Always put a license line, even on generated files, just to be safe.

  • Patch Set #2, Line 7: * FMAP_OFFSET was changed to zero for testing purposes.

    Doesn't look like it's zero below?

  • Patch Set #2, Line 84: //#define FMAP_SECTION_UNUSED_HOLE_START 0xfff000

    ??

• File tests/include/lib/fmap/fmap_data.h:

  • Patch Set #2, 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.)

  • Patch Set #2, Line 145: 1568

    nit: just use sizeof()?

• File tests/lib/fmap-test.c:

  • Patch Set #2, 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?

  • Patch Set #2, Line 38: size = MIN(size, flash_buffer_size - offset);

    Doesn't the check above make this unnecessary?

  • Patch Set #2, 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.

  • Patch Set #2, Line 146: assert_int_not_equal(-1, fmap_locate_area_as_rdev("RO_VPD", rdev));

    Please test for actual offsets, not just != -1.

  • Patch Set #2, Line 161: }

    Test the returned rdev, too? Make sure you cannot rdev_writeat() the RO version?

  • Patch Set #2, 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?

To view, visit change 48557. To unsubscribe, or for help writing mail filters, visit settings.