Re: [PATCH v3 2/3] linux: statvfs: allocate spare for f_type

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

On 21/07/23 12:34, наб wrote:
> On Fri, Jul 21, 2023 at 10:03:05AM -0300, Adhemerval Zanella Netto wrote:
>> On 03/07/23 11:59, наб via Libc-alpha wrote:
>>> diff --git a/sysdeps/unix/sysv/linux/bits/statvfs.h b/sysdeps/unix/sysv/linux/bits/statvfs.h
>>> index 8dfb5ce761..cf98460e00 100644
>>> --- a/sysdeps/unix/sysv/linux/bits/statvfs.h
>>> +++ b/sysdeps/unix/sysv/linux/bits/statvfs.h
>>> @@ -51,7 +51,8 @@ struct statvfs
>>>  #endif
>>>      unsigned long int f_flag;
>>>      unsigned long int f_namemax;
>>> -    int __f_spare[6];
>>> +    unsigned int f_type;
>>> +    int __f_spare[5];
>>>    };
>>>  
>>>  #ifdef __USE_LARGEFILE64
>>> @@ -71,7 +72,8 @@ struct statvfs64
>>>  #endif
>>>      unsigned long int f_flag;
>>>      unsigned long int f_namemax;
>>> -    int __f_spare[6];
>>> +    unsigned int f_type;
>>> +    int __f_spare[5];
>>>    };
>>>  #endif
>>>  
>> All ABIs define fstatfs/statfs64 as signed, as the constants in include/uapi/linux/magic.h.
>> Should we do the same here?
> Kernel sources say
>   include/asm-generic/compat.h:   compat_int_t    f_type;
>   include/linux/statfs.h: long f_type;
>   include/uapi/asm-generic/statfs.h:      __statfs_word f_type;
>   include/uapi/asm-generic/statfs.h:      __statfs_word f_type;
>   include/uapi/asm-generic/statfs.h:      __u32 f_type;
> and
>   arch/alpha/include/uapi/asm/statfs.h:#define __statfs_word __u32
>   arch/parisc/include/uapi/asm/statfs.h:#define __statfs_word long
>   include/uapi/asm-generic/statfs.h:#ifndef __statfs_word
>   include/uapi/asm-generic/statfs.h:#define __statfs_word __kernel_long_t
>   include/uapi/asm-generic/statfs.h:#define __statfs_word __u32
> so it's a damn mess: it looks to me like it's a signed long when it just
> kinda worked out that way but u32 when actually constructing this ABI
> for new arches(?).

From kernel headers it is 'long' for 64 bits and 'uint32_t' for 32 bits 
(and off course with a couple of architectures exceptions).

> 
> I opted for an u32 because the raw constants
> represent "4 bytes of data", not "4-byte number";
> this is also consistent with the hurd.
> 
> That said,
>   $ man fstatfs | grep -i "$(printf '%08x\n' $(man fstatfs | grep -o '0x[^ 	]*') | grep '^[8-f]')"
>              BPF_FS_MAGIC          0xcafe4a11
>              BTRFS_SUPER_MAGIC     0x9123683e
>              CIFS_MAGIC_NUMBER     0xff534d42
>              EFIVARFS_MAGIC        0xde5e81e4
>              F2FS_SUPER_MAGIC      0xf2f52010
>              HPFS_SUPER_MAGIC      0xf995e849
>              HUGETLBFS_MAGIC       0x958458f6
>              RAMFS_MAGIC           0x858458f6
>              SELINUX_MAGIC         0xf97cff8c
>              SMB2_MAGIC_NUMBER     0xfe534d42
>              VXFS_SUPER_MAGIC      0xa501fcf5
>              XENFS_SUPER_MAGIC     0xabba1974
> so a quick test with
>   $ cat fs.cpp
>   #include <type_traits>
>   #include <stdio.h>
>   #include <sys/vfs.h>
>   #include <linux/magic.h>
>   
>   int main() {
>           printf("signed? %d\n", std::is_signed_v<decltype(statfs::f_type)>);
>           printf("sizeof? %zu\n", sizeof(statfs::f_type));
>           struct statfs sf;
>           fstatfs(0, &sf);
>           printf("sf.f_type as lu=%lu\n", sf.f_type);
>           unsigned int tpu = sf.f_type;
>           int tpd = sf.f_type;
>           printf("sf.f_type as d=%d? %d\n", tpu, tpu == HUGETLBFS_MAGIC);
>           printf("sf.f_type as u=%u? %d\n", tpd, tpd == HUGETLBFS_MAGIC);
>   }
> 
>   $ dpkg --print-architecture; ./fs < /dev/hugepages/
>   amd64
>   signed? 1
>   sizeof? 8
>   sf.f_type as lu=2508478710
>   sf.f_type as d=-1786488586? 1
>   sf.f_type as u=2508478710? 1
>   
>   $ dpkg --print-architecture; ./fs < /dev/hugepages/
>   ppc64el
>   signed? 1
>   sizeof? 8
>   sf.f_type as lu=2508478710
>   sf.f_type as d=-1786488586? 1
>   sf.f_type as u=2508478710? 1
>   
>   $ dpkg --print-architecture; ./fs < /dev/hugepages/
>   s390x
>   signed? 0
>   sizeof? 4
>   sf.f_type as lu=2240043254    # my vm doesn't have hugetlbfs, so used ramfs
>   sf.f_type as d=-2054924042? 1
>   sf.f_type as u=2240043254? 1
> reveals that there aren't really any issues we run into with signedness here.
> In C:
>  switch(tpu/tpd/sf.f_type) { case ..._MAGIC: puts("tpu"); } works,
>  and comparing against const unsigned tt = ..._MAGIC; works;
>  comparing against const int tt = ..._MAGIC; doesn't work for sf.f_type.
> 
> In C++:
>   c++     fs.cpp   -o fs
>   fs.cpp:18:27: error: case value evaluates to 2508478710, which cannot be narrowed to type 'int' [-Wc++11-narrowing]
>      18 |                             switch(tpd) { case HUGETLBFS_MAGIC: puts("tpd"); }
>         |                                                ^
>   /usr/include/linux/magic.h:19:26: note: expanded from macro 'HUGETLBFS_MAGIC'
>      19 | #define HUGETLBFS_MAGIC         0x958458f6      /* some random number */
>         |                                 ^
>   1 error generated.
>   make: *** [<builtin>: fs] Error 1
> 
> So this Must be an u32 to behave correctly in C++,
> we don't lose anything else for making it unsigned,
> and it's consistent with Hurd.
> 
> Thoughts?

Fair enough, I think with current kernel ABI status using unsigned would be
the best option.