Attention is currently required from: Sam McNally, Anastasia Klimchuk, Evan Benn.
16 comments:
Commit Message:
Patch Set #2, Line 7: tests/: Add subregion alignment unit-test
is a subregion a thing? […]
Done
Patch Set #2, 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:
Patch Set #2, Line 110: va_list logfile_args;
you dont need to va_copy because you only pass ap to vfprintf
Ack
Patch Set #2, 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
Patch Set #2, Line 116: if (level <= verbose_screen) {
verbose_screen is part of `cli_output. […]
Ack
Patch Set #2, Line 118: /* msg_*spew often happens inside chip accessors in possibly
why do we need to fflush at all?
Ack
Patch Set #2, 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
Patch Set #2, 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.
Patch Set #2, 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.
Patch Set #2, Line 469: * {MOCK_CHIP_SUBREGION_CONTENTS} [..] {MOCK_CHIP_SUBREGION_CONTENTS}.
im not sure what this is telling me
Done
Patch Set #2, 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.
Patch Set #2, 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
Patch Set #2, 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
Patch Set #2, Line 502: printf("Subregion chip W op..\n");
write the word
Done
Patch Set #2, 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.
Patch Set #2, 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
To view, visit change 71659. To unsubscribe, or for help writing mail filters, visit settings.