Re: [PATCH] read-cache: avoid misaligned reads in index v4

From: Jeff King <peff@peff.net>
Date: Sep 26, 2022, 17:35 UTC
Message-ID: <YzHiyPkCgVHym3H4@coredump.intra.peff.net>
In-Reply-To: <e5954e90-6b5c-46a6-0842-b3d7d1e06b33@github.com>

On Mon, Sep 26, 2022 at 08:39:10AM -0700, Victoria Dye wrote:

Show 15 quoted lines

> > So this can just be:
> > 
> >   ce->ce_stat_data.sd_mtime.sec = get_be32(ondisk + offsetof(struct ondisk_cache_entry, mtime));
> > 
> > which is mercifully shorter.
> > 
> > Assuming we dismiss the rest of what I said as not worth it for a
> > minimal fix, I do think that simplification is worth rolling a v2.
> 
> That makes sense from a technical perspective, but I included the starting
> entry offset for readability reasons. It might be confusing to someone
> unfamiliar with C struct memory alignment to see every other 'get_be32'
> refer to the exact entry it's reading via the 'offsetof()', but have that
> information absent only for a few entries. And, the double 'offsetof()'
> would still be used by the 'mtime.nsec'/'ctime.nsec' fields anyway.

Ah, right, I wasn't looking close enough. I was thinking that you were reading the whole struct via a single function call, but of course that is not true with get_be32(), and the nsec loads just below make that obvious.

> In any case, if this patch is intended to be a short-lived change on the way
> to a more complete refactor and/or I'm being overzealous on the readability,
> I'd be happy to change it. :)

No, I was just mis-reading it. I think what you've got here is a good stopping point to fix the immediate problem.

-Peff