Attention is currently required from: Felix Singer, Hsuan-ting Chen, Edward O'Callaghan, Nikolai Artemiev. Sam McNally has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59714 )
Change subject: util/cbfstool: Port elogtool to libflashrom ......................................................................
Patch Set 10:
(6 comments)
File util/cbfstool/uflashrom.c:
https://review.coreboot.org/c/coreboot/+/59714/comment/63fa3005_251ee898 PS10, Line 25: if (level != FLASHROM_MSG_SPEW) This condition will never be false given the early return above. I'm not sure the fflush() is needed given only stderr is used.
https://review.coreboot.org/c/coreboot/+/59714/comment/6951a3fc_129f471b PS10, Line 38: size_t new_size = size - offset; I suspect not finding the end of the region will confuse the parts of elogtool attempting to manage the elog size.
https://review.coreboot.org/c/coreboot/+/59714/comment/17703a29_382843f5 PS10, Line 73: /* empty region causes seg fault in API. */ This comment seems a bit lost. Perhaps hoist it to the if (region) above, or remove it.
https://review.coreboot.org/c/coreboot/+/59714/comment/af43ac37_217fb141 PS10, Line 90: /* FIXME: The flashrom_image_read() API starts the write into the buffer This seems like a slightly strange place to opine about the libflashrom API. That said, this expectation doesn't scale nicely to more than one region - at least, not by default - a "packed" option might be reasonable to request such treatment.
In my view, the critical problem is that the offset and length of a region cannot be retrieved; a packed option (with corresponding offset query adjustments) would be more of a nice convenience.
https://review.coreboot.org/c/coreboot/+/59714/comment/2883f50b_e4ff12c0 PS10, Line 145: r |= flashrom_layout_include_region(layout, region); This has the inverse problem of the read path: libflashrom expects the region in the buffer to be at its offset in the layout, but the buffer provided by elog_write() is just the contents of that region.
I guess the flashrom_layout_read_fmap_from_buffer() call above could never be expected to succeed unless the region happens to be the FMAP region.
https://review.coreboot.org/c/coreboot/+/59714/comment/de3d00e7_f2e7f19a PS10, Line 154: flashrom_flag_set(flashctx, FLASHROM_FLAG_VERIFY_WHOLE_CHIP, true); I think this would be false on the old implementation.
-
Sam McNally (Code Review)