Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41440 )
Change subject: sconfig: Add support for firmware configuration ......................................................................
Patch Set 1:
(7 comments)
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/main.c File util/sconfig/main.c:
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/main.c@336 PS1, Line 336: strncasecmp
I don't think you want to make these case insensitive. […]
Ack
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/main.c@372 PS1, Line 372:
nit: maybe sanity-check bits (start <= end, end < 32)?
Ack
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/main.c@373 PS1, Line 373: if (!field) {
Shouldn't this be a fatal error if field already exists (not just silently ignore)?
This is what lets a variant add options to an existing field in overridetree.cb
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/main.c@392 PS1, Line 392: option = option->next;
nit: might also want to check that option name doesn't exist yet while doing this
meta question: Do we want to let a variant override an option? (rather than just add options to existing fields like above)
What I have here doesn't really do that either so I need to add checks either way.
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/main.c@394 PS1, Line 394: }
nit: should probably sanity check add->value < 1 << field->end_bit
Ack
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/sconfig.y File util/sconfig/sconfig.y:
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/sconfig.y@79 PS1, Line 79: NUMBER
nit: could consider making the second number optional for single-bit fields?
I was wondering how to do that but it looks like I can just add another entry here without the second number and it should work.
https://review.coreboot.org/c/coreboot/+/41440/1/util/sconfig/sconfig.y@85 PS1, Line 85: strtoul($<string>3,
nit: off-topic, but I'm not sure why we're using strto(u)l for all the numbers in here. […]
I actually started with $<number> here and ran into issues before realizing that everything else was converting a string. Let me dig up what the issue was.