42 comments:
spurious `to`?
Patch Set #19, Line 18: An example
actually `Examples`
Patch Set #19, Line 22: The fmap functions are mostly copied from cbfstool.
Which probably means we can't license them GPL v2+.
while still
allowing --layout
this is not what I read in the code?
Patch Set #19, Line 58: binary containing
s/fmap contained in ?
also, please maintain the 80 chars limit
Patch Set #19, Line 619: fill_flash->chip->total_size * 1024
NB. we should introduce a macro/function for this sooner than later
Patch Set #19, Line 33: * Software Foundation.
Is the above license not GPL compatible?
Patch Set #19, 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.
Patch Set #19, Line 55: 0x5F5F464D41505F5F
writing it in big endian seems weird
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`.
Patch Set #19, Line 40: #include "flash.h"
mmh?
Is this file altered? code added?
Patch Set #19, Line 51: sizeof(*fmap) + fmap->nareas * sizeof(struct fmap_area)
this is `fmap_size(fmap)`?
in any case, no need to break the line
Patch Set #19, Line 57: while (i < FMAP_STRLEN) {
seems weird to not use a `for` loop
Patch Set #19, Line 62: if (i == FMAP_STRLEN - 1) {
seems weird to check this inside the loop
Patch Set #19, Line 88: unsigned long int
`size_t`?
`bool`?
<= ?
Patch Set #19, Line 91: strlen(FMAP_SIGNATURE)
`sizeof(struct fmap)`
Patch Set #19, Line 92: const
casting to `const` is unnecessary
if (fmap_found)
break;
should be below the inner loop for enhanced readability
Patch Set #19, Line 126: (offset != 0)
so `&image[0]` is always checked? that doesn't seem to make much sense
Patch Set #19, Line 129: (const struct fmap *)&image[offset])) {
no need to break the line
Patch Set #19, Line 158: = -1;
initialization seems never used
Patch Set #19, 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?
Patch Set #19, 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)
Patch Set #19, 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?
Patch Set #19, Line 419: offset < 0
no, it's not (see type)
Patch Set #19, Line 426: sizeof(struct fmap) + sizeof(struct fmap_area) * fmap->nareas
there's a function for that
Patch Set #19, Line 428: !fmap_out
`!*fmap_out`
Patch Set #19, 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
<= ?
Patch Set #19, 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?
Patch Set #19, 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.
Patch Set #19, Line 479: if (rom_offset + len - offset < sizeof(struct fmap)) {
seems impossible by loop condition
Patch Set #19, Line 485: fmap = malloc(sizeof(struct fmap));
missing check for NULL
Patch Set #19, Line 486: if (flashctx->chip->read(flashctx, (uint8_t *)fmap, offset, sizeof(*fmap))) {
we could skip reading the signature again
Patch Set #19, Line 491: sizeof(struct fmap) + sizeof(struct fmap_area) * fmap->nareas
I'm sure I've seen a function for this.
Patch Set #19, Line 492: fmap = realloc(fmap, fmap_len);
missing check for NULL
Patch Set #19, Line 494: if (flashctx->chip->read(flashctx, (uint8_t *)fmap, offset, fmap_len)) {
we could skip reading the header again
Patch Set #19, Line 499: if (is_valid_fmap(fmap)) {
IIRC, this can be checked on the header alone
Patch Set #19, Line 515: return ret;
potentially leaking `fmap` if we didn't succeed
Patch Set #19, 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 change 23203. To unsubscribe, or for help writing mail filters, visit settings.