On Mon, Sep 26, 2022 at 08:39:10AM -0700, Victoria Dye wrote: > > 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
