Attention is currently required from: Sam McNally, Edward O'Callaghan, Anastasia Klimchuk.

View Change

17 comments:

• Commit Message:

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

  • 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

• 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

  • 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 splitting them into two different streams, curious to learn though

  • Patch Set #2, Line 116: if (level <= verbose_screen) {

    verbose_screen is part of `cli_output.c`, maybe something else here?

  • Patch Set #2, Line 118: /* msg_*spew often happens inside chip accessors in possibly

    why do we need to fflush at all?

  • 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 choose, but I think its just a 'region'.

    maybe the test is 'write_region_smaller_than_erase_granularity'

  • 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 ensuring it doesnt get modified accidently

  • Patch Set #2, Line 466: #define MOCK_CHIP_SUBREGION_CONTENTS 0xCC

    this can be a const int instead of a define

  • Patch Set #2, Line 469: * {MOCK_CHIP_SUBREGION_CONTENTS} [..] {MOCK_CHIP_SUBREGION_CONTENTS}.

    im not sure what this is telling me

  • Patch Set #2, Line 471: uint8_t *newcontents = calloc(1, mock_chip_size);

    memsetting so calloc could be malloc

  • 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

    https://source.chromium.org/chromiumos/chromiumos/codesearch/+/HEAD:src/third_party/flashrom/include/libflashrom.h;l=253

    or

    https://source.chromium.org/chromiumos/chromiumos/codesearch/+/HEAD:src/third_party/flashrom/include/flash.h;l=533

    so i dont actually know what this does

  • 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 tricky because the macros return early.

    maybe this should go at the unit test top level

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

  • Patch Set #2, Line 502: printf("Subregion chip W op..\n");

    write the word

  • 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?

  • 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.

    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.

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