Nico Huber has posted comments on this change. ( https://review.coreboot.org/22344 )
Change subject: linux_spi: Dynamically detect max buffer size ......................................................................
Patch Set 6: Verified-1 Code-Review+2
(3 comments)
https://review.coreboot.org/#/c/22344/5/linux_spi.c File linux_spi.c:
https://review.coreboot.org/#/c/22344/5/linux_spi.c@143 PS5, Line 143: if (feof(fp))
In any case, I went ahead and added an feof() to check for this condition. I was thinking of using stat(), but that would add a few more lines and seemed redundant with fopen() error handling in this case. LMK if you have a preference.
feof() is fine. stat() seems overkill and I'm not sure how well it is implemented for sysfs ;)
BTW - It may be worth moving all this logic to a file handling library some day, especially as we improve support for Linux interfaces that involve searching thru sysfs and proc and potentially opening/closing a lot of files.
Sounds good, especially if we hit the same pattern (e.g. reading an unsigned) from a file again.
https://review.coreboot.org/#/c/22344/5/linux_spi.c@154 PS5, Line 154: msg_pwarn("Buffer size %ld from %s seems wrong.\n", tmp, BUF_SIZE_FROM_SYSFS);
Yeah, that's certainly an assumption that I suppose language lawyers may ta
Same for the part of the string we didn't read, e.g. what if we don't hit EOF after 10 chars oO
https://review.coreboot.org/#/c/22344/5/linux_spi.c@163 PS5, Line 163:
Should I follow-up with a patch to use sysconf(_SC_PAGESIZE) instead, as su
Not a big issue, IMO. But if you do, I'm happy to review.