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

[Adhemerval Zanella Netto]

On 23/07/23 12:35, наб 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 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.
> We don't lose any generality by using u32, and by doing so we both make
> the API consistent with the Hurd, and allow C++
>   switch(f_type) { case RAMFS_MAGIC: ...; }
> 
> Also fix tst-statvfs so that it actually fails;
> as it stood, all it did was return 0 always.
> Test statfs()' and statvfs()' f_types are the same.
> 
> Link: https://lore.kernel.org/linux-man/f54kudgblgk643u32tb6at4cd3kkzha6hslahv24szs4raroaz@ogivjbfdaqtb/t/#u
> Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>

LGTM for 2.39 (so you will probably need to resend once NEWS is adjusted).

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
> Clean rebase, squashed 3/3 into 2/2, added note that we need to use u32
> to allow switch/cases in C++, io/tst-statvfs now formats statvfs.f_type.
> 
>  NEWS                                       |  5 +++++
>  io/tst-statvfs.c                           | 19 +++++++++++--------
>  sysdeps/unix/sysv/linux/bits/statvfs.h     |  6 ++++--
>  sysdeps/unix/sysv/linux/internal_statvfs.c |  2 ++
>  4 files changed, 22 insertions(+), 10 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 93f7d9faaa..bf79f39065 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -54,6 +54,11 @@ Major new features:
>    explicitly enabled, then fortify source is forcibly disabled so to keep
>    original behavior unchanged.
>  
> +* 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:
>  
>  * libcrypt is no longer built by default, one may use the --enable-crypt
> diff --git a/io/tst-statvfs.c b/io/tst-statvfs.c
> index 227c62d7da..f3097ce1a8 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, tp=%x\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
> -                );
> +              st.f_type);
>      }
>    return 0;
>  }
> 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.  */