David Hendricks has posted comments on this change. ( https://review.coreboot.org/19342 )
Change subject: layout: Add -i <region>[:<file>] support. ......................................................................
Patch Set 3: Code-Review-1
(3 comments)
still need to address Nico's comments
https://review.coreboot.org/#/c/19342/1/flashrom.c File flashrom.c:
PS1, Line 1330: /** : * @brief Writes included layout regions into buffer(s) : * : * If partial read to file is to be done (-i <region>[:<regionfile>] syntax : * used) then this will write to corresponding region-sized files. If an : * argument to -r/-w/-v is specified then it will write a chip-sized file. Both : * partial read/writes and full read/write may be done at once. : * : * @param buf Buffer with data to write : * @param size Size of buffer : * @param filename Filename corresponding to optional argument to -r/-w/-v : * @return 0 on success, : * 1 if any read fails. : */ This can be deleted.
Line 1444: extra line
https://review.coreboot.org/#/c/19342/2/layout.h File layout.h:
PS2, Line 43: chip
`const char`? we'd never want to change it, would we?
Yeah, there seems to have been something lost in translation. The original chromiumos version uses a statically allocated buffer and copies the filename into it, the ported version changes the 'region:file' string so that the colon is replaced by '\0' and file points at the first character in the filename.
Bear in mind that name and file both come from optarg which we currently strdup(). I think the cleaner way to do this would be to: 1. Pass optarg into register_include_arg() 2. Find the colon 3. strdup() the region name and file name. 4. free() the two strings as needed in layout_cleanup().
And yes, const is a good idea (for both of them, I think).