Nico Huber 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:
(15 comments)
looks much better, thank you. I like the movement to fmap.c
https://review.coreboot.org/#/c/23203/20/cli_classic.c File cli_classic.c:
https://review.coreboot.org/#/c/23203/20/cli_classic.c@107 PS20, Line 107: "r" "rb" for windows? not sure
https://review.coreboot.org/#/c/23203/20/fmap.h File fmap.h:
https://review.coreboot.org/#/c/23203/20/fmap.h@69 PS20, Line 69: const `const` on the parameter itself is meaningless in the contract
https://review.coreboot.org/#/c/23203/19/fmap.h File fmap.h:
https://review.coreboot.org/#/c/23203/19/fmap.h@33 PS19, Line 33: * GNU General Public License ("GPL") version 2 as published by the Free
It's dual-licensed, as the last paragraph states. […]
That's why I though, why mention GPLv2 explicitly? Also wondering if we miss something ;)
Ok checked, it's the standard 3-clause BSD text. Generally GPL compatible... weird.
https://review.coreboot.org/#/c/23203/20/fmap.c File fmap.c:
https://review.coreboot.org/#/c/23203/20/fmap.c@44 PS20, Line 44: int fmap_size(const struct fmap *fmap) could be static now
https://review.coreboot.org/#/c/23203/20/fmap.c@47 PS20, Line 47: return -1; All callers seem to know that it's non-NULL but ignore possible negative return values. Maybe just remove the check and return a size_t?
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 […]
Are there nested FMAPs? I've only seen the default ones in coreboot and they seem to cover the complete flash.
https://review.coreboot.org/#/c/23203/20/fmap.c@151 PS20, Line 151: off_t fmap_offset = fmap_lsearch(buf + rom_offset, len); : if (fmap_offset < 0) { : msg_gdbg("Failed to find fmap using linear search.\n"); : goto _free_ret; : } : : int fmap_len = fmap_size((struct fmap *)&buf[fmap_offset]); : *fmap_out = malloc(fmap_len); : if (*fmap_out == NULL) { : msg_gerr("Out of memory.\n"); : goto _free_ret; : } : : memcpy(*fmap_out, buf + fmap_offset, fmap_len); Beside the error message, this is probably the same as calling fmap_read_from_buffer().
https://review.coreboot.org/#/c/23203/20/fmap.c@179 PS20, Line 179: unsigned int chip_size = flashctx->chip->total_size * 1024; : int sig_len = strlen(FMAP_SIGNATURE); make both const?
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?
Or just move it up into fmap_read_from_rom()?
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]. […]
realloc() frees the original memory if moved. So we only have to take care of the original pointer (tmp) in the error case, what we do.
https://review.coreboot.org/#/c/23203/20/libflashrom.h File libflashrom.h:
https://review.coreboot.org/#/c/23203/20/libflashrom.h@67 PS20, Line 67: const the later const is not meaningful here
https://review.coreboot.org/#/c/23203/20/libflashrom.c File libflashrom.c:
https://review.coreboot.org/#/c/23203/20/libflashrom.c@396 PS20, Line 396: if (l->num_entries == MAX_ROMLAYOUT) { we could also check in advance `if (fmap->nareas > MAX_ROMLAYOUT)` but this works too
https://review.coreboot.org/#/c/23203/20/libflashrom.c@405 PS20, Line 405: sizeof(l->entries[i].name) FMAP_STRLEN is shorter, use that or MIN() of both? Also an explicit 0-termination would be good (I know there should be one in the FMAP, but we didn't check the entries, did we?)
https://review.coreboot.org/#/c/23203/20/libflashrom.c@477 PS20, Line 477: ret = 1; redundant
https://review.coreboot.org/#/c/23203/20/libflashrom.c@484 PS20, Line 484: ret = 1; redundant
-
Nico Huber (Code Review)