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 19:
(42 comments)
https://review.coreboot.org/#/c/23203/19//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/23203/19//COMMIT_MSG@14 PS19, Line 14: to spurious `to`?
https://review.coreboot.org/#/c/23203/19//COMMIT_MSG@18 PS19, Line 18: An example actually `Examples`
https://review.coreboot.org/#/c/23203/19//COMMIT_MSG@22 PS19, Line 22: The fmap functions are mostly copied from cbfstool. Which probably means we can't license them GPL v2+.
https://review.coreboot.org/#/c/23203/19//COMMIT_MSG@24 PS19, Line 24: while still : allowing --layout this is not what I read in the code?
https://review.coreboot.org/#/c/23203/19/cli_classic.c File cli_classic.c:
https://review.coreboot.org/#/c/23203/19/cli_classic.c@58 PS19, Line 58: binary containing s/fmap contained in ?
also, please maintain the 80 chars limit
https://review.coreboot.org/#/c/23203/19/cli_classic.c@619 PS19, Line 619: fill_flash->chip->total_size * 1024 NB. we should introduce a macro/function for this sooner than later
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: * Software Foundation. Is the above license not GPL compatible?
https://review.coreboot.org/#/c/23203/19/fmap.h@44 PS19, Line 44: #define FMAP_VER_MINOR 1 /* this header's FMAP minor version */ If we only support v1.1 but there were lower versions, please mention that in the commit message.
https://review.coreboot.org/#/c/23203/19/fmap.h@55 PS19, Line 55: 0x5F5F464D41505F5F writing it in big endian seems weird
https://review.coreboot.org/#/c/23203/19/fmap.c File fmap.c:
PS19: The mixed usage of `size_t`, `unsigned int` and `unsigned long int` for the same length is a little annoying. Please just use `size_t`.
https://review.coreboot.org/#/c/23203/19/fmap.c@40 PS19, Line 40: #include "flash.h" mmh?
Is this file altered? code added?
https://review.coreboot.org/#/c/23203/19/fmap.c@51 PS19, Line 51: sizeof(*fmap) + fmap->nareas * sizeof(struct fmap_area) this is `fmap_size(fmap)`?
in any case, no need to break the line
https://review.coreboot.org/#/c/23203/19/fmap.c@57 PS19, Line 57: while (i < FMAP_STRLEN) { seems weird to not use a `for` loop
https://review.coreboot.org/#/c/23203/19/fmap.c@62 PS19, Line 62: if (i == FMAP_STRLEN - 1) { seems weird to check this inside the loop
https://review.coreboot.org/#/c/23203/19/fmap.c@88 PS19, Line 88: unsigned long int `size_t`?
https://review.coreboot.org/#/c/23203/19/fmap.c@89 PS19, Line 89: int `bool`?
https://review.coreboot.org/#/c/23203/19/fmap.c@91 PS19, Line 91: < <= ?
https://review.coreboot.org/#/c/23203/19/fmap.c@91 PS19, Line 91: strlen(FMAP_SIGNATURE) `sizeof(struct fmap)`
https://review.coreboot.org/#/c/23203/19/fmap.c@92 PS19, Line 92: const casting to `const` is unnecessary
https://review.coreboot.org/#/c/23203/19/fmap.c@120 PS19, Line 120: if (fmap_found) : break; should be below the inner loop for enhanced readability
https://review.coreboot.org/#/c/23203/19/fmap.c@126 PS19, Line 126: (offset != 0) so `&image[0]` is always checked? that doesn't seem to make much sense
https://review.coreboot.org/#/c/23203/19/fmap.c@129 PS19, Line 129: (const struct fmap *)&image[offset])) { no need to break the line
https://review.coreboot.org/#/c/23203/19/fmap.c@158 PS19, Line 158: = -1; initialization seems never used
https://review.coreboot.org/#/c/23203/19/fmap.c@166 PS19, Line 166: ret = fmap_lsearch(image, image_len); I don't want to overoptimize things, but I have the feeling that we would have actually less code if we'd always use bsearch and just skip everything above `len` in the inner loop (what we already do) so just start with `stride` aligned up to the next power of 2?
https://review.coreboot.org/#/c/23203/19/libflashrom.h File libflashrom.h:
https://review.coreboot.org/#/c/23203/19/libflashrom.h@64 PS19, Line 64: const qualification has no effect here and the name shouldn't be needed (should be clear from the type what the parameter is about)
https://review.coreboot.org/#/c/23203/19/libflashrom.h@67 PS19, Line 67: struct flashrom_flashctx *const flashctx, const char *filename); The library was intentionally designed to be free from file i/o. I would prefer to keep that. Can we move the file i/o into the CLI code and just pass an already filled buffer + size here?
https://review.coreboot.org/#/c/23203/19/libflashrom.c File libflashrom.c:
https://review.coreboot.org/#/c/23203/19/libflashrom.c@419 PS19, Line 419: offset < 0 no, it's not (see type)
https://review.coreboot.org/#/c/23203/19/libflashrom.c@426 PS19, Line 426: sizeof(struct fmap) + sizeof(struct fmap_area) * fmap->nareas there's a function for that
https://review.coreboot.org/#/c/23203/19/libflashrom.c@428 PS19, Line 428: !fmap_out `!*fmap_out`
https://review.coreboot.org/#/c/23203/19/libflashrom.c@442 PS19, Line 442: static int read_fmap_from_rom(struct fmap **fmap_out, struct flashctx *const flashctx, off_t rom_offset, size_t len) please break this line
https://review.coreboot.org/#/c/23203/19/libflashrom.c@466 PS19, Line 466: < <= ?
https://review.coreboot.org/#/c/23203/19/libflashrom.c@466 PS19, Line 466: offset < rom_offset + len - sizeof(struct fmap)
should also do a sanity check to make sure that rom_offset + len <= chip_size.
could be asserted at the beginning of the function, right?
https://review.coreboot.org/#/c/23203/19/libflashrom.c@468 PS19, Line 468: (offset != 0)
means you check offset == 0 quite often if rom_offset is 0, could this not be checked once?
both conditions don't make much sense, imho. it should be `(offset - rom_offset)` in both cases, I guess
The code might become more clear if we let `offset` start at 0 and only add `rom_offset` when reading from flash.
https://review.coreboot.org/#/c/23203/19/libflashrom.c@479 PS19, Line 479: if (rom_offset + len - offset < sizeof(struct fmap)) { seems impossible by loop condition
https://review.coreboot.org/#/c/23203/19/libflashrom.c@485 PS19, Line 485: fmap = malloc(sizeof(struct fmap)); missing check for NULL
https://review.coreboot.org/#/c/23203/19/libflashrom.c@486 PS19, Line 486: if (flashctx->chip->read(flashctx, (uint8_t *)fmap, offset, sizeof(*fmap))) { we could skip reading the signature again
https://review.coreboot.org/#/c/23203/19/libflashrom.c@491 PS19, Line 491: sizeof(struct fmap) + sizeof(struct fmap_area) * fmap->nareas I'm sure I've seen a function for this.
https://review.coreboot.org/#/c/23203/19/libflashrom.c@492 PS19, Line 492: fmap = realloc(fmap, fmap_len); missing check for NULL
https://review.coreboot.org/#/c/23203/19/libflashrom.c@494 PS19, Line 494: if (flashctx->chip->read(flashctx, (uint8_t *)fmap, offset, fmap_len)) { we could skip reading the header again
https://review.coreboot.org/#/c/23203/19/libflashrom.c@499 PS19, Line 499: if (is_valid_fmap(fmap)) { IIRC, this can be checked on the header alone
https://review.coreboot.org/#/c/23203/19/libflashrom.c@515 PS19, Line 515: return ret; potentially leaking `fmap` if we didn't succeed
https://review.coreboot.org/#/c/23203/19/libflashrom.c@534 PS19, Line 534: struct flashrom_layout *l = get_global_layout(); We should either allocate it or add a check below to not overflow the global structure.
-
Nico Huber (Code Review)