Re: [PATCH v2 13/25] y2038: Use a common definition for stat

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 04/06/2021 16:37, Carlos O'Donell wrote:
> On 5/18/21 4:56 PM, Adhemerval Zanella wrote:
>> From: Lukasz Majewski <lukma@denx.de>
>>
>> Instead of replicate the same definitions from struct_stat_time64.h
>> on the multiple struct_stat.h, use a common header which is included
>> when required (struct_stat_time64_helper.h).  The 64-bit time support
>> is added only for LFS support.
>>
>> The __USE_TIME_BITS64 is not defined internally yet, although the
>> internal header is used when building the 64-bit stat implementations.
> 
> Requesting a v3 please.
> 
> Please add the __glibc_reserved* members to the structures to preserve
> maximum compatibility with the kernel structure.
> 
> See review of patch 14/25 for more general details.
> 
> I am almost OK with this change as-is because in general everyone
> should be using statx at this point with the ability to extend and
> provide more information.
> 
> However because the kernel contains:
> 
>  24 struct stat {

This struct more specifically is the one used on compat non-LFS syscalls,
which glibc only issues for compat symbols. But even the 'struct stat64'
does add the two extra unused members.

>  25         unsigned long   st_dev;         /* Device.  */
>  26         unsigned long   st_ino;         /* File serial number.  */
>  27         unsigned int    st_mode;        /* File mode.  */
>  28         unsigned int    st_nlink;       /* Link count.  */
>  29         unsigned int    st_uid;         /* User ID of the file's owner.  */
>  30         unsigned int    st_gid;         /* Group ID of the file's group. */
>  31         unsigned long   st_rdev;        /* Device number, if device.  */
>  32         unsigned long   __pad1;
>  33         long            st_size;        /* Size of file, in bytes.  */
>  34         int             st_blksize;     /* Optimal block size for I/O.  */
>  35         int             __pad2;
>  36         long            st_blocks;      /* Number 512-byte blocks allocated. */
>  37         long            st_atime;       /* Time of last access.  */
>  38         unsigned long   st_atime_nsec;
>  39         long            st_mtime;       /* Time of last modification.  */
>  40         unsigned long   st_mtime_nsec;
>  41         long            st_ctime;       /* Time of last status change.  */
>  42         unsigned long   st_ctime_nsec;
>  43         unsigned int    __unused4;
>  44         unsigned int    __unused5;
>  45 };
> 
> With __unused4 and __unused5. I think we should match this ABI as closely as we can
> to avoid future compatibility issues with legacy APIs.
> 

These kernel interfaces are really a mess and not really in sync across
architectures. Some ABI adds the reserved fields only for non LFS,
which the generic interface adds regardless.  And from kernel comment
the reversed fields were added to allow glibc compatibility, not
really to provide further extension.

In any case , I think it should not hurt to add two extra unsigned int 
reserved fields for 64 bit time support.


> OK. Reindent.
> 
>>    };
>>  #endif
>>  
>> @@ -127,5 +135,4 @@ struct stat64
>>  /* Nanosecond resolution time values are supported.  */
>>  #define _STATBUF_ST_NSEC
>>  
>> -
> 
> Please avoid spurious empty line removal when not related to the patch at hand.

Ack.

> 
>> +  __blksize_t st_blksize;	/* Optimal block size for I/O.  */
>> +  __blkcnt64_t st_blocks;	/* Number 512-byte blocks allocated. */
> 
> OK.
> 
> Plase add __glibc_reserved4 and __glibc_reserved5 here.

Ack.