Change in flashrom[master]: Add support to get layout from fmap (e.g. coreboot rom)
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. -- To view, visit https://review.coreboot.org/23203 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0e7fad38ed79a84d41358e1f175c36d255786c12 Gerrit-Change-Number: 23203 Gerrit-PatchSet: 19 Gerrit-Owner: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: David Hendricks <david.hendricks@gmail.com> Gerrit-Reviewer: Nico Huber <nico.h@gmx.de> Gerrit-Reviewer: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: Werner Zeh <werner.zeh@siemens.com> Gerrit-Comment-Date: Thu, 09 Aug 2018 21:27:15 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
participants (1)
-
Nico Huber (Code Review)