Nico Huber has posted comments on this change. ( https://review.coreboot.org/23021 )
Change subject: layout: Add -i <region>[:<file>] support. ......................................................................
Patch Set 13:
(8 comments)
https://review.coreboot.org/#/c/23021/13/flash.h File flash.h:
https://review.coreboot.org/#/c/23021/13/flash.h@303 PS13, Line 303: const Nit, `const` qualification has no meaning in the forward declaration.
https://review.coreboot.org/#/c/23021/13/flashrom.c File flashrom.c:
https://review.coreboot.org/#/c/23021/13/flashrom.c@1383 PS13, Line 1383: return 1; This was probably added because of my misinterpretation that we don't allow mixing a full-image file with per-region files. Should be reverted to the simple `continue;` it was. All such constraints should be handled in a higher level.
https://review.coreboot.org/#/c/23021/13/flashrom.c@1470 PS13, Line 1470: write_buf_to_file Nit, rather write_buf_to_file*s*?
https://review.coreboot.org/#/c/23021/13/flashrom.c@1500 PS13, Line 1500: if (ret) : goto out; mhmm?
https://review.coreboot.org/#/c/23021/13/flashrom.c@1506 PS13, Line 1506: #endif oops
https://review.coreboot.org/#/c/23021/13/flashrom.c@2651 PS13, Line 2651: if (get_num_include_args_with_files(get_layout(flash)) > 0) { Why check this explicitly? I'd expect read_buf_from_include_args() to do nothing by definition if the count is 0.
https://review.coreboot.org/#/c/23021/13/flashrom.c@2676 PS13, Line 2676: *files missing space
https://review.coreboot.org/#/c/23021/13/layout.c File layout.c:
https://review.coreboot.org/#/c/23021/13/layout.c@233 PS13, Line 233: for (j = 0; j < l->num_entries; j++) { As mentioned in PS7, `j` should start at `i + 1` otherwise you will check each pair (x, y) twice, once with i=x, j=y and once with j=x and i=y.
-
Nico Huber (Code Review)