Attention is currently required from: Sam McNally, Anastasia Klimchuk, Evan Benn.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71659 )
Change subject: tests/: Add non-aligned write within a region unit-test ......................................................................
Patch Set 2:
(16 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/71659/comment/0901f4f6_097bdf43 PS2, Line 7: tests/: Add subregion alignment unit-test
is a subregion a thing? […]
Done
https://review.coreboot.org/c/flashrom/+/71659/comment/6ca559fc_d50b4381 PS2, Line 9: A written region that is sized below that of the erasure granularity
I don't understand the bug from this description
should make more sense now given all the other fixes give context.
File tests/chip.c:
https://review.coreboot.org/c/flashrom/+/71659/comment/2dbdad01_75b44fb5 PS2, Line 110: va_list logfile_args;
you dont need to va_copy because you only pass ap to vfprintf
Ack
https://review.coreboot.org/c/flashrom/+/71659/comment/f25d06f3_9a15fed8 PS2, Line 114: output_type = stderr;
I don't know the motivation for printing the messages, so I also dont know the motivation for splitt […]
Ack
https://review.coreboot.org/c/flashrom/+/71659/comment/96e19d18_041e83d5 PS2, Line 116: if (level <= verbose_screen) {
verbose_screen is part of `cli_output. […]
Ack
https://review.coreboot.org/c/flashrom/+/71659/comment/9f190440_a96060c8 PS2, Line 118: /* msg_*spew often happens inside chip accessors in possibly
why do we need to fflush at all?
Ack
https://review.coreboot.org/c/flashrom/+/71659/comment/04957694_341b54a0 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 choo […]
Done
https://review.coreboot.org/c/flashrom/+/71659/comment/20aeae7f_05b0d69b 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 en […]
Well that should all be part of the W25Q128V chip spec this struct is defining. That sounds like another test to add to ensure the data structure is maintained or a comment above it. Either way, maybe out of scope here.
https://review.coreboot.org/c/flashrom/+/71659/comment/4c742e8a_f82500af PS2, Line 466: #define MOCK_CHIP_SUBREGION_CONTENTS 0xCC
this can be a const int instead of a define
well not quite in this case because reasons, however I added a comment.
https://review.coreboot.org/c/flashrom/+/71659/comment/5b635c7d_837de663 PS2, Line 469: * {MOCK_CHIP_SUBREGION_CONTENTS} [..] {MOCK_CHIP_SUBREGION_CONTENTS}.
im not sure what this is telling me
Done
https://review.coreboot.org/c/flashrom/+/71659/comment/2ba94eae_f9400a56 PS2, Line 471: uint8_t *newcontents = calloc(1, mock_chip_size);
memsetting so calloc could be malloc
I prefer consistent use of calloc() that ensures heap is always zero. malloc nano optimisation for a uninit heap gains me nothing over the advantage of consistency.
https://review.coreboot.org/c/flashrom/+/71659/comment/05d64979_94ba7308 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 […]
Documenting those flags better is outside the scope of this change. However I agree they need better doxygen comments in libflashrom.h
https://review.coreboot.org/c/flashrom/+/71659/comment/e80af7bd_d27a532c 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 tri […]
Ack
https://review.coreboot.org/c/flashrom/+/71659/comment/9b3356db_91ef6bed PS2, Line 502: printf("Subregion chip W op..\n");
write the word
Done
https://review.coreboot.org/c/flashrom/+/71659/comment/24fbe3ff_33366af9 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?
No, it segfauls for some unknown reason outside the scope of the work here.
https://review.coreboot.org/c/flashrom/+/71659/comment/692915fe_1814f6a8 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. […]
Done
-
Edward O'Callaghan (Code Review)