Attention is currently required from: Julius Werner, Patrick Rudolph.
Angel Pons has posted comments on this change by Patrick Rudolph. ( https://review.coreboot.org/c/coreboot/+/78506?usp=email )
Change subject: tests: Add unit tests for CFR ......................................................................
Patch Set 5:
(5 comments)
File src/commonlib/bsd/cfr_parser.c:
https://review.coreboot.org/c/coreboot/+/78506/comment/fe26dee7_afe3a86f?usp... : PS5, Line 21: case CFR_TAG_OPTION_ENUM: : return sizeof(struct lb_cfr_numeric_option); : case CFR_TAG_OPTION_NUMBER: : return sizeof(struct lb_cfr_numeric_option); : case CFR_TAG_OPTION_BOOL: : return sizeof(struct lb_cfr_numeric_option);
Is there a reason for these case statements to be separate? They can look like that: […]
I guess it's for visual consistency. For this kind of case-return approach I usually go for one-line:
``` case CFR_TAG_OPTION_ENUM: return sizeof(struct lb_cfr_numeric_option); case CFR_TAG_OPTION_NUMBER: return sizeof(struct lb_cfr_numeric_option); case CFR_TAG_OPTION_BOOL: return sizeof(struct lb_cfr_numeric_option); ``` (had to use spaces so that it'd show up nicely on Gerrit)
https://review.coreboot.org/c/coreboot/+/78506/comment/228aeb14_2eaccd8d?usp... : PS5, Line 31: default: Would it make sense to complain about unknown entries?
https://review.coreboot.org/c/coreboot/+/78506/comment/a3d0a6c5_dd6e2c17?usp... : PS5, Line 109: ret = cfr_iterate(&child, callback, priv);
How deep can recursion be here? If someone passes a long enough parent->child->child->... […]
It generally shouldn't be a problem, since the structure must not have loops and won't get too deeply nested.
Still, I think it would be nice to add a recursion guard. Maybe it can be a simple `max_depth` parameter?
``` static int cfr_iterate(const struct region_device *root, int (*callback)(const struct region_device *root, const struct region_device *child, void *priv), void *priv, int max_depth) { struct region_device child; struct lb_cfr_header hdr; int ret, size, offset;
if (max_depth <= 0) return CB_ERR; ```
```suggestion ret = cfr_iterate(&child, callback, priv, max_depth - 1); ```
https://review.coreboot.org/c/coreboot/+/78506/comment/ffb6cd3c_49f15f70?usp... : PS5, Line 144: // FIXME checksum
Sorry, but I cannot allow the input parsing code to be submitted w/o checksum checking if there is o […]
+1
File tests/drivers/cfr.c:
https://review.coreboot.org/c/coreboot/+/78506/comment/1bd6bd45_e877ca2b?usp... : PS5, Line 339: void *head = malloc(0x100000); : assert(head); nit: Hmmm, put this inside a helper function?