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

quoted

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