Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42859 )
Change subject: amdfwtool: Take a config file instead of command line parameters ......................................................................
Patch Set 55: Code-Review+1
(2 comments)
looks good to me and I'd say that the other small things I found today and didn't comment on yet should be addressed in a follow-up patch, so that this one will finally land in upstream coreboot soon. I'll still need to verify that the resulting amdfw binary doesn't change for the different platforms; when I've done that and the binaries are identical I'll +2 the patch.
https://review.coreboot.org/c/coreboot/+/42859/54/util/amdfwtool/data_parse.... File util/amdfwtool/data_parse.c:
https://review.coreboot.org/c/coreboot/+/42859/54/util/amdfwtool/data_parse.... PS54, Line 265: (void *)
this cast is probably unnecessary, since both sides are amd_bios_entry type
Done
https://review.coreboot.org/c/coreboot/+/42859/54/util/amdfwtool/data_parse.... PS54, Line 317: /* get line */ : /* blank comment */ : if (!regexec(&blank_or_comment_expr, oneline, 0, NULL, 0)) { : /* skip comment and blank */ : continue; : } : : if (regexec(&entries_line_expr, oneline, 3, match, 0) == 0) { : oneline[match[1].rm_eo] = '\0'; : oneline[match[2].rm_eo] = '\0'; : } else { : /* no match */ : continue; : }
this code is identical to lines 340-353 below, so it's probably a good idea to factor that out into […]
Done