Change in flashrom[master]: Add support to get layout from fmap (e.g. coreboot rom)

[Nico Huber (Code Review)]

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