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(a)aheymans.xyz>
Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz>
Gerrit-Reviewer: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de>
Gerrit-Reviewer: Paul Menzel <paulepanter(a)users.sourceforge.net>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Werner Zeh <werner.zeh(a)siemens.com>
Gerrit-Comment-Date: Thu, 09 Aug 2018 21:27:15 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No