[PATCH v3 1/3] hurd: statvfs: __f_type -> f_type

[PATCH v3 1/3] hurd: statvfs: __f_type -> f_type

[наб]

[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]

No further changes needed ([f]statvfs() just cast to struct statfs *
and call [f]statfs()).

Link: https://lore.kernel.org/linux-man/f54kudgblgk643u32tb6at4cd3kkzha6hslahv24szs4raroaz@ogivjbfdaqtb/t/#u
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
---
 sysdeps/mach/hurd/bits/statvfs.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sysdeps/mach/hurd/bits/statvfs.h b/sysdeps/mach/hurd/bits/statvfs.h
index 3fbfd93335..a94759ef55 100644
--- a/sysdeps/mach/hurd/bits/statvfs.h
+++ b/sysdeps/mach/hurd/bits/statvfs.h
@@ -29,7 +29,7 @@
 
 struct statvfs
   {
-    unsigned int __f_type;
+    unsigned int f_type;
     unsigned long int f_bsize;
 #ifndef __USE_FILE_OFFSET64
     __fsblkcnt_t f_blocks;
@@ -59,7 +59,7 @@ struct statvfs
 #ifdef __USE_LARGEFILE64
 struct statvfs64
   {
-    unsigned int __f_type;
+    unsigned int f_type;
     unsigned long int f_bsize;
     __fsblkcnt64_t f_blocks;
     __fsblkcnt64_t f_bfree;
-- 
2.39.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[наб]

[-- Attachment #1: Type: text/plain, Size: 2579 bytes --]

This is the only missing part in struct statvfs.
The LSB calls [f]statfs() deprecated, and its weird types are definitely
off-putting. However, its use is required to get f_type.

Instead, allocate one of the six spares to f_type,
copied directly from struct statfs.
This then becomes a small glibc extension to the standard interface
on Linux and the Hurd, instead of two different interfaces, one of which
is quite odd due to being an ABI type, and there no longer is any reason
to use statfs().

The underlying kernel type is a mess, but all architectures agree on u32
(or more) for the ABI, and all filesystem magicks are 32-bit integers.

Link: https://lore.kernel.org/linux-man/f54kudgblgk643u32tb6at4cd3kkzha6hslahv24szs4raroaz@ogivjbfdaqtb/t/#u
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
 sysdeps/unix/sysv/linux/bits/statvfs.h     | 6 ++++--
 sysdeps/unix/sysv/linux/internal_statvfs.c | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

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
 
diff --git a/sysdeps/unix/sysv/linux/internal_statvfs.c b/sysdeps/unix/sysv/linux/internal_statvfs.c
index 6a1b7b755f..112d3c241a 100644
--- a/sysdeps/unix/sysv/linux/internal_statvfs.c
+++ b/sysdeps/unix/sysv/linux/internal_statvfs.c
@@ -57,6 +57,7 @@ __internal_statvfs (struct statvfs *buf, const struct statfs *fsbuf)
   buf->__f_unused = 0;
 #endif
   buf->f_namemax = fsbuf->f_namelen;
+  buf->f_type = fsbuf->f_type;
   memset (buf->__f_spare, '\0', sizeof (buf->__f_spare));
 
   /* What remains to do is to fill the fields f_favail and f_flag.  */
@@ -99,6 +100,7 @@ __internal_statvfs64 (struct statvfs64 *buf, const struct statfs64 *fsbuf)
   buf->__f_unused = 0;
 #endif
   buf->f_namemax = fsbuf->f_namelen;
+  buf->f_type = fsbuf->f_type;
   memset (buf->__f_spare, '\0', sizeof (buf->__f_spare));
 
   /* What remains to do is to fill the fields f_favail and f_flag.  */
-- 
2.39.2


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 3/3] statvfs: f_type: NEWS & test

[наб]

[-- Attachment #1: Type: text/plain, Size: 2247 bytes --]

Also fix tst-statvfs so that it actually fails;
as it stood, all it did was return 0 always.

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
 NEWS             |  5 +++++
 io/tst-statvfs.c | 19 +++++++++++--------
 2 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/NEWS b/NEWS
index 709ee40e50..fc2392f168 100644
--- a/NEWS
+++ b/NEWS
@@ -48,6 +48,11 @@ Major new features:
 * The strlcpy and strlcat functions have been added.  They are derived
   from OpenBSD, and are expected to be added to a future POSIX version.
 
+* struct statvfs now has an f_type member, equal to the f_type statfs member;
+  on the Hurd this was always available under a reserved name,
+  and under Linux a spare has been allocated: it was always zero
+  in previous versions of glibc, and zero is not a valid result.
+
 Deprecated and removed features, and other changes affecting compatibility:
 
 * In the Linux kernel for the hppa/parisc architecture some of the
diff --git a/io/tst-statvfs.c b/io/tst-statvfs.c
index 227c62d7da..b38ecae466 100644
--- a/io/tst-statvfs.c
+++ b/io/tst-statvfs.c
@@ -1,5 +1,7 @@
 #include <stdio.h>
+#include <sys/statfs.h>
 #include <sys/statvfs.h>
+#include <support/check.h>
 
 
 /* This test cannot detect many errors.  But it will fail if the
@@ -11,17 +13,18 @@ do_test (int argc, char *argv[])
   for (int i = 1; i < argc; ++i)
     {
       struct statvfs st;
-      if (statvfs (argv[i], &st) != 0)
-        printf ("%s: failed (%m)\n", argv[i]);
-      else
-        printf ("%s: free: %llu, mandatory: %s\n", argv[i],
-                (unsigned long long int) st.f_bfree,
+      struct statfs stf;
+      TEST_COMPARE (statvfs (argv[i], &st), 0);
+      TEST_COMPARE (statfs (argv[i], &stf), 0);
+      TEST_COMPARE (st.f_type, stf.f_type);
+      printf ("%s: free: %llu, mandatory: %s\n", argv[i],
+              (unsigned long long int) st.f_bfree,
 #ifdef ST_MANDLOCK
-                (st.f_flag & ST_MANDLOCK) ? "yes" : "no"
+              (st.f_flag & ST_MANDLOCK) ? "yes" : "no"
 #else
-                "no"
+              "no"
 #endif
-                );
+              );
     }
   return 0;
 }
-- 
2.39.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Adhemerval Zanella Netto]

On 03/07/23 11:59, наб via Libc-alpha wrote:
> This is the only missing part in struct statvfs.
> The LSB calls [f]statfs() deprecated, and its weird types are definitely
> off-putting. However, its use is required to get f_type.
> 
> Instead, allocate one of the six spares to f_type,
> copied directly from struct .
> This then becomes a small glibc extension to the standard interface
> on Linux and the Hurd, instead of two different interfaces, one of which
> is quite odd due to being an ABI type, and there no longer is any reason
> to use statfs().
> 
> The underlying kernel type is a mess, but all architectures agree on u32
> (or more) for the ABI, and all filesystem magicks are 32-bit integers.
> 
> Link: https://lore.kernel.org/linux-man/f54kudgblgk643u32tb6at4cd3kkzha6hslahv24szs4raroaz@ogivjbfdaqtb/t/#u
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> ---
>  sysdeps/unix/sysv/linux/bits/statvfs.h     | 6 ++++--
>  sysdeps/unix/sysv/linux/internal_statvfs.c | 2 ++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> 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?

> diff --git a/sysdeps/unix/sysv/linux/internal_statvfs.c b/sysdeps/unix/sysv/linux/internal_statvfs.c
> index 6a1b7b755f..112d3c241a 100644
> --- a/sysdeps/unix/sysv/linux/internal_statvfs.c
> +++ b/sysdeps/unix/sysv/linux/internal_statvfs.c
> @@ -57,6 +57,7 @@ __internal_statvfs (struct statvfs *buf, const struct statfs *fsbuf)
>    buf->__f_unused = 0;
>  #endif
>    buf->f_namemax = fsbuf->f_namelen;
> +  buf->f_type = fsbuf->f_type;
>    memset (buf->__f_spare, '\0', sizeof (buf->__f_spare));
>  
>    /* What remains to do is to fill the fields f_favail and f_flag.  */
> @@ -99,6 +100,7 @@ __internal_statvfs64 (struct statvfs64 *buf, const struct statfs64 *fsbuf)
>    buf->__f_unused = 0;
>  #endif
>    buf->f_namemax = fsbuf->f_namelen;
> +  buf->f_type = fsbuf->f_type;
>    memset (buf->__f_spare, '\0', sizeof (buf->__f_spare));
>  
>    /* What remains to do is to fill the fields f_favail and f_flag.  */

Re: [PATCH v3 3/3] statvfs: f_type: NEWS & test

[Adhemerval Zanella Netto]

On 03/07/23 11:59, наб via Libc-alpha wrote:
> Also fix tst-statvfs so that it actually fails;
> as it stood, all it did was return 0 always.
> 
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

I think this patch should be squashed with previous one, rest looks ok.

> ---
>  NEWS             |  5 +++++
>  io/tst-statvfs.c | 19 +++++++++++--------
>  2 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 709ee40e50..fc2392f168 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -48,6 +48,11 @@ Major new features:
>  * The strlcpy and strlcat functions have been added.  They are derived
>    from OpenBSD, and are expected to be added to a future POSIX version.
>  
> +* struct statvfs now has an f_type member, equal to the f_type statfs member;
> +  on the Hurd this was always available under a reserved name,
> +  and under Linux a spare has been allocated: it was always zero
> +  in previous versions of glibc, and zero is not a valid result.
> +
>  Deprecated and removed features, and other changes affecting compatibility:
>  
>  * In the Linux kernel for the hppa/parisc architecture some of the
> diff --git a/io/tst-statvfs.c b/io/tst-statvfs.c
> index 227c62d7da..b38ecae466 100644
> --- a/io/tst-statvfs.c
> +++ b/io/tst-statvfs.c
> @@ -1,5 +1,7 @@
>  #include <stdio.h>
> +#include <sys/statfs.h>
>  #include <sys/statvfs.h>
> +#include <support/check.h>
>  
>  
>  /* This test cannot detect many errors.  But it will fail if the
> @@ -11,17 +13,18 @@ do_test (int argc, char *argv[])
>    for (int i = 1; i < argc; ++i)
>      {
>        struct statvfs st;
> -      if (statvfs (argv[i], &st) != 0)
> -        printf ("%s: failed (%m)\n", argv[i]);
> -      else
> -        printf ("%s: free: %llu, mandatory: %s\n", argv[i],
> -                (unsigned long long int) st.f_bfree,
> +      struct statfs stf;
> +      TEST_COMPARE (statvfs (argv[i], &st), 0);
> +      TEST_COMPARE (statfs (argv[i], &stf), 0);
> +      TEST_COMPARE (st.f_type, stf.f_type);
> +      printf ("%s: free: %llu, mandatory: %s\n", argv[i],
> +              (unsigned long long int) st.f_bfree,
>  #ifdef ST_MANDLOCK
> -                (st.f_flag & ST_MANDLOCK) ? "yes" : "no"
> +              (st.f_flag & ST_MANDLOCK) ? "yes" : "no"
>  #else
> -                "no"
> +              "no"
>  #endif
> -                );
> +              );
>      }
>    return 0;
>  }

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

[наб]

[-- Attachment #1: Type: text/plain, Size: 4897 bytes --]

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(?).

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Adhemerval Zanella Netto]

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.

Re: [PATCH v3 3/3] statvfs: f_type: NEWS & test

[Florian Weimer]

* наб:

> diff --git a/io/tst-statvfs.c b/io/tst-statvfs.c
> index 227c62d7da..b38ecae466 100644
> --- a/io/tst-statvfs.c
> +++ b/io/tst-statvfs.c
> @@ -1,5 +1,7 @@
>  #include <stdio.h>
> +#include <sys/statfs.h>
>  #include <sys/statvfs.h>
> +#include <support/check.h>
>  
>  
>  /* This test cannot detect many errors.  But it will fail if the
> @@ -11,17 +13,18 @@ do_test (int argc, char *argv[])
>    for (int i = 1; i < argc; ++i)
>      {
>        struct statvfs st;
> -      if (statvfs (argv[i], &st) != 0)
> -        printf ("%s: failed (%m)\n", argv[i]);
> -      else
> -        printf ("%s: free: %llu, mandatory: %s\n", argv[i],
> -                (unsigned long long int) st.f_bfree,
> +      struct statfs stf;
> +      TEST_COMPARE (statvfs (argv[i], &st), 0);
> +      TEST_COMPARE (statfs (argv[i], &stf), 0);
> +      TEST_COMPARE (st.f_type, stf.f_type);

This fails with certain file systems because the types of f_type differ
in signedness:

=====FAIL: io/tst-statvfs.out=====
tst-statvfs.c:19: numeric comparison failure
   left: 2435016766 (0x9123683e); from: st.f_type
  right: -1859950530 (0x9123683e); from: stf.f_type
/builddir/build/BUILD/glibc-2.38.9000-40-gd6fe19facc/build-i686-redhat-linux/io/
tst-statvfs: free: 53658025, mandatory: no, tp=9123683e
tst-statvfs.c:19: numeric comparison failure
   left: 2435016766 (0x9123683e); from: st.f_type
  right: -1859950530 (0x9123683e); from: stf.f_type
tst-statvfs.c: free: 53658025, mandatory: no, tp=9123683e
tst-statvfs.c:19: numeric comparison failure
   left: 2435016766 (0x9123683e); from: st.f_type
  right: -1859950530 (0x9123683e); from: stf.f_type
/tmp: free: 53658025, mandatory: no, tp=9123683e
error: 3 test failures

Thanks,
Florian

Re: [PATCH v3 3/3] statvfs: f_type: NEWS & test

[Adhemerval Zanella Netto]

On 15/08/23 06:15, Florian Weimer wrote:
> * наб:
> 
>> diff --git a/io/tst-statvfs.c b/io/tst-statvfs.c
>> index 227c62d7da..b38ecae466 100644
>> --- a/io/tst-statvfs.c
>> +++ b/io/tst-statvfs.c
>> @@ -1,5 +1,7 @@
>>  #include <stdio.h>
>> +#include <sys/statfs.h>
>>  #include <sys/statvfs.h>
>> +#include <support/check.h>
>>  
>>  
>>  /* This test cannot detect many errors.  But it will fail if the
>> @@ -11,17 +13,18 @@ do_test (int argc, char *argv[])
>>    for (int i = 1; i < argc; ++i)
>>      {
>>        struct statvfs st;
>> -      if (statvfs (argv[i], &st) != 0)
>> -        printf ("%s: failed (%m)\n", argv[i]);
>> -      else
>> -        printf ("%s: free: %llu, mandatory: %s\n", argv[i],
>> -                (unsigned long long int) st.f_bfree,
>> +      struct statfs stf;
>> +      TEST_COMPARE (statvfs (argv[i], &st), 0);
>> +      TEST_COMPARE (statfs (argv[i], &stf), 0);
>> +      TEST_COMPARE (st.f_type, stf.f_type);
> 
> This fails with certain file systems because the types of f_type differ
> in signedness:
> 
> =====FAIL: io/tst-statvfs.out=====
> tst-statvfs.c:19: numeric comparison failure
>    left: 2435016766 (0x9123683e); from: st.f_type
>   right: -1859950530 (0x9123683e); from: stf.f_type
> /builddir/build/BUILD/glibc-2.38.9000-40-gd6fe19facc/build-i686-redhat-linux/io/
> tst-statvfs: free: 53658025, mandatory: no, tp=9123683e
> tst-statvfs.c:19: numeric comparison failure
>    left: 2435016766 (0x9123683e); from: st.f_type
>   right: -1859950530 (0x9123683e); from: stf.f_type
> tst-statvfs.c: free: 53658025, mandatory: no, tp=9123683e
> tst-statvfs.c:19: numeric comparison failure
>    left: 2435016766 (0x9123683e); from: st.f_type
>   right: -1859950530 (0x9123683e); from: stf.f_type
> /tmp: free: 53658025, mandatory: no, tp=9123683e
> error: 3 test failures

Maybe add a TEST_COMPARE_NO_SIGN? The cast is always an option.

Re: [PATCH v3 3/3] statvfs: f_type: NEWS & test

[Florian Weimer]

* Adhemerval Zanella Netto:

>> This fails with certain file systems because the types of f_type differ
>> in signedness:
>> 
>> =====FAIL: io/tst-statvfs.out=====
>> tst-statvfs.c:19: numeric comparison failure
>>    left: 2435016766 (0x9123683e); from: st.f_type
>>   right: -1859950530 (0x9123683e); from: stf.f_type
>> /builddir/build/BUILD/glibc-2.38.9000-40-gd6fe19facc/build-i686-redhat-linux/io/
>> tst-statvfs: free: 53658025, mandatory: no, tp=9123683e
>> tst-statvfs.c:19: numeric comparison failure
>>    left: 2435016766 (0x9123683e); from: st.f_type
>>   right: -1859950530 (0x9123683e); from: stf.f_type
>> tst-statvfs.c: free: 53658025, mandatory: no, tp=9123683e
>> tst-statvfs.c:19: numeric comparison failure
>>    left: 2435016766 (0x9123683e); from: st.f_type
>>   right: -1859950530 (0x9123683e); from: stf.f_type
>> /tmp: free: 53658025, mandatory: no, tp=9123683e
>> error: 3 test failures
>
> Maybe add a TEST_COMPARE_NO_SIGN? The cast is always an option.

Or change the type of the new field to match the old field?

Due to the way TEST_COMPARE works, a no-sign option does not make much
sense, I'm afraid.  It's supposed to compare the mathematical values
regardless of type sizes.  If we want to accept certain signed vs
unsigned mismatches as valid, I think we need to use a common size, at
which point we might as well use a cast.

Conceptually, this should be close to what we want:

  if (st.f_type != stf.f_type)
    TEST_COMPARE (st.f_type, stf.f_type);

Except that I expect it to produce a compiler warning about signedness
mismatch.  So yes, I think we're going to have to add a cast.

Thanks,
Florian

Re: [PATCH v3 3/3] statvfs: f_type: NEWS & test

[Adhemerval Zanella Netto]

On 15/08/23 09:41, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>>> This fails with certain file systems because the types of f_type differ
>>> in signedness:
>>>
>>> =====FAIL: io/tst-statvfs.out=====
>>> tst-statvfs.c:19: numeric comparison failure
>>>    left: 2435016766 (0x9123683e); from: st.f_type
>>>   right: -1859950530 (0x9123683e); from: stf.f_type
>>> /builddir/build/BUILD/glibc-2.38.9000-40-gd6fe19facc/build-i686-redhat-linux/io/
>>> tst-statvfs: free: 53658025, mandatory: no, tp=9123683e
>>> tst-statvfs.c:19: numeric comparison failure
>>>    left: 2435016766 (0x9123683e); from: st.f_type
>>>   right: -1859950530 (0x9123683e); from: stf.f_type
>>> tst-statvfs.c: free: 53658025, mandatory: no, tp=9123683e
>>> tst-statvfs.c:19: numeric comparison failure
>>>    left: 2435016766 (0x9123683e); from: st.f_type
>>>   right: -1859950530 (0x9123683e); from: stf.f_type
>>> /tmp: free: 53658025, mandatory: no, tp=9123683e
>>> error: 3 test failures
>>
>> Maybe add a TEST_COMPARE_NO_SIGN? The cast is always an option.
> 
> Or change the type of the new field to match the old field?

We discussed this briefly [1] on why Ahelenia decided to use an
unsigned type.  We still can change it, I don't have a strong
opinion here.

> 
> Due to the way TEST_COMPARE works, a no-sign option does not make much
> sense, I'm afraid.  It's supposed to compare the mathematical values
> regardless of type sizes.  If we want to accept certain signed vs
> unsigned mismatches as valid, I think we need to use a common size, at
> which point we might as well use a cast.
> 
> Conceptually, this should be close to what we want:
> 
>   if (st.f_type != stf.f_type)
>     TEST_COMPARE (st.f_type, stf.f_type);
> 
> Except that I expect it to produce a compiler warning about signedness
> mismatch.  So yes, I think we're going to have to add a cast.
> 
> Thanks,
> Florian
> 

[1] https://sourceware.org/pipermail/libc-alpha/2023-July/150308.html

Re: [PATCH v3 3/3] statvfs: f_type: NEWS & test

[наб]

[-- Attachment #1: Type: text/plain, Size: 3395 bytes --]

On Tue, Aug 15, 2023 at 02:41:56PM +0200, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
> >> This fails with certain file systems because the types of f_type differ
> >> in signedness:
> >> 
> >> =====FAIL: io/tst-statvfs.out=====
> >> tst-statvfs.c:19: numeric comparison failure
> >>    left: 2435016766 (0x9123683e); from: st.f_type
> >>   right: -1859950530 (0x9123683e); from: stf.f_type
> >> /builddir/build/BUILD/glibc-2.38.9000-40-gd6fe19facc/build-i686-redhat-linux/io/
> >> tst-statvfs: free: 53658025, mandatory: no, tp=9123683e
> >> tst-statvfs.c:19: numeric comparison failure
> >>    left: 2435016766 (0x9123683e); from: st.f_type
> >>   right: -1859950530 (0x9123683e); from: stf.f_type
> >> tst-statvfs.c: free: 53658025, mandatory: no, tp=9123683e
> >> tst-statvfs.c:19: numeric comparison failure
> >>    left: 2435016766 (0x9123683e); from: st.f_type
> >>   right: -1859950530 (0x9123683e); from: stf.f_type
> >> /tmp: free: 53658025, mandatory: no, tp=9123683e
> >> error: 3 test failures
> >
> > Maybe add a TEST_COMPARE_NO_SIGN? The cast is always an option.
> 
> Or change the type of the new field to match the old field?
See 92861d93cdad13834f4d8f39504b550a80ad8200 for why it's an u32
(in short: the values are all logically u32s, but defined as untyped
 0x123 literals by accident, C++ needs this to be u32 to allow
 switch/casing on *_MAGICs, the ABI for struct statfs
 uses an effectively-random (signed/unsigned 32/64-bit integer)
 unnameable type that depends on the architecture,
 so we provide a sane and unmisuable API to that).

To that last point ‒ that test failure doesn't happen on amd64,
even with btrfs like in your log, only on i686.

(I'd be shocked if current struct statfs{}.f_type users were all
 correct on all arches in the era of amd64 monoculture;
 I knew about this and still wrote it wrong!)

> Due to the way TEST_COMPARE works, a no-sign option does not make much
> sense, I'm afraid.  It's supposed to compare the mathematical values
> regardless of type sizes.  If we want to accept certain signed vs
> unsigned mismatches as valid, I think we need to use a common size, at
> which point we might as well use a cast.
Making the line "TEST_COMPARE (st.f_type, (unsigned int) stf.f_type);"
did fix it in an i686 chroot.

idk if it's easier for you to take a patch or just edit the file,
but in case it's the former, scissor-patch follows.

Best,
-- >8 --
Subject: [PATCH] io/tst-statvfs: fix statfs().f_type comparison test on some
 arches

On i686 f_type is an i32 so the test fails when that has the top bit set.

Explicitly cast to u32.

Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
---
 io/tst-statvfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io/tst-statvfs.c b/io/tst-statvfs.c
index f3097ce1a8..fb23733667 100644
--- a/io/tst-statvfs.c
+++ b/io/tst-statvfs.c
@@ -16,7 +16,7 @@ do_test (int argc, char *argv[])
       struct statfs stf;
       TEST_COMPARE (statvfs (argv[i], &st), 0);
       TEST_COMPARE (statfs (argv[i], &stf), 0);
-      TEST_COMPARE (st.f_type, stf.f_type);
+      TEST_COMPARE (st.f_type, (unsigned int) stf.f_type);
       printf ("%s: free: %llu, mandatory: %s, tp=%x\n", argv[i],
               (unsigned long long int) st.f_bfree,
 #ifdef ST_MANDLOCK
-- 
2.39.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/3] statvfs: f_type: NEWS & test

[Florian Weimer]

* наб:

> Subject: [PATCH] io/tst-statvfs: fix statfs().f_type comparison test on some
>  arches
>
> On i686 f_type is an i32 so the test fails when that has the top bit set.
>
> Explicitly cast to u32.
>
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
> ---
>  io/tst-statvfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/io/tst-statvfs.c b/io/tst-statvfs.c
> index f3097ce1a8..fb23733667 100644
> --- a/io/tst-statvfs.c
> +++ b/io/tst-statvfs.c
> @@ -16,7 +16,7 @@ do_test (int argc, char *argv[])
>        struct statfs stf;
>        TEST_COMPARE (statvfs (argv[i], &st), 0);
>        TEST_COMPARE (statfs (argv[i], &stf), 0);
> -      TEST_COMPARE (st.f_type, stf.f_type);
> +      TEST_COMPARE (st.f_type, (unsigned int) stf.f_type);
>        printf ("%s: free: %llu, mandatory: %s, tp=%x\n", argv[i],
>                (unsigned long long int) st.f_bfree,
>  #ifdef ST_MANDLOCK

Looks good.

Reviewed-by: Florian Weimer <fweimer@redhat.com>

Can you install this yourself, or should I push it?

Thanks,
Florian

Re: [PATCH v3 3/3] statvfs: f_type: NEWS & test

[наб]

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

On Tue, Aug 15, 2023 at 04:23:37PM +0200, Florian Weimer wrote:
> * наб:
> > Subject: [PATCH] io/tst-statvfs: fix statfs().f_type comparison test on some
> >  arches
> Looks good.
> 
> Reviewed-by: Florian Weimer <fweimer@redhat.com>
> 
> Can you install this yourself,
Don't know what this means, sorry;
I don't think I have write access to the glibc repo?

> or should I push it?
Please do.

Best,

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 3/3] statvfs: f_type: NEWS & test

[Florian Weimer]

* наб:

> On Tue, Aug 15, 2023 at 04:23:37PM +0200, Florian Weimer wrote:
>> * наб:
>> > Subject: [PATCH] io/tst-statvfs: fix statfs().f_type comparison test on some
>> >  arches
>> Looks good.
>> 
>> Reviewed-by: Florian Weimer <fweimer@redhat.com>
>> 
>> Can you install this yourself,
> Don't know what this means, sorry;
> I don't think I have write access to the glibc repo?

That's what I meant.

>> or should I push it?
> Please do.

Pushed it.

Thanks,
Florian