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 22:
(16 comments)
Ok, found some more flaws. Most are minor and easy to fix. But the bsearch has some major issues that I couldn't ignore, sorry ;)
https://review.coreboot.org/#/c/23203/22//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/23203/22//COMMIT_MSG@14 PS22, Line 14: to This `to` seems spurious.
https://review.coreboot.org/#/c/23203/22//COMMIT_MSG@22 PS22, Line 22: mostly copied from Rather `based on` now?
https://review.coreboot.org/#/c/23203/22/cli_classic.c File cli_classic.c:
https://review.coreboot.org/#/c/23203/22/cli_classic.c@642 PS22, Line 642: && !fmapfile I stared at this for a moment, now I'm sure the `!fmapfile` is redundant.
https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl File flashrom.8.tmpl:
https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl@52 PS22, Line 52: [\fB-i\fR <image>]] [\fB-n\fR] [\fB-N\fR] [\fB-f\fR]] This line is unnecessarily long now. Please break before the `-n`. Would still look good and fit an 80 chars view if inden- tation is reduced by 3 spaces (the current 15 spaces indent is odd anyway).
https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl@202 PS22, Line 202: partioning par*ti*tioning
https://review.coreboot.org/#/c/23203/22/flashrom.8.tmpl@217 PS22, Line 217: partioning partitioning
https://review.coreboot.org/#/c/23203/22/fmap.h File fmap.h:
https://review.coreboot.org/#/c/23203/22/fmap.h@3 PS22, Line 3: -present I know it has been discussed already. But this is incredibly annoying. While everybody seems to know that this `-present` is not legally binding and nonsense, coming from a huge com- pany like Facebook (that should know better) feels quite of- fensive. Do they really want to be the Disney of open source?
I won't block it. Just saying... you can't complain about this to your superiors often enough. Just every day "Every minute a(nother) copy of this is online, it's lowering Face- book's reputation".
https://review.coreboot.org/#/c/23203/22/fmap.h@45 PS22, Line 45: #define FMAP_VER_MINOR 1 /* this header's FMAP minor version */ Didn't check earlier versions, but now this is unused? shouldn't we check the minor version too?
https://review.coreboot.org/#/c/23203/22/fmap.h@67 PS22, Line 67: int is_valid_fmap(const struct fmap *fmap); nit, could be static now
https://review.coreboot.org/#/c/23203/22/fmap.c File fmap.c:
https://review.coreboot.org/#/c/23203/22/fmap.c@1 PS22, Line 1: nit, missing initial line break
https://review.coreboot.org/#/c/23203/22/fmap.c@203 PS22, Line 203: return 1; leaking `fmap`
https://review.coreboot.org/#/c/23203/22/fmap.c@260 PS22, Line 260: fmap_len = fmap_size(fmap); : struct fmap *tmp = fmap; : fmap = realloc(fmap, fmap_len); : if (!fmap) { : msg_gerr("Failed to realloc.\n"); : free(tmp); : goto _free_ret; : } : : if (flashctx->chip->read(flashctx, (uint8_t *)fmap + sizeof(*fmap), : offset + sizeof(*fmap), fmap_len - sizeof(*fmap))) { : msg_cerr("Cannot read %zu bytes at offset %06zx\n", : fmap_len - sizeof(*fmap), offset + sizeof(*fmap)); : /* Treat read failure to be fatal since this : * should be a valid, usable fmap. */ : goto _free_ret; : } : : *fmap_out = malloc(fmap_len); : if (*fmap_out == NULL) { : msg_gerr("Out of memory.\n"); : goto _free_ret; : } : : memcpy(*fmap_out, fmap, fmap_len); This path is flawed a little. At least it's missing an unconditional `break` after the memcpy(). But this also shows that everything after setting `fmap_found = 1` could happen outside the loops. So optio- nally we could refactor as follows:
for (stride = ... ) { ... for (offset = ... ) { ... if (is_valid_fmap(fmap)) { msg_gdbg(... fmap_found = 1; break; }
msg_gerr(... ret = 2; } if (fmap_found) break; }
if (!fmap_found) goto _free_ret;
/* code from above follows, moved out of the loops */
Um, with that said, there seems to be no valid path to hit the realloc() a second time. Every failure after the realloc() is fatal, and if we found and read a valid fmap, we should stop sear- ching, right?
https://review.coreboot.org/#/c/23203/22/fmap.c@292 PS22, Line 292: if (fmap_found) This would still be set after a read failure, for instance. So it's probably safest to place the `ret = 0` right after the memcpy() to `*fmap_out`.
https://review.coreboot.org/#/c/23203/22/fmap.c@321 PS22, Line 321: a fmap How do you pronounce it? Is it `*a* fmap` or `*an* eff-map`?
https://review.coreboot.org/#/c/23203/22/libflashrom.c File libflashrom.c:
https://review.coreboot.org/#/c/23203/22/libflashrom.c@433 PS22, Line 433: { Ugly, but as we were too lazy for proper deserialization again, we should guard here with __FLASHROM_LITTLE_ENDIAN__ like it's done in flasrom_layout_read_from_ifd().
https://review.coreboot.org/#/c/23203/22/libflashrom.c@468 PS22, Line 468: { Same guard here.