Attention is currently required from: Jakub Czapiga, Martin L Roth, Maximilian Brune.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81081?usp=email )
Change subject: lib/device_tree: Add some FDT helper functions ......................................................................
Patch Set 14:
(3 comments)
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/a2252249_5f8b2a76 : PS12, Line 200: if (!strncmp(*path, node_name, path_sub_len)) {
Isn't that the same as creating variable length arrays? I thought we don't do that anymore in corebo […]
Don't we? It's used in acpigen and vboot/ec_sync, for example.
I think VLAs might be discouraged because they make it hard to see that a variable stack allocation takes place, but alloca makes it very explicit. I don't think the function is fundamentally bad in any way, you just need to be sure you know how you're using it (and in this case I think it's safe to assume that FDT paths will be reasonably bounded).
https://review.coreboot.org/c/coreboot/+/81081/comment/9fe14573_1fe14861 : PS12, Line 235: } while (be32dec(blob + offset) != FDT_TOKEN_END_NODE);
Well `fdt_next_node_name` expects a `FDT_TOKEN_BEGIN_NODE`. […]
Yeah, I think keeping the code simple is more important here. We're not writing a device tree verifier. As long as broken input doesn't cause it to crash or use bogus data, I think that's good enough.
https://review.coreboot.org/c/coreboot/+/81081/comment/3b24cef0_ee1d7eb6 : PS12, Line 337: results[(*count_results)++] = offset;
Maybe do both? […]
Ack, I think that's good enough.
-
Julius Werner (Code Review)