Thanks for the thorough review - great suggestions as usual.
I finally had some time to look through this all. Some of the proposed changes required substantial refactoring, so the code looks quite a bit different than the previous patch. It looks less like the code in cbfstool and elsewhere, but by the same token should look more like flashrom code.
I re-tested --fmap with ROM images that had were bsearch-able and ones that required lsearch. --fmap-file was simplified to only use lsearch since bsearch doesn't really matter there anyway.
LMK what you think.
39 comments:
Patch Set #19, Line 58: fmap in <fmapfile>
s/fmap contained in ? […]
Dropped the word "binary" to make it <80.
NB. […]
*nod*
What should we call it? FLASH_SIZE_BYTES()? Or maybe flash_size_bytes() as a static inline in flash.h?
Patch Set #19, Line 33: * GNU General Public License ("GPL") version 2 as published by the Free
Is the above license not GPL compatible?
It's dual-licensed, as the last paragraph states. Also, in general BSD licensed code can be used in GPLv2 projects.
Or maybe I'm missing something?
Patch Set #19, Line 44: #define FMAP_VER_MAJOR 1 /* this header's FMAP minor version */
If we only support v1.1 but there were lower versions, please […]
Done
writing it in big endian seems weird
agreed
The mixed usage of `size_t`, `unsigned int` and `unsigned long […]
OK, I went ahead and updated that in a few places.
Some functions return <0 to indicate failure and >=0 if successful e.g. to indicate offset. To avoid mixing types they could be updated to take an off_t buffer as an output and return 0 or 1 to indicate failure or success. But that would make the API inconsistent with how it's used elsewhere, and probably not worth it for now.
Patch Set #19, Line 40: #include "flash.h"
mmh? […]
Yes.
this is `fmap_size(fmap)`? […]
Done
Patch Set #19, Line 57: if (fmap->ver_major != FMAP_VER_MAJOR)
seems weird to not use a `for` loop
Usually I'd agree, but I think the author may have been trying to clarify how the conditions within the loop affect the flow.
I don't have a strong preference either way, so I went ahead and updated it.
seems weird to check this inside the loop
I suspect the intention here is to say that if we reach FMAP_STRLEN - 1 without quitting due to the above if-statement, then then this should return failure. Another way to write this would be:
if ((i == FMAP_STRLEN - 1) && (fmap->name[i] != 0)
Or perhaps these two if-statements could be combined into:
if (!isgraph(fmap->name[i]) && (fmap->name[i] != 0)
Anyway, even if it seems a bit awkward to you or I it might be best to leave it as is unless there is a real problem to address.
Patch Set #19, Line 88: bool fmap_found =
`size_t`?
Done
`bool`?
Meh. Done.
<= ?
Yes, it should be inclusive. Done.
Patch Set #19, Line 91: *)&image[offset])) {
`sizeof(struct fmap)`
Yep, done.
Patch Set #19, Line 92: ap at
casting to `const` is unnecessary
Done
msg_gerr("Out of memory.\n");
return
should be below the inner loop for enhanced readability
Gratuitous, but will make the change anyway.
Patch Set #19, Line 124: p, fmap_size(fmap));
same as above where this should be inclusive and subtract the sizeof an fmap struct
so `&image[0]` is always checked? that doesn't seem to make much sense
True, I guess it was never a problem earlier, but of course now that we might have a slow programmers it's important to not waste cycles...
Anyway, I tried a couple of things and I think the simplest/cleanest solution to address that corner case is to have a variable that tracks whether we've checked offset 0.
Patch Set #19, Line 129: struct flashctx *const flashctx, size_t rom_offset, size_t len)
no need to break the line
Done
Patch Set #19, Line 158: malloc
initialization seems never used
Done.
Patch Set #19, Line 166: _free_ret:
I don't want to overoptimize things, but I have the feeling that […]
After thinking about this a bit, I think we should just get rid of fmap_bsearch in this file. We really only care about the bsearch if we're reading directly from ROM, but in this codepath we're only going to read from memory.
qualification has no effect here and the name shouldn't be […]
OK.
Patch Set #19, Line 67: struct flashrom_flashctx *, const uint8_t *const buf, size_t len);
The library was intentionally designed to be free from file i/o. […]
Sure. It took a bit of refactoring, but done.
`!*fmap_out`
Done
Patch Set #19, Line 442: msg_gdbg("Adding fmap layout to global layout.\n");
please break this line
Done
<= ?
Yep, this should be inclusive. Done.
Patch Set #19, Line 466: t flashctx *const flashctx, const uint8_t *cons
could be asserted at the beginning of the function, right?
Done. I had a check in the calling function, but it's probably clearer to put it at the beginning of this function.
both conditions don't make much sense, imho. it should be […]
Yes - I've added a variable to track whether or not we have checked offset 0. It's a bit of a kludge, but I think it addresses this corner case in the simplest manner.
This function always reads from flash so beginning at rom_offset is the most intuitive way.
Since this does not bail out on the stride loop we might as well use continue instead of break? if w […]
Yes, that sounds like a good idea.
While we're at it, I demoted this debug print to msg_cdbg(). If there's one failure due to a locked region, there will likely be many dozens or hundreds of similar benign failures.
seems impossible by loop condition
Agreed, removed.
Patch Set #19, Line 485: goto _free_ret;
missing check for NULL
Done
we could skip reading the signature again
Done
I'm sure I've seen a function for this.
Done
Patch Set #19, Line 492: return ret;
missing check for NULL
Done
we could skip reading the header again
Done
IIRC, this can be checked on the header alone
Done
Patch Set #19, Line 515: * @param layout Layout to bet set.
potentially leaking `fmap` if we didn't succeed
Done
msg_gdbg("Adding fmap layout to global layout.\n");
if (flashrom_layout_parse_fmap(layout, flashctx, fmap)) {
msg_gerr("Failed to add fmap regions to layout.\n");
ret = 1;
}
free(fmap);
return ret;
}
/**
* @brief Read a layout by searching buffer for fmap.
*
* @param[out] layout Points to a struct flashrom_layout pointer that
* gets set if the fmap is read and parsed successfully.
* @param[in] flashctx Flash context
* @param[in] buffer Buffer to search in
* @param[in] size Size of buffer to search
*
* @return 0 on success,
* 2 if the fmap couldn't be read,
* 1 on any other error.
*/
int flashrom_layout_read_fmap_from_buffer(struct flashrom_layout **const layout,
struct flashctx *const flashctx, const uint8_t *const buf, size_t size)
{
struct fmap *fmap = NULL;
int ret = 1;
if (!buf || !size)
goto _ret;
msg_gdbg("Attempting to read fmap from buffer.\n");
if (fmap_read_from_buffer(&fmap, buf, size)) {
msg_gerr("Failed to read fmap from buffer.\n");
ret = 1;
goto _ret;
}
msg_gdbg("Adding fmap layout to global layout.\n");
if (flashrom_layout_parse_fmap(layout, flashctx, fmap)) {
msg_gerr("Failed to add fmap regions to layout.\n");
ret = 1;
goto _free_ret;
}
ret = 0;
_free_ret:
free(fmap);
_ret:
return ret;
}
/**
* @brief Free a layout.
*
* @param layout Layout to free.
*/
void flashrom_layout_release(struct flashrom_layout *const layout)
{
if (layout == get_global_layout())
return;
free(layout);
}
/**
* @brief Set the active layout for a flash context.
*
* Note: This just sets a pointer. The caller must not release the layout
* as long as he uses it through the given flash context.
*
* @param flashctx Flash context whose layout will be set.
* @param layout Layout to bet set.
This whole thing should go in fmap.c.
We should either allocate it or add a check below to not overflow the […]
Done
To view, visit change 23203. To unsubscribe, or for help writing mail filters, visit settings.