Nico Huber has posted comments on this change. ( https://review.coreboot.org/23021 )
Change subject: layout: Add -i <region>[:<file>] support. ......................................................................
Patch Set 7:
(10 comments)
There are still some issues when it comes to the updated behaviour due to that http://crbug.com/263495
* Manpage needs update. * Separation of this and the next commit makes no sense because the introduced if/else relies on the file arguments to -w/-v being optional. * I think we need a check for the case that both files to -w/-v and to -i are given (it seems to me the latter would be silently ignored, didn't test though).
https://review.coreboot.org/#/c/23021/7/flashrom.8.tmpl File flashrom.8.tmpl:
https://review.coreboot.org/#/c/23021/7/flashrom.8.tmpl@227 PS7, Line 227: write spurious
https://review.coreboot.org/#/c/23021/7/flashrom.8.tmpl@229 PS7, Line 229: For write operations, files specified using : .B "-i" : take precedence over content from the argument to : .B "-w." That's no more true. -i and -w filenames are mutually exclusive, right?
https://review.coreboot.org/#/c/23021/7/flashrom.8.tmpl@252 PS7, Line 252: .B " flashrom -p prog -l <layout> -i foo:foo.bin -i bar:bar.bin -w rom.bin Hmmm, just realized that this doesn't work. The -i files are just ignored when -w gets a file passed.
I'd be fine if you document the final behaviour here. But maybe it's yet easier to squash the two commits...
https://review.coreboot.org/#/c/23021/7/flashrom.c File flashrom.c:
https://review.coreboot.org/#/c/23021/7/flashrom.c@1380 PS7, Line 1380: if (!entry->included || !entry->file) : continue; Optionally:
if (!entry->included) continue; if (!entry->file) { msg_gerr("Error: No image file given for region "%s".\n", entry->name); return 1; }
I know this is already covered in the next commit for the CLI, but I would feel much better if the code here looks consistent.
https://review.coreboot.org/#/c/23021/7/flashrom.c@1383 PS7, Line 1383: buf + entry->start ?
https://review.coreboot.org/#/c/23021/7/flashrom.c@1464 PS7, Line 1464: const char *filename) Should fit a single line.
https://review.coreboot.org/#/c/23021/7/flashrom.c@2647 PS7, Line 2647: * processed and merged into newcontents since -i files take priority. This comment still makes no sense to me. I'd guess it was written before the if / else decision was introduced.
https://review.coreboot.org/#/c/23021/7/layout.c File layout.c:
https://review.coreboot.org/#/c/23021/7/layout.c@239 PS7, Line 239: 0 i + 1 ?
https://review.coreboot.org/#/c/23021/7/layout.c@244 PS7, Line 244: continue; Not needed if we start with `j > i`.
https://review.coreboot.org/#/c/23021/7/layout.c@272 PS7, Line 272: for (i = 0; i < layout.num_entries; i++) { : free(layout.entries[i].file); : layout.entries[i].file = NULL; : layout.entries[i].included = 0; : } : layout.num_entries = 0; This should actually be done in flashrom_layout_release() (before the `if (layout == get_global_layout())` check).
It's a bit ugly atm, because nobody took the time yet, to untangle the CLI parts and the static layout struct here. Hmmm, now it doesn't look like much work: read_romlayout() would allocate a new layout, normalize_romentries() should work on the layout linked to the `flashctx`. Everything else doesn't seem to use the static struct.