On Mon, Jan 25, 2021 at 06:37:14PM -0500, Taylor Blau wrote:
> +struct revindex_header {
> + uint32_t signature;
> + uint32_t version;
> + uint32_t hash_id;
> +};
> [...]
> + struct revindex_header *hdr;
> [...]
> + data = xmmap(NULL, revindex_size, PROT_READ, MAP_PRIVATE, fd, 0);
> + hdr = data;
> +
> + if (ntohl(hdr->signature) != RIDX_SIGNATURE) {
I guess this technique was pulled from the .idx code paths, but IMHO we
should avoid this kind of struct-casting. It relies on the compiler's
struct packing, and I'm not 100% sure it doesn't violate some weird
pointer-aliasing rules.
OTOH, as long as we do not ever care about sizeof(revindex_header), this
is likely to work in practice, since every field is at least 4-byte
aligned (but there is probably an extra 4 bytes of padding at the end).
The "right" way is probably something like:
const char *header = data;
if (get_be32(header) != RIDX_SIGNATURE)
error...;
header += 4;
if (get_be32(header) != 1)
error...;
header += 4;
...etc...
I thought we had helpers to read and advance the pointer, but it looks
like those are specific to the bitmap code (e.g., read_be32(), though it
uses a separate offset variable).
I dunno. Maybe I am being overly picky. The .idx code already does it
like this, and I believe the index (as in .git/index) does, too. We have
run into problems (as in b5007211b6 (pack-bitmap: do not use gcc packed
attribute, 2014-11-27)), but that was due to a more odd-sized struct, as
well as using sizeof().
The rest of the patch looks good to me.
-Peff
