Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38421 )
Change subject: commonlib/bsd: Add new CBFS core implementation ......................................................................
Patch Set 12:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/cbfs_pri... File src/commonlib/bsd/cbfs_private.c:
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/cbfs_pri... PS11, Line 75: offset + sizeof(mdata.h), todo) != todo)
Line 53 guarantees sizeof(mdata) >= data_offset
Should line 55 return an error then instead of continuing? I am concerned about silently missing the assumption that data_offset will always be <= sizeof(mdata).
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... File src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h:
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... PS11, Line 4: PRIVATE
Thanks for the explanation. […]
I think having a comment about intention that you provided above, Julius, would be helpful. Obviously it doesn't need to be as long as what wrote, but explain the intention. That way we can point people at it if we see violations of the original intent.
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... PS11, Line 82: enum cbfs_walk_flags
I much prefer using an enum over a plain unsigned int. […]
It is helpful to provide that extra hint, but an enum tells me that it's a single value of that enumeration -- not a combination of values from the enumeration. I guess I'm in the minority on that, but enum doesn't scream combination of flags.
https://review.coreboot.org/c/coreboot/+/38421/11/src/commonlib/bsd/include/... PS11, Line 94: Verify the metadata with |master_hash| if provided.
Discarding a const qualifier is a very bad idea, IMHO.
Ya. I wouldn't cast off the const.