David Hendricks has posted comments on this change. ( https://review.coreboot.org/25706 )
Change subject: linux_mtd: Initial import ......................................................................
Patch Set 7: Verified+1
(2 comments)
I was able to successfully test on Bob after fixing the argument to fopen(). Yay!
Taking the follow-up patch for write-protection into consideration, I felt like adding fileno() in multiple places wasn't any better and having both dev_fd and dev_fp and using them wherever is appropriate. In either case we need to be careful about closing the stream and descriptor, so IMO it's easiest/cleanest to open them both in one place and close them both in another place.
Patch Set 6:
I'm sorry, I didn't think this through when suggesting to use fread()/ fwrite() for the MTD device file too: Writing should be followed by an fflush().
Done.
I still think it's the right thing to do, at least for educating any potential reader of our code :) well, the same is true for a version of the loop with read()/write() that checks for EINTR.
Done.
https://review.coreboot.org/#/c/25706/4/linux_mtd.c File linux_mtd.c:
https://review.coreboot.org/#/c/25706/4/linux_mtd.c@231 PS4, Line 231: perr(
I think I found it, fopen() doesn't know an "rw" mode. I think […]
D'oh! Yeah, that makes sense. I guess "rw" is legal, even if it's non-sensical, since I didn't see a compiler warning.
https://review.coreboot.org/#/c/25706/6/linux_mtd.c File linux_mtd.c:
https://review.coreboot.org/#/c/25706/6/linux_mtd.c@342 PS6, Line 342: %s
There is no "rw". I think what we want is "r+", i.e. […]
Yep, that was it.
-
David Hendricks (Code Review)