Attention is currently required from: Raul Rangel, Martin L Roth, Jon Murphy, Arthur Heymans, Angel Pons, Karthik Ramasubramanian, Felix Held.
Tim Van Patten has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74736 )
Change subject: util/sconfig: Include optional maiboard chip header ......................................................................
Patch Set 3:
(3 comments)
File util/sconfig/main.c:
https://review.coreboot.org/c/coreboot/+/74736/comment/41462666_09aec6ca PS3, Line 1402: &chip_h[strlen("src/")] I don't understand this indexing into `chip_h`.
Is the assumption that `mainboard_path` begins with (if so, why can we assume that?)? Or was this meant to find the substring "src/" first, and then starting `fprintf`'ing from there?
https://review.coreboot.org/c/coreboot/+/74736/comment/d6281a98_b6483f55 PS3, Line 1405: free(mainboard_path); This is potentially a double-free, since `mainboard_path` does not always allocate its own memory (ref1).
Instead, either an additional flag needs to indicate when the `free()` should be done, or `mainboard_path` needs to always have memory allocated for it, rather than reusing memory in some cases.
https://review.coreboot.org/c/coreboot/+/74736/comment/108e4f24_285f99e9 PS3, Line 2030: mainboard_path = base_devtree_dir; ref1
-
Tim Van Patten (Code Review)