Werner Zeh has posted comments on this change. ( https://review.coreboot.org/23203 )
Change subject: Add support to get layout from fmap (e.g. coreboot rom) ......................................................................
Patch Set 20:
(5 comments)
That's gonna be a nice feature. Thanks for doing it.
https://review.coreboot.org/#/c/23203/20/cli_classic.c File cli_classic.c:
https://review.coreboot.org/#/c/23203/20/cli_classic.c@102 PS20, Line 102: 1 This should be -1!
https://review.coreboot.org/#/c/23203/20/flashrom.8.tmpl File flashrom.8.tmpl:
https://review.coreboot.org/#/c/23203/20/flashrom.8.tmpl@203 PS20, Line 203: Maybe add "flashrom" here? or at least an "it" would make the sentence correct.
https://review.coreboot.org/#/c/23203/20/fmap.c File fmap.c:
https://review.coreboot.org/#/c/23203/20/fmap.c@59 PS20, Line 59: flash What you actually check here is not the overall flashsize itself but the size of this FMAP instance in the flash. If you want you can be more precise in the comment but it's up to you.
https://review.coreboot.org/#/c/23203/20/fmap.c@182 PS20, Line 182: if (!flashctx || !flashctx->chip) : return 1; Would you like to add this check to fmap_lsearch_rom() as well?
https://review.coreboot.org/#/c/23203/20/fmap.c@256 PS20, Line 256: struct fmap *tmp = fmap; : fmap = realloc(fmap, fmap_len); : if (!fmap) { : msg_gerr("Failed to realloc.\n"); : free(tmp); : goto _free_ret; : } realloc is allowed to move the re-allocated memory buffer to a completely new address[1]. If that happens you will end up not freeing allocated memory. You can check if after realloc() fmap and tmp do not match and free tmp in this case. Or do I miss something here? [1] https://stackoverflow.com/questions/35413891/does-realloc-change-pointer-add...
-
Werner Zeh (Code Review)