Attention is currently required from: Hung-Te Lin, Nico Huber, Edward O'Callaghan, Angel Pons, Patrick Rudolph. Douglas Anderson has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/50206 )
Change subject: linux_mtd: Switch fopen() to open() for MTD devices ......................................................................
Patch Set 2:
(3 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/50206/comment/c9847dcc_d0d14070 PS2, Line 26: overhead / translation that we just don't need.
Proof?
I mean, the OS exposes the open/close/read/write calls and so the stdio functions _must_ be implemented atop them. It can't be done any other way. Thus the sdio functions _must_ be an extra layer? What am I missing?
...or are you saying to prove the "we just don't need". Aside from your comment about short reads, I would argue that the new implementation works as well as the old and isn't any more complicated.
In any case, probably not relevant since I'm proposing abandoning this change in response to your point about short reads.
Patchset:
PS2: I think Nico's point about read() being able to return a shorter read is a good one and I'm thinking that maybe we should just abandon this change? I mostly implemented it in response to Hung-Te's feedback in https://review.coreboot.org/c/flashrom/+/50155 and I have no objection to leaving things using stdio.
Hung-Te: what do you think?
File linux_mtd.c:
https://review.coreboot.org/c/flashrom/+/50206/comment/ce81c346_9cf3b813 PS2, Line 206: if (read(dev_fileno, buf + i, step) != step) {
AFAIR, read() can be interrupted and return a short count even if […]
Huh, this is a good point and I didn't think about it. I think you're right and this could be a real problem. From my understanding the fread() abstracts this out from us? That would actually be a good reason to use fread().
-
Douglas Anderson (Code Review)