Attention is currently required from: Angel Pons, Anastasia Klimchuk, Nikolai Artemiev. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/62554 )
Change subject: layout: Change signature for prepare_layout_for_extraction ......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
Thanks for reply! :)
I'm not sure if I mentioned this somewhere already, the topic seems familiar. I kind of see this extract feature as CLI code.
Sorry if I missed that! I don't remember, but maybe because extract feature was upstreamed earlier when I just started and I wasn't fully aware of all everything that going on.
Don't worry, if we discussed this somewhere else already, I lost track of it anyway.
My reasoning for this patch chain was to align with larger movement to make cli a libflashrom user, which means libflashrom is used from everywhere and needs to be tested.
Yes, I got that :) we need to adapt libflashrom API for this. Making existing functions public isn't always the best way, though.
So far, we have kept libflashrom free from file i/o and some target environments just don't support it
Maybe I am missing something, but I don't see `prepare_layout_for_extraction` doing file i/o at the moment? It is populating file names, which is prep step for i/o, but the i/o itself is done next in read operation.
That's right. But for prepare_layout_for_extraction() to be useful in the libflashrom API, we'd need another function there that probably does what write_buf_to_include_args() currently does. And that's doing file i/o. Also, the description in your next patch says "Fill in file name". So I'd say it's part of a file i/o concept.
It's why I generally prefer to discuss the coarse direction of a topic on the mailing list first. Here on Gerrit, people are often focused to bring patches in step-by-step, but sometimes the bigger picture is only visible at the end of a patch train. It can be frustrating for both authors and reviewers to see that the effort on prior reviews doesn't pan out. Or worse, they might want to merge something that they consider wrong just because they put all the effort into it.
Maybe `prepare_layout_for_extraction` can be renamed into something more specific?
I would much prefer to discuss first if the whole topic belongs into libflashrom or the CLI.
I agree with keeping libflashrom free from file i/o.