Attention is currently required from: Simon Buhrow, Thomas Heijligen, Aarya.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/71173 )
Change subject: flashrom.c: Add new erase_by_layout and walk_by_layout implementations ......................................................................
Patch Set 6: Code-Review+2
(13 comments)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/71173/comment/72895725_cc3f516b PS6, Line 1546: , erasefn_count); `, count_usable_erasers(flashctx));`
https://review.coreboot.org/c/flashrom/+/71173/comment/b706c028_fe325ac0 PS6, Line 1554: else you don't need a `else`
https://review.coreboot.org/c/flashrom/+/71173/comment/2c08f614_cc688774 PS6, Line 1681: int ret = 0; if you use `ret = 1` as the default you exchange many `ret = 1`'s with one `ret = 0`.
https://review.coreboot.org/c/flashrom/+/71173/comment/e907466c_3313af85 PS6, Line 1683: const struct romentry *entry = NULL; pair with while loop construct.
https://review.coreboot.org/c/flashrom/+/71173/comment/a0360be1_3f602b59 PS6, Line 1684: struct erase_layout *erase_layout = create_erase_layout(flashctx); If you can help it, pair with the branch below. Don't do C89'ism where you try to put all your declarations at the top.
``` const struct flashrom_layout *const flash_layout = get_layout(flashctx); struct erase_layout *erase_layout = create_erase_layout(flashctx); if (!flash_layout || !erase_layout) { goto _ret; } ```
https://review.coreboot.org/c/flashrom/+/71173/comment/a6a1ae1a_5a09e89f PS6, Line 1685: //no layout delete
https://review.coreboot.org/c/flashrom/+/71173/comment/cf32f3b3_a90c7fb2 PS6, Line 1690: //erase layout creation failed delete
https://review.coreboot.org/c/flashrom/+/71173/comment/b3f6ccca_b995331d PS6, Line 1695: //not enough memory : if (!curcontents || !newcontents) { : ret = 1; : goto _ret; : } delete?
https://review.coreboot.org/c/flashrom/+/71173/comment/896d286e_2f1631ee PS6, Line 1701: erase_write( line-wrap:
``` erase_write(flashctx, entry->region.start, entry->region.end, curcontents, newcontents, erase_layout, all_skipped); ```
https://review.coreboot.org/c/flashrom/+/71173/comment/524ba946_5bf0cc55 PS6, Line 1701: (uint8_t*) don't cast, just use the correct type in the first place in the function definition.
https://review.coreboot.org/c/flashrom/+/71173/comment/9243a6b1_20e4d549 PS6, Line 1703: !!! one `!` is sufficient or be consistent with the above and have none?
https://review.coreboot.org/c/flashrom/+/71173/comment/f73cbf64_17ae7ac3 PS6, Line 1708: free_erase_layout(erase_layout, erasefn_count); `free_erase_layout(erase_layout, count_usable_erasers(flashctx));`
Although I think the API of the free function should not require a length, it should be able to walk the data structure until exhaustion of the `NULL` case.
https://review.coreboot.org/c/flashrom/+/71173/comment/ad55ad7d_3f5f7da1 PS6, Line 1718: else ditto