12 comments:
File src/commonlib/bsd/cbfs_mcache.c:
There should be a format description of this cache.
Patch Set #4, Line 26: if (args->end - args->mcache < sizeof(*token))
This seems like we're dangling lots of conditions on the state of teh cache. Why can't we be more explicit that every file entry needs a token as well as needing space for the entry indicating termination of the cbfs? If those things aren't met we have failed entirely -- and propagate that situation up the stack?
Patch Set #4, Line 42: if (offset != args->offset)
Why is this conditional? I feel like we should always keep a token marker for every file.
Patch Set #4, Line 54: args->offset = ALIGN_UP(offset + data_offset + data_length, CBFS_ALIGNMENT);
I'm not clear why we would be keeping track of this at all. The walker should just spit offsets out that are consumed and logged.
Patch Set #4, Line 72: ERROR("mcache overflow, should increase CBFS_MCACHE size!\n");
This should be a major error.
By default we are falling back to the boot media? Why? Shouldn't we have an error when this occurs? Likewise there isn't a comment nor a console output indicating this explicit case was hit.
Patch Set #4, Line 135: if (memcmp(mdata->h.magic, CBFS_FILE_MAGIC, sizeof(mdata->h.magic)) != 0) {
This needs some commentary. From what I can tell there's a token if there's no cbfs metadata, correct?
File src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h:
Patch Set #4, Line 98: struct vb2_hash *master_hash);
Why isn't master_hash const?
Patch Set #4, Line 299: const struct cbfs_boot_device *rw = vboot_get_cbfs_boot_device();
When this returns NULL what semantics is it indicating?
What else?
Patch Set #4, Line 310: cbmem_entry_find
This failing is a broader problem. I don't think we should fall back into ENV_ROMSTAGE_OR_BEFORE case.
Patch Set #4, Line 324: ENV_BOOTBLOCK
How do you envision this stuff working with separate verstage and NO_RETURN_FROM_VERSTAGE?
File src/security/vboot/vboot_loader.c:
Patch Set #4, Line 46: return; /* No RW mcache if we're staying in RO. */
Dont' we already have this covered with vboot_get_cbfs_boot_device() returning NULL?
To view, visit change 38423. To unsubscribe, or for help writing mail filters, visit settings.