Attention is currently required from: Sam McNally, Felix Singer, Hsuan-ting Chen, Nikolai Artemiev. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59714 )
Change subject: util/cbfstool: Port elogtool to libflashrom ......................................................................
Patch Set 10:
(4 comments)
File util/cbfstool/uflashrom.c:
https://review.coreboot.org/c/coreboot/+/59714/comment/955dc63e_9865b876 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 end is padded out with flash erasure values [0xFF].
https://review.coreboot.org/c/coreboot/+/59714/comment/e1ac5228_50029c49 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.
No because this is where the seg fault would occur. This comment is consistent with the futility drv as well. I rather keep it consistent
https://review.coreboot.org/c/coreboot/+/59714/comment/956f84a3_34e3396d 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. […]
API discussion aside, this was to give context on what is happening here and not to attempt to design the API itself.
https://review.coreboot.org/c/coreboot/+/59714/comment/32d90e16_7fb186bc 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 […]
What is the AI you are asking for here?
-
Edward O'Callaghan (Code Review)