Nico Huber has posted comments on this change. ( https://review.coreboot.org/23021 )
Change subject: layout: Add -i <region>[:<file>] support. ......................................................................
Patch Set 5:
(15 comments)
https://review.coreboot.org/#/c/23021/5/cli_classic.c File cli_classic.c:
https://review.coreboot.org/#/c/23021/5/cli_classic.c@61 PS5, Line 61: <: rather :<
https://review.coreboot.org/#/c/23021/5/flashrom.c File flashrom.c:
https://review.coreboot.org/#/c/23021/5/flashrom.c@1320 PS5, Line 1320: const char *filename, const char *size_msg) Line limit is 112 chars for flashrom.
(not my first choice, though hadn't time to look at the discussion, back in 2011~2012 iirc)
https://review.coreboot.org/#/c/23021/5/flashrom.c@1364 PS5, Line 1364: #ifdef __LIBPAYLOAD__ : msg_gerr("Error: No file I/O support in libpayload\n"); : return 1; : #else Seems unnecessary, now that we use read_buf_from_file().
https://review.coreboot.org/#/c/23021/5/flashrom.c@1368 PS5, Line 1368: int ret = 0, i; `ret` is not used (as a variable) anymore.
https://review.coreboot.org/#/c/23021/5/flashrom.c@1379 PS5, Line 1379: } I'm a bit confused here. Do we allow such layouts at all? Will try to investigate myself...
https://review.coreboot.org/#/c/23021/5/flashrom.c@1387 PS5, Line 1387: if ((entry->start > size) || (entry->end > size)) { Checking `entry->end` should suffice.
https://review.coreboot.org/#/c/23021/5/flashrom.c@1391 PS5, Line 1391: } Again, do we allow such layouts?
https://review.coreboot.org/#/c/23021/5/flashrom.c@1484 PS5, Line 1484: #ifdef __LIBPAYLOAD__ : msg_gerr("Error: No file I/O support in libpayload\n"); : return 1; : #else Should be kept in do_write_buf_to_file() where the actual file handling happens. It also shows that flashrom.c is not the right place for this maybe worth a follow-up.
https://review.coreboot.org/#/c/23021/5/flashrom.c@1502 PS5, Line 1502: msg_gdbg("\n"); /* avoid printing in caller's output */ `i == 0` isn't always the first included region
https://review.coreboot.org/#/c/23021/5/flashrom.c@2660 PS5, Line 2660: /* : * This must be done before any files specified by -i arguments are : * processed and merged into newcontents since -i files take priority. : * See http://crbug.com/263495. : */ I don't understand it. The comment seems to assume that we call both after another?
Also we should catch it in the CLI in the next patch. So we have a proper error message in case both a -w filename and -i :files are given. Same for -v.
https://review.coreboot.org/#/c/23021/5/layout.c File layout.c:
https://review.coreboot.org/#/c/23021/5/layout.c@175 PS5, Line 175: -l/--layout This list might get very long...
https://review.coreboot.org/#/c/23021/5/layout.c@189 PS5, Line 189: /* Quotes or whitespace are not allowed in region names. */ why test it here? why add it now?
https://review.coreboot.org/#/c/23021/5/layout.c@202 PS5, Line 202: found++; As we bail out otherwise, `found` will always be `num_include_args` in the end?
https://review.coreboot.org/#/c/23021/5/layout.c@213 PS5, Line 213: #ifdef __LIBPAYLOAD__ : msg_gerr("Error: No file I/O support in libpayload\n"); : return 1; : #else libflashrom doesn't call it anyway, does it? The original guards only exist to make it compile (i.e. hide calls to i/o stream functions).
https://review.coreboot.org/#/c/23021/5/layout.c@217 PS5, Line 217: file = strdup(file); why? is it necessary? doesn't `file` point into `argv` of main()?
-
Nico Huber (Code Review)