Almost there...
20 comments:
I would go for flash_size(), keep things simple: […]
Sounds good to me. Since there are dozens of places that will need to change, I'll send a follow-up patch.
Patch Set #20, Line 95: int main(int argc, char *argv[])
I just remembered that we have a read_buf_from_file() function in flashrom.c. Would be nice to have a file handling library eventually to make this stuff more obvious.
This should be -1!
Yes, it most certainly should!
"rb" for windows? not sure
Done
Patch Set #20, Line 655: lashrom_flag_set(fill_flash, FLASHROM_FLAG_VERIF
updated to use read_buf_from_file()
Maybe add "flashrom" here? or at least an "it" would make the sentence correct.
I've updated this to use similar language as the --ifd option.
`const` on the parameter itself is meaningless in the contract
True. I think I saw it elsewhere in the codebase, wasn't sure if it's intended for readability or something. Anyway, no need to clutter this.
could be static now
Done
All callers seem to know that it's non-NULL but ignore possible […]
Makes sense, done.
What you actually check here is not the overall flashsize itself but the size of this FMAP instance […]
@Werner: fmap->size is supposed to be the size of the address space we're working with (e.g. the flash size). It's not very well documented I guess :-/
@Nico: No, fmap doesn't have any concept of nesting.
changed to msg_gdbg() since caller will likely print an error message
uint8_t *buf;
if (!flashctx || !flashctx->chip)
return 1;
if (prepare_flash_access(flashctx, true, false, false, false))
goto _finalize_ret;
/* likely more memory than we need, but it simplifies handling and
* printing offsets to keep them uniform with what's on the ROM */
buf = malloc(rom_offset + len);
if (!buf) {
msg_gerr("Out of memory.\n");
goto _finalize_ret;
Beside the error message, this is probably the same as calling […]
Yep, let's clean up the redundant code.
}
make both const?
Done
size_t rom_offset, size_t len, int min_stride)
{
Or just move it up into fmap_read_from_rom()?
Moved to fmap_read_from_rom()
ret = 2;
continue;
}
fmap_len = fmap_size(fmap);
struct fmap *tmp = fmap;
f
realloc() frees the original memory if moved. So we only have […]
According to the man page: "If the area pointed to was moved, a free(ptr) is done."
the later const is not meaningful here
Done
Patch Set #20, Line 396: msg_gerr("Cannot add fmap entries to layout - Too many entries.\n");
we could also check in advance `if (fmap->nareas > MAX_ROMLAYOUT)` […]
I like that idea, so I went ahead and added the check before the loop (using l->num_entries + fmap->nareas).
Patch Set #20, Line 405: emcpy(l->entries[l->num_en
FMAP_STRLEN is shorter, use that or MIN() of both? […]
Good ideas. I went ahead and just memset() romentry's name to 0. It's overkill, but not having garbage in that memory may help in case we need to debug something.
Patch Set #20, Line 477: msg_gerr("Failed to read fmap from buffer.\n");
redundant
Done
Patch Set #20, Line 484: goto _free_ret;
redundant
Done
To view, visit change 23203. To unsubscribe, or for help writing mail filters, visit settings.