Attention is currently required from: Sam McNally, Edward O'Callaghan, Anastasia Klimchuk.
Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71659 )
Change subject: tests/: Add subregion alignment unit-test ......................................................................
Patch Set 2:
(17 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/71659/comment/a709f11e_40075213 PS2, Line 7: tests/: Add subregion alignment unit-test is a subregion a thing?
maybe it is not the alignment but the size relative to erase sizes thats important?
https://review.coreboot.org/c/flashrom/+/71659/comment/7fba71d2_252033e7 PS2, Line 9: A written region that is sized below that of the erasure granularity I don't understand the bug from this description
File tests/chip.c:
https://review.coreboot.org/c/flashrom/+/71659/comment/5586f951_4838fef1 PS2, Line 110: va_list logfile_args; you dont need to va_copy because you only pass ap to vfprintf
https://review.coreboot.org/c/flashrom/+/71659/comment/bc8150ef_1cbe26a8 PS2, Line 114: output_type = stderr; I don't know the motivation for printing the messages, so I also dont know the motivation for splitting them into two different streams, curious to learn though
https://review.coreboot.org/c/flashrom/+/71659/comment/1e0e9263_e6658fa8 PS2, Line 116: if (level <= verbose_screen) { verbose_screen is part of `cli_output.c`, maybe something else here?
https://review.coreboot.org/c/flashrom/+/71659/comment/fb83b4b7_1e1024df PS2, Line 118: /* msg_*spew often happens inside chip accessors in possibly why do we need to fflush at all?
https://review.coreboot.org/c/flashrom/+/71659/comment/4a7a5156_58a84076 PS2, Line 444: void write_chip_subregion_with_dummyflasher_test_success(void **state) // XXX this is the first mention of a 'subregion' in flashrom, maybe there is an existing word you can choose, but I think its just a 'region'.
maybe the test is 'write_region_smaller_than_erase_granularity'
https://review.coreboot.org/c/flashrom/+/71659/comment/942e87bc_b03b1f47 PS2, Line 458: struct flashchip mock_chip = chip_W25Q128_V; do you want to assert something about the size of the erasers here? as much for documentation as ensuring it doesnt get modified accidently
https://review.coreboot.org/c/flashrom/+/71659/comment/389cfb5d_b58bb732 PS2, Line 466: #define MOCK_CHIP_SUBREGION_CONTENTS 0xCC this can be a const int instead of a define
https://review.coreboot.org/c/flashrom/+/71659/comment/005ad273_3e93ca71 PS2, Line 469: * {MOCK_CHIP_SUBREGION_CONTENTS} [..] {MOCK_CHIP_SUBREGION_CONTENTS}. im not sure what this is telling me
https://review.coreboot.org/c/flashrom/+/71659/comment/45070264_b17b9b3a PS2, Line 471: uint8_t *newcontents = calloc(1, mock_chip_size); memsetting so calloc could be malloc
https://review.coreboot.org/c/flashrom/+/71659/comment/49603d7c_332879b5 PS2, Line 477: flashrom_flag_set(&flashctx, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, false); suspect these flags are important what is being tested, but its not documented here or
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/HEAD:src/thir...
or
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/HEAD:src/thir...
so i dont actually know what this does
https://review.coreboot.org/c/flashrom/+/71659/comment/56f20945_29dc8e05 PS2, Line 478: flashrom_set_log_callback((flashrom_log_callback *)&unittest_print_cb); why are we setting up a print callback? We also dont seem to undo this at the end, which will be tricky because the macros return early.
maybe this should go at the unit test top level
https://review.coreboot.org/c/flashrom/+/71659/comment/d91320c0_1f4bb962 PS2, Line 485: flashrom_layout_release(layout); flashctx->layout is dangling. could set it to default_layout or NULL, which seem to do the same thing but neither way is documented
https://review.coreboot.org/c/flashrom/+/71659/comment/2fe11234_1c758365 PS2, Line 502: printf("Subregion chip W op..\n"); write the word
https://review.coreboot.org/c/flashrom/+/71659/comment/ac67e596_0ffdf83e PS2, Line 508: //flashrom_layout_set(&flashctx, NULL); // use default layout. the layout you use is the same as the default or NULL layout, does that not work?
https://review.coreboot.org/c/flashrom/+/71659/comment/f11e3614_95d8a45d PS2, Line 514: assert_int_equal(0, flashrom_image_verify(&flashctx, newcontents, mock_chip_size)); maybe documenting right here what the predicted bug is would be useful.
does the test fail here or at the write step?
Does this test catch a real bug on some patch? I would like to have a look.
-
Evan Benn (Code Review)