Attention is currently required from: Jakub Czapiga, Werner Zeh. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/59312 )
Change subject: cbfs: Add helper functions to look up size and type of a file ......................................................................
Patch Set 1:
(3 comments)
File src/include/cbfs.h:
https://review.coreboot.org/c/coreboot/+/59312/comment/1669e976_85b86f6b PS1, Line 134: static inline bool cbfs_file_exists(const char *name); : static inline bool cbfs_ro_file_exists(const char *name);
I wonder, if we actually need these functions. […]
Here was the original code that highlighted the need for a helper like this: https://review.coreboot.org/c/coreboot/+/55367/52/src/soc/intel/elkhartlake/...
Sometimes you may need to check if a file is there without immediately loading it if it is (and like I tried to point out in the comments, we should avoid those situations as much as possible, but sometimes they can't be avoided).
https://review.coreboot.org/c/coreboot/+/59312/comment/65465664_dbc082b4 PS1, Line 308: static inline size_t cbfs_get_size(const char *name)
Should not these functions be inaccessible for code running in CAR mode and similar? This might make […]
It's the same stack requirement as a normal CBFS file load? Early stages do those as well. The general coreboot minimum stack size is 2K (and I believe in practice all platforms are much larger... for x86 platforms the CAR stack tends to be 8K or more). cbfs_mdata is intentionally limited to 256 bytes since that should hopefully never be an issue.
File src/lib/cbfs.c:
https://review.coreboot.org/c/coreboot/+/59312/comment/4c9c3d86_07a45654 PS1, Line 528: if ((err = _cbfs_boot_lookup(prog_name(pstage), false, &mdata, &rdev)))
Would not it be cleaner to set err value in a separate line above? Personally I find assign-and-chec […]
I'd say it's a personal style choice... it's not forbidden by the coreboot style guide. I find them useful and increasing readability, because the same code can be written in fewer lines (so you can have more meaningful parts of a function on one screen). Also, I think they are much more important for while-loops, and when you allow them in while-loops there's no reason not to use them in if-statements where appropriate as well. (For example, writing a loop like https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src... where you want to call a function, use the result to decide whether the loop ends, and then continue working on the result if it doesn't, looks a lot more ugly and harder to follow without assign-and-check.)
Also, in my experience, most concerns about this just come from some 90's era conventional wisdom that it was dangerous (accidentally writing = instead of ==), but these days GCC forces you to put extra parenthesis around the assignment anyway so doing it unintentionally is impossible.