15 comments:
File src/commonlib/bsd/cbfs_private.c:
Patch Set #11, Line 8: size_t devsize = cbfs_dev_size(dev);
Maybe not terrible, but I think you should call this in cbfs_walk() and pass the value to this function. Otherwise, we'll be doing the size lookup for every file.
Patch Set #11, Line 52: be32toh(mdata.h.type)
You already have local variable, 'type', for this.
Patch Set #11, Line 70: goto next_file;
I can see where this might be the correct action for certain circumstances. Wouldn't we want to this to be fatal in others?
Patch Set #11, Line 75: offset + sizeof(mdata.h), todo) != todo)
I have a comment below related to this as well, but I don't see how we're guaranteed the mdata has enough buffer.
Patch Set #11, Line 117: const size_t todo = be32toh(src->h.offset) - already_read;
How do we know that:
1. cbfs_mdata is sufficiently large?
2. src->h.offset is >= already_read?
For the 2nd, I think we're relying on cbfs_walk() implementation to be the only sequence one would call copy_fill_metadata?
Patch Set #11, Line 135: args->namesize > already_read - offsetof(union cbfs_mdata, filename)
Separate this out into its own condition with a comment? It seems this check doesn't concern itself with the attributes -- only the filename. I'm not understanding the logic. And the || below then assumes we can read into filename when it's not entirely clear. Were you intending || to be &&?
Patch Set #11, Line 136: memcmp(args->name, mdata->filename, args->namesize) != 0)
We aren't ensuring filename (and CBFS_METADATA_MAX_SIZE coverage) is not being exceeded for out of bounds accesses.
File src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h:
Patch Set #11, Line 4: PRIVATE
What is the intention of private? Only a predetermined set of users should call these functions?
Perhaps call out the name (cbfs_glue.h) since this comment is large and it makes it clear what 'this' is referring to?
Patch Set #11, Line 18: * CBFS_ENABLE_HASHING Should be 0 to avoid linking hashing features, 1 otherwise.
I think this should be more explicit as to what hashing is covered. It's just the metadata.
Patch Set #11, Line 49: empty entries
What exactly does this mean? cbfs_file w/ no data, only metadata?
Patch Set #11, Line 82: enum cbfs_walk_flags
Since this is a bit field shouldn't we be passing in an unsigned type for the flags? Those flags are not mutually exclusive.
Patch Set #11, Line 87: * |dst| and load remaining metadata from |dev| as required.
I know we say copy here, but I think we should note that the fields are still in serialized format, i.e. big endian. I guess we should note the same is true everywhere w.r.t. this API. All cbfs_mdata is in serialized form?
Patch Set #11, Line 94: Verify the metadata with |master_hash| if provided.
From this description it's not clear what master_hash is representing for the target file. If it's truly the master hash for the entirety of cbfs metadata that would imply a full read of cbfs metadata to perform the calculation.
Also, if this is about verification why isn't master_hash object marked const?
To view, visit change 38421. To unsubscribe, or for help writing mail filters, visit settings.