Attention is currently required from: Edward O'Callaghan, Angel Pons, Nikolai Artemiev, Anastasia Klimchuk, Sergii Dmytruk. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/58484 )
Change subject: libflashrom,writeprotect: add flashrom_wp_{config,range,mode}_to_str() ......................................................................
Patch Set 25:
(2 comments)
File libflashrom.h:
https://review.coreboot.org/c/flashrom/+/58484/comment/238e3f85_cf90f5ac PS25, Line 138: arbitrary It definitely has a minimum. It's hard to get right without leaking information about the implementation. It also raises questions, what if a newer version of the library supports more config bits? we'd have to start versioning the API... at least we should pass the size of the destination string as another argument.
IMO, much better to allocate the string inside libflashrom.
File writeprotect.c:
https://review.coreboot.org/c/flashrom/+/58484/comment/e6faa3c1_e075bf88 PS25, Line 466: } This leaks internal information, it looks much like debugging code TBH. IIRC, I've seen some discussion about debug output later in the queue. IMO, it seems right to log things in internal code. A CLI is always limited in what it can present, and that's ok as it should present things to a humble user. Information that is only useful for developers would land in the log, is that a bad idea?
Not sure about the other two functions below. On one hand they seem convenient, on the other they don't do anything the CLI code coudn't do.