Attention is currently required from: Jakub Czapiga, Werner Zeh.
3 comments:
File src/include/cbfs.h:
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/romstage/fsp_params.c#127
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).
Patch Set #1, 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:
Patch Set #1, 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/commonlib/bsd/cbfs_private.c#45 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.
To view, visit change 59312. To unsubscribe, or for help writing mail filters, visit settings.