16 comments:
File src/commonlib/bsd/cbfs_private.c:
Patch Set #4, Line 10: cbfs_dev_size(dev)
Shouldn't we just call this once? The size of dev shouldn't change.
Patch Set #4, Line 29: ENABLE_HASHING
This should probably be namespaced better.
Patch Set #4, Line 54: goto next_file;
Shouldn't this be a major error? If we're wanting to ensure we touch every file this algo just bypasses things that aren't the correct size.
Patch Set #4, Line 56: if (offset + data_offset >= cbfs_dev_size(dev)) {
Wouldn't this fail on the read()? But if you are wanting to check this shouldn't you be ensuring the data length doesn't exceed the device size too?
Patch Set #4, Line 77: (uint8_t *)&mdata
Shouldn't we be reading into the byte array here and above? Then interpret through the union for the cbfs_file?
Patch Set #4, Line 88: For others, finish the hash first if needed.
Could you please elaborate on why?
Patch Set #4, Line 102: sizeof(real_hash)
sorry for not being familiar w/ vb2 APIs, but it seems weird to pass in sizeof(real_hash) when it's just the possible max -- not the actual digest size.
There doesn't appear to be any checks against the size of the structures. offset and attributes_offset could exceed the size of cbf_mdata. You're checking data_offset above. But not that the data fits.
File src/commonlib/bsd/include/commonlib/bsd/cbfs_private.h:
Patch Set #4, Line 61: header, filename and attributes
Shouldn't we be passing along the metadata size? Why is the callback needing the logic to figure out where the attributes and filename live? I know's it's some addition, but it seems better to calculate that up front?
Patch Set #4, Line 69: * the remaining CBFS (but not call |walker| anymore).
This function seems a bit overloaded. Can't we wrap all this with some entry points that provide the semantics we need w/o throwing everything into a single call?
Patch Set #4, Line 72: union cbfs_mdata *mdata
Why isn't this const?
Patch Set #4, Line 80: to copy behind that
I don't understand what this is saying. If do_hash is true it won't copy additional attributes? What is additional? Is it just omitting the hash attributes?
Patch Set #4, Line 83: union cbfs_mdata *src
const
Patch Set #4, Line 87: bit shorter than this
'This' being the size returned?
Patch Set #4, Line 91: offset to the file data
relative or absolute within |dev| ?
Also, what is master_hash at this point? Is the assumption that all of cbfs metadata is checked prior to filling in the parameters? Or is this the hash for the metadata itself?
File src/commonlib/bsd/include/commonlib/bsd/cbfs_serialized.h:
Patch Set #4, Line 131: struct vb2_hash hash;
>-------union {
>------->-------uint8_t raw[0];
#if VB2_SUPPORT_SHA1
>------->-------uint8_t sha1[VB2_SHA1_DIGEST_SIZE];
#endif
#if VB2_SUPPORT_SHA256
>------->-------uint8_t sha256[VB2_SHA256_DIGEST_SIZE];
#endif
#if VB2_SUPPORT_SHA512
>------->-------uint8_t sha512[VB2_SHA512_DIGEST_SIZE];
#endif
>-------} bytes;
How are we envisioning dealing with this? From your added comment it'll be dynamic as before?
The other thing that you've done here is changed the serialized format by changing hash_type from a 32-bit value to a single byte. Shouldn't we add a new type and tag so things can be backwards compatible?
To view, visit change 38421. To unsubscribe, or for help writing mail filters, visit settings.