Attention is currently required from: Richard Hughes, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Patrick Rudolph. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49643 )
Change subject: libflashrom: Return progress state to the library user ......................................................................
Patch Set 8:
(4 comments)
Patchset:
PS4:
Any thoughts on the splitting up idea suggested above Richard?
I think the context of adding support to programmers is useful to understand the needs of the framework for reporting progress; splitting this into two changes doesn't seem very useful.
File 82802ab.c:
https://review.coreboot.org/c/flashrom/+/49643/comment/abc60d8c_769ca106 PS8, Line 137: set_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len); Updating progress on every byte seems pretty costly. Does it matter? (Are the usual writes to this device small?) Perhaps set_progress itself should include a check to avoid costly too-frequent messages.
It looks like other programmers also update progress on every byte, so it probably should be addressed in set_progress (and apply to library users as well) or flashrom_progress_cb (applying only to flashrom CLI).
File cli_output.c:
https://review.coreboot.org/c/flashrom/+/49643/comment/f8c2a97c_ce7114c7 PS8, Line 85: msg_ginfo("[%s] %u%% complete", flashrom_progress_stage_to_string (stage), pc); Would it make sense to terminate this with a \r to avoid scrolling terminals?
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/49643/comment/4128a208_2dbbc29b PS8, Line 105: typedef void(flashrom_progress_callback)(struct flashrom_flashctx *flashctx, enum flashrom_progress_stage stage, size_t current, size_t total, void *user_data); The progress will go 0-100 for every region being operated on, which might be surprising (but is probably still okay) when operating on multiple flash regions. Might be nice to also report the name of the current region, but I suspect that would be difficult to plumb through.