Re: [PATCH] dir: support platforms that require aligned reads

["René Scharfe" <l.s.r@web.de>]

Am 16.07.2017 um 16:04 schrieb Jeff King:
> On Sun, Jul 16, 2017 at 02:17:37PM +0200, René Scharfe wrote:
> 
>> -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:
> 
>> 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. :(

Assuming that there is no padding probably even works for the platforms
the code currently supports (basically x86), but I don't know about
others.  We'd need to change the writing side as well to match, though.
Which is probably a good idea, but I tried to keep the patch small and
its impact low.  Cross-machine usability is currently explicitly not
supported -- not sure why, though.

René