Attention is currently required from: Simon Buhrow, Nico Huber, Thomas Heijligen, Paul Menzel, Aarya.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/65879 )
Change subject: flashrom.c:Add function to get a flattened view of the chip erase blocks ......................................................................
Patch Set 54:
(9 comments)
File erasure_layout.c:
https://review.coreboot.org/c/flashrom/+/65879/comment/75da22dc_7cc888bb PS54, Line 26: if (layout) { rewrite your function like this:
``` if (!layout) return;
for (unsigned int i = 0; i < erasefn_count; i++) { free(layout[i].layout_list); } free(layout); ```
https://review.coreboot.org/c/flashrom/+/65879/comment/9aad4e8a_84f60ab1 PS54, Line 34: ) The signature of this function should be,
``` unsigned int create_erase_layout(struct flashctx *const flashctx, struct erase_layout **elayout) { ```
ret is <0 on error, =0 on no erasefn_count's or >0 with no. of erasefn_count's.
https://review.coreboot.org/c/flashrom/+/65879/comment/4479918a_b8c1a4b2 PS54, Line 34: create_erase_layout with the comments below I am expecting a few static functions that should be relatively small and this function will stitch them together.
https://review.coreboot.org/c/flashrom/+/65879/comment/80303a73_0fb8a18e PS54, Line 48: ; \n
https://review.coreboot.org/c/flashrom/+/65879/comment/b7f4e7a5_9f0b45e8 PS54, Line 54: int j = 0; : while(addr < chip->total_size * 1024) { ok `j` is shadowed below. Rewrite this while-loop construct as a `for` loop construct as:
``` for (unsigned i = 0; addr < chip->total_size * 1024; i++) { const struct eraseblock *block = &chip->block_erasers[eraser_idx].eraseblocks[i]; block_count += block->count; addr += block->size * block->count; } ``` to avoid shadowing or scope to a helper function.
* The key message I am communicating here is minimum scope.
The other secondary message I am pealing back at is how these loops can become infinite or undefined.
https://review.coreboot.org/c/flashrom/+/65879/comment/8df0ae7e_206596a2 PS54, Line 55: (
trivial: `e (`
Ack
https://review.coreboot.org/c/flashrom/+/65879/comment/59ca8312_cfe70e94 PS54, Line 62: (struct eraseblock_data *)malloc use `calloc()`.
Is the construction of `layout[layout_idx].layout_list` on the heap it's own function? It would make digesting how deep these loops are tractable as to maintain this code.
https://review.coreboot.org/c/flashrom/+/65879/comment/4ac13ba1_a5da5a8f PS54, Line 86: (layout[layout_idx-1].layout_list[sub_block_index].start_addr >= start_addr && : layout[layout_idx-1].layout_list[sub_block_index].end_addr <= end_addr && : sub_block_index < layout[layout_idx-1].block_count) in this new function made out of this `if`-block turn this wild predicate into a intermediate with a name.
https://review.coreboot.org/c/flashrom/+/65879/comment/fff59680_94925c32 PS54, Line 84: if (layout_idx > 0) { : layout[layout_idx].layout_list[block_num].first_sub_block_index = sub_block_index; : while (layout[layout_idx-1].layout_list[sub_block_index].start_addr >= start_addr && : layout[layout_idx-1].layout_list[sub_block_index].end_addr <= end_addr && : sub_block_index < layout[layout_idx-1].block_count) { : sub_block_index++; : } : layout[layout_idx].layout_list[block_num].last_sub_block_index = sub_block_index - 1; : } looks like another function with a base case of `layout_idx == 0 => return`.