Change in flashrom[master]: Add support for reading the current flash contents from a file
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 2:
(13 comments)
A general question: Would it make sense to update the given reference file after a successful flash?
https://review.coreboot.org/#/c/23263/2/cli_classic.c File cli_classic.c:
https://review.coreboot.org/#/c/23263/2/cli_classic.c@60 PS2, Line 60: " --ifd read layout from an Intel Firmware Descriptor\n" This line is too long now for a 80-columns terminal. Though, I think we can just avoid to shift all these lines, see below.
https://review.coreboot.org/#/c/23263/2/cli_classic.c@63 PS2, Line 63: referencefile Maybe just shorten it, e.g. <ref-file>, and leave the other lines as they were.
https://review.coreboot.org/#/c/23263/2/cli_classic.c@63 PS2, Line 63: -C I'm not sure about the `-C` maybe only add the long option for now?
https://review.coreboot.org/#/c/23263/2/cli_classic.c@363 PS2, Line 363: contents "reference"
https://review.coreboot.org/#/c/23263/2/flashrom.c File flashrom.c:
https://review.coreboot.org/#/c/23263/2/flashrom.c@2348 PS2, Line 2348: * @param buffer_len Size of source buffer in bytes. reference parameter should be mentioned here
https://review.coreboot.org/#/c/23263/2/flashrom.c@2355 PS2, Line 2355: int flashrom_image_write(struct flashctx *const flashctx, void *const buffer, const size_t buffer_len, const char *referencefile) I just realized that this is already part of the libflashrom interface which should be kept free of file i/o handling. You should pass another buffer here, already filled with the contents of the reference file, e.g. `refcontents` to align with `oldcontents`/`newcontents`.
https://review.coreboot.org/#/c/23263/2/flashrom.c@2400 PS2, Line 2400: */ This justification makes sense in the commit message but doesn't belong here. Please condense it to a single line, e.g.
/* If given, assume flash contains same data as `refcontents`. */
https://review.coreboot.org/#/c/23263/2/flashrom.c@2401 PS2, Line 2401: spurious empty line
https://review.coreboot.org/#/c/23263/2/flashrom.c@2402 PS2, Line 2402: if (referencefile) { : msg_cinfo("Reading old flash chip contents from file... "); : if (read_buf_from_file(oldcontents, flash_size, referencefile)) { : ret = 1; : msg_cinfo("FAILED.\n"); : goto _finalize_ret; : } move into do_write() please
https://review.coreboot.org/#/c/23263/2/flashrom.c@2410 PS2, Line 2410: spurious empty line
https://review.coreboot.org/#/c/23263/2/flashrom.c@2418 PS2, Line 2418: spurious empty line
https://review.coreboot.org/#/c/23263/2/flashrom.c@2438 PS2, Line 2438: if (verify_all) { : msg_cerr("Checking if anything has changed.\n"); : msg_cinfo("Reading current flash chip contents... "); : if (!flashctx->chip->read(flashctx, curcontents, 0, flash_size)) { : msg_cinfo("done.\n"); : if (!memcmp(oldcontents, curcontents, flash_size)) { : nonfatal_help_message(); : goto _finalize_ret; : } : msg_cerr("Apparently at least some data has changed.\n"); : } else : msg_cerr("Can't even read anymore!\n"); misaligned by accident?
https://review.coreboot.org/#/c/23263/2/flashrom.c@2580 PS2, Line 2580: goto _free_ret; Same as `filename`, `referencefile` should be read here if given.
-
Nico Huber (Code Review)