Hello Furquan Shaikh, Patrick Georgi, Angel Pons, Arthur Heymans, Kyösti Mälkki, Aaron Durbin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/41743
to review the following change.
Change subject: util/sconfig: Refactor and fix add_register() ......................................................................
util/sconfig: Refactor and fix add_register()
add_register() contained a duplicate check but only compared the new key to the first (smallest in order) list member. Fix that and factor the list handling out so it can be used by other functions.
Change-Id: I5a8346f36fa024351e1282c9681868ecf451b283 Signed-off-by: Nico Huber nico.h@gmx.de --- M util/sconfig/main.c 1 file changed, 19 insertions(+), 18 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/43/41743/1
diff --git a/util/sconfig/main.c b/util/sconfig/main.c index 6676bae..3e3bd17 100644 --- a/util/sconfig/main.c +++ b/util/sconfig/main.c @@ -528,33 +528,34 @@ new_resource(bus->dev, type, index, base); }
-void add_register(struct chip_instance *chip_instance, char *name, char *val) +static void add_reg(struct reg **const head, char *const name, char *const val) { - struct reg *r = S_ALLOC(sizeof(struct reg)); + struct reg *const r = S_ALLOC(sizeof(struct reg)); + struct reg *prev = NULL; + struct reg *cur;
r->key = name; r->value = val; - if (chip_instance->reg) { - struct reg *head = chip_instance->reg; - // sorting to be equal to sconfig's behaviour - int sort = strcmp(r->key, head->key); + + for (cur = *head; cur != NULL; prev = cur, cur = cur->next) { + const int sort = strcmp(r->key, cur->key); if (sort == 0) { printf("ERROR: duplicate 'register' key.\n"); exit(1); } - if (sort < 0) { - r->next = head; - chip_instance->reg = r; - } else { - while ((head->next) - && (strcmp(head->next->key, r->key) < 0)) - head = head->next; - r->next = head->next; - head->next = r; - } - } else { - chip_instance->reg = r; + if (sort < 0) + break; } + r->next = cur; + if (prev) + prev->next = r; + else + *head = r; +} + +void add_register(struct chip_instance *chip_instance, char *name, char *val) +{ + add_reg(&chip_instance->reg, name, val); }
void add_slot_desc(struct bus *bus, char *type, char *length, char *designation,
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41743 )
Change subject: util/sconfig: Refactor and fix add_register() ......................................................................
Patch Set 3: Code-Review+1
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41743 )
Change subject: util/sconfig: Refactor and fix add_register() ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/41743 )
Change subject: util/sconfig: Refactor and fix add_register() ......................................................................
util/sconfig: Refactor and fix add_register()
add_register() contained a duplicate check but only compared the new key to the first (smallest in order) list member. Fix that and factor the list handling out so it can be used by other functions.
Change-Id: I5a8346f36fa024351e1282c9681868ecf451b283 Signed-off-by: Nico Huber nico.h@gmx.de Reviewed-on: https://review.coreboot.org/c/coreboot/+/41743 Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M util/sconfig/main.c 1 file changed, 19 insertions(+), 18 deletions(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/util/sconfig/main.c b/util/sconfig/main.c index 6676bae..3e3bd17 100644 --- a/util/sconfig/main.c +++ b/util/sconfig/main.c @@ -528,33 +528,34 @@ new_resource(bus->dev, type, index, base); }
-void add_register(struct chip_instance *chip_instance, char *name, char *val) +static void add_reg(struct reg **const head, char *const name, char *const val) { - struct reg *r = S_ALLOC(sizeof(struct reg)); + struct reg *const r = S_ALLOC(sizeof(struct reg)); + struct reg *prev = NULL; + struct reg *cur;
r->key = name; r->value = val; - if (chip_instance->reg) { - struct reg *head = chip_instance->reg; - // sorting to be equal to sconfig's behaviour - int sort = strcmp(r->key, head->key); + + for (cur = *head; cur != NULL; prev = cur, cur = cur->next) { + const int sort = strcmp(r->key, cur->key); if (sort == 0) { printf("ERROR: duplicate 'register' key.\n"); exit(1); } - if (sort < 0) { - r->next = head; - chip_instance->reg = r; - } else { - while ((head->next) - && (strcmp(head->next->key, r->key) < 0)) - head = head->next; - r->next = head->next; - head->next = r; - } - } else { - chip_instance->reg = r; + if (sort < 0) + break; } + r->next = cur; + if (prev) + prev->next = r; + else + *head = r; +} + +void add_register(struct chip_instance *chip_instance, char *name, char *val) +{ + add_reg(&chip_instance->reg, name, val); }
void add_slot_desc(struct bus *bus, char *type, char *length, char *designation,
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41743 )
Change subject: util/sconfig: Refactor and fix add_register() ......................................................................
Patch Set 4:
Automatic boot test returned (PASS/FAIL/TOTAL): 4/0/4 Emulation targets: "QEMU x86 q35/ich9" using payload TianoCore : SUCCESS : https://lava.9esec.io/r/4532 "QEMU x86 q35/ich9" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4531 "QEMU x86 i440fx/piix4" using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/4530 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/4529
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Angel Pons (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)