Nico Huber has posted comments on this change. ( https://review.coreboot.org/23263 )
Change subject: Add support for reading the current flash contents from a file ......................................................................
Patch Set 4:
(7 comments)
We are close :) though, flashrom_image_write() seems to be too convoluted to be extended easily and needs some more attention.
https://review.coreboot.org/#/c/23263/4/cli_classic.c File cli_classic.c:
https://review.coreboot.org/#/c/23263/4/cli_classic.c@114 PS4, Line 114: C: You can just drop the C: and it should work. The 'C' below only tells getopt_long() what to return in case of --flash-contents which could be any distinct number. Though, better would be to use a non-character number like --ifd does, e.g. 0x0101. Ideally, we would use an enum for these numbers like this:
enum { OPTION_IFD = 0x0100, OPTION_FLASH_CONTENTS, };
https://review.coreboot.org/#/c/23263/4/flashrom.c File flashrom.c:
https://review.coreboot.org/#/c/23263/4/flashrom.c@2349 PS4, Line 2349: * @param refbuffer If given, assume flash contains same data as `refbuffer` Full stop please, as the lines above have that too.
https://review.coreboot.org/#/c/23263/4/flashrom.c@2349 PS4, Line 2349: flash `flash chip` please, for consistent wording.
https://review.coreboot.org/#/c/23263/4/flashrom.c@2357 PS4, Line 2357: void Can be (and therefore should be) const.
https://review.coreboot.org/#/c/23263/4/flashrom.c@2397 PS4, Line 2397: oldcontents = refcontents; I would rather copy the data here. Would allow to keep the surrounding code as is. You'd have to keep track of where `oldcontents` points to, otherwise, e.g. to make sure you don't free() `refbuffer`.
Also, I just realized that you need to copy the data into `curcontents` as well. `oldcontents` is only optionally used for verification later and not for the checks which parts of the flash chip have to be updated. Basically, after this if/else block, `curcontents` has to contain valid data at least for all included regions in the layout, and `oldcontents` has to contain valid data for the whole flash if `verify_all` is set.
With that written down, you can/should only copy into `oldcontents` if `verify_all` is set (in other words if `oldcontents != NULL`).
https://review.coreboot.org/#/c/23263/4/flashrom.c@2561 PS4, Line 2561: uint8_t *refcontents = NULL; Please move up to the general declarations above (`newcontents` is only declared later because it couldn't be `const` otherwise, stupid language if you ask me, or stupid me because I always try to make as much as possible `const`).
https://review.coreboot.org/#/c/23263/4/flashrom.c@2582 PS4, Line 2582: _free_ret_refcontents: You only need a single `_free_ret` as `free(NULL);` is valid.