Re: [PATCH] dir: support platforms that require aligned reads
- From
Jeff King <peff@peff.net>
- Date
- Jul 16, 2017, 14:04 UTC
- Message-ID
- <20170716140409.3ywepgvo5c6ognsy@sigill.intra.peff.net>
- In-Reply-To
- <d3db2984-f238-7166-affa-f1f7df566404@web.de>
On Sun, Jul 16, 2017 at 02:17:37PM +0200, René Scharfe wrote:
Show 23 quoted lines
> -static void stat_data_from_disk(struct stat_data *to, const struct stat_data *from)
> +static void stat_data_from_disk(struct stat_data *to, const unsigned char *data)
> {
> - to->sd_ctime.sec = get_be32(&from->sd_ctime.sec);
> - to->sd_ctime.nsec = get_be32(&from->sd_ctime.nsec);
> - to->sd_mtime.sec = get_be32(&from->sd_mtime.sec);
> - to->sd_mtime.nsec = get_be32(&from->sd_mtime.nsec);
> - to->sd_dev = get_be32(&from->sd_dev);
> - to->sd_ino = get_be32(&from->sd_ino);
> - to->sd_uid = get_be32(&from->sd_uid);
> - to->sd_gid = get_be32(&from->sd_gid);
> - to->sd_size = get_be32(&from->sd_size);
> + memcpy(to, data, sizeof(*to));
> + to->sd_ctime.sec = ntohl(to->sd_ctime.sec);
> + to->sd_ctime.nsec = ntohl(to->sd_ctime.nsec);
> + to->sd_mtime.sec = ntohl(to->sd_mtime.sec);
> + to->sd_mtime.nsec = ntohl(to->sd_mtime.nsec);
> + to->sd_dev = ntohl(to->sd_dev);
> + to->sd_ino = ntohl(to->sd_ino);
> + to->sd_uid = ntohl(to->sd_uid);
> + to->sd_gid = ntohl(to->sd_gid);
> + to->sd_size = ntohl(to->sd_size);
> }Hmm. I would have written this to pull the bytes directly out of the array, like:
to->sd_ctime.sec = get_be32(data); data += 4; to->sd_ctime.nsec = get_be32(data); data += 4;
etc. Or even a helper to do the advancing like:
to->sd_ctime.sec = parse_be32(&data);
That reduces assumptions about padding in "struct stat_data". But looking more at this code, and reading your comment:
Show 5 quoted lines
> Side note: The OS name is not enough for determining the layout of > struct ondisk_untracked_cache. Different platforms can have different > int sizes and padding. Adding the machine type could help, but that > would be a breaking change. At that point we would be better off > defining a machine-independent format, no?
it looks like assumptions about struct layout are pervasive and part of the on-disk format. Yuck. :(
-Peff