[v3,7/7] dirent: Deprecate getdirentries

Message ID 20201021141542.2003377-8-adhemerval.zanella@linaro.org
State Not applicable
Headers
Series Fix getdents{64} regression on some FS |

Commit Message

Adhemerval Zanella Oct. 21, 2020, 2:15 p.m. UTC
  The interface has some issues:

  1. It is build on top getdents on Linux and requires handling
     non-LFS call using LFS getdents.

  2. It is not wildly used and the non-LFS support is as problematic
     as non-LFS readdir.  glibc only exports the LFS getdents.

  3. It is not a direct replacement over BSD since on some plataform
     its signature has changed (FreeBSD 11, for instance, used to
     set the offset as a 'long' and changed to 'off_t' on version 12).

The idea is to eventually move the symbols to compat ones.
---
 NEWS                             |  3 +++
 dirent/dirent.h                  | 14 ++++++++++----
 sysdeps/unix/sysv/linux/Makefile |  3 +++
 3 files changed, 16 insertions(+), 4 deletions(-)
  

Comments

John Paul Adrian Glaubitz Jan. 6, 2022, 9:48 p.m. UTC | #1
Hi Adhemerval!

On 10/21/20 16:15, Adhemerval Zanella wrote:
> The interface has some issues:
> 
>   1. It is build on top getdents on Linux and requires handling
>      non-LFS call using LFS getdents.
> 
>   2. It is not wildly used and the non-LFS support is as problematic
>      as non-LFS readdir.  glibc only exports the LFS getdents.
> 
>   3. It is not a direct replacement over BSD since on some plataform
>      its signature has changed (FreeBSD 11, for instance, used to
>      set the offset as a 'long' and changed to 'off_t' on version 12).
> 
> The idea is to eventually move the symbols to compat ones.
> ---
>  NEWS                             |  3 +++
>  dirent/dirent.h                  | 14 ++++++++++----
>  sysdeps/unix/sysv/linux/Makefile |  3 +++
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index 3c1e509744..63623dcee2 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -47,6 +47,9 @@ Deprecated and removed features, and other changes affecting compatibility:
>    as a compatibility symbol.  Applications should use the getrlimit or
>    prlimit.
>  
> +* The function getdirentries is now deprecated, applications should use
> +  either getdents64, readdir64 or readdir.
> +
>  Changes to build and runtime requirements:
>  
>  * On Linux, the system administrator needs to configure /dev/pts with
> diff --git a/dirent/dirent.h b/dirent/dirent.h
> index 92d0925047..62bb77b4e1 100644
> --- a/dirent/dirent.h
> +++ b/dirent/dirent.h
> @@ -348,29 +348,35 @@ extern int alphasort64 (const struct dirent64 **__e1,
>  /* Read directory entries from FD into BUF, reading at most NBYTES.
>     Reading starts at offset *BASEP, and *BASEP is updated with the new
>     position after reading.  Returns the number of bytes read; zero when at
> -   end of directory; or -1 for errors.  */
> +   end of directory; or -1 for errors.
> +   This is deprecated and getdents64 or readdir should be used instead.  */
>  # ifndef __USE_FILE_OFFSET64
>  extern __ssize_t getdirentries (int __fd, char *__restrict __buf,
>  				size_t __nbytes,
>  				__off_t *__restrict __basep)
> -     __THROW __nonnull ((2, 4));
> +     __THROW __nonnull ((2, 4))
> +     __attribute_deprecated_msg__ ("Use getdents64 instead");
>  # else
>  #  ifdef __REDIRECT
>  extern __ssize_t __REDIRECT_NTH (getdirentries,
>  				 (int __fd, char *__restrict __buf,
>  				  size_t __nbytes,
>  				  __off64_t *__restrict __basep),
> -				 getdirentries64) __nonnull ((2, 4));
> +				 getdirentries64)
> +     __THROW __nonnull ((2, 4))
> +     __attribute_deprecated_msg__ ("Use getdents64 instead");
>  #  else
>  #   define getdirentries getdirentries64
>  #  endif
>  # endif
>  
>  # ifdef __USE_LARGEFILE64
> +/* This is deprecated and getdents64 or readdir64 should be used instead.  */
>  extern __ssize_t getdirentries64 (int __fd, char *__restrict __buf,
>  				  size_t __nbytes,
>  				  __off64_t *__restrict __basep)
> -     __THROW __nonnull ((2, 4));
> +     __THROW __nonnull ((2, 4))
> +     __attribute_deprecated_msg__ ("Use getdents64 instead");
>  # endif
>  #endif /* Use misc.  */
>  
> diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
> index 09604e128b..242b07734f 100644
> --- a/sysdeps/unix/sysv/linux/Makefile
> +++ b/sysdeps/unix/sysv/linux/Makefile
> @@ -262,6 +262,9 @@ ifeq ($(subdir),dirent)
>  sysdep_routines += getdirentries getdirentries64
>  tests += tst-getdents64
>  tests-internal += tst-readdir64-compat
> +
> +# Avoid the warning for the weak_alias for _DIRENT_MATCHES_DIRENT64
> +CFLAGS-getdirentries64.c = -Wno-deprecated-declarations
>  endif
>  
>  ifeq ($(subdir),nis)

This particular patch seems to break the apt build:

FAILED: apt-pkg/CMakeFiles/apt-pkg.dir/acquire.cc.o 
/usr/bin/c++ -DAPT_DOMAIN=\"libapt-pkg6.0\" -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_WITH_GETLINE=1 -Dapt_pkg_EXPORTS -I/<<PKGBUILDDIR>>/obj-m68k-linux-gnu/include -I/<<PKGBUILDDIR>>/obj-m68k-linux-gnu/include/apt-pkg -g -O2 -ffile-prefix-map=/<<PKGBUILDDIR>>=. -specs=/usr/share/dpkg/pie-compile.specs -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -Wall -Wextra -Wcast-align -Wlogical-op -Wredundant-decls -Wmissing-declarations -Wunsafe-loop-optimizations -Wctor-dtor-privacy -Wdisabled-optimization -Winit-self -Wmissing-include-dirs -Wnoexcept -Wsign-promo -Wundef -Wdouble-promotion -Wsuggest-override -Werror=suggest-override -Werror=return-type -std=gnu++17 -MD -MT apt-pkg/CMakeFiles/apt-pkg.dir/acquire.cc.o -MF apt-pkg/CMakeFiles/apt-pkg.dir/acquire.cc.o.d -o apt-pkg/CMakeFiles/apt-pkg.dir/acquire.cc.o -c /<<PKGBUILDDIR>>/apt-pkg/acquire.cc
In file included from /usr/include/features.h:472,
                 from /usr/include/m68k-linux-gnu/c++/11/bits/os_defines.h:39,
                 from /usr/include/m68k-linux-gnu/c++/11/bits/c++config.h:586,
                 from /usr/include/c++/11/cstring:41,
                 from /<<PKGBUILDDIR>>/obj-m68k-linux-gnu/include/apt-pkg/hashes.h:17,
                 from /<<PKGBUILDDIR>>/obj-m68k-linux-gnu/include/apt-pkg/acquire.h:68,
                 from /<<PKGBUILDDIR>>/obj-m68k-linux-gnu/include/apt-pkg/acquire-item.h:22,
                 from /<<PKGBUILDDIR>>/apt-pkg/acquire.cc:17:
/usr/include/dirent.h:366:6: error: expected initializer before ‘noexcept’
  366 |      __THROW __nonnull ((2, 4))
      |      ^~~~~~~

See the full build log here:

> https://buildd.debian.org/status/fetch.php?pkg=apt&arch=m68k&ver=2.3.14&stamp=1641503494&raw=0

I have built and uploaded a custom version of the glibc package which includes your patch
so that the m68k and sh4 buildds based on qemu-user will not be affected by BZ #23960.

Do you have any suggestion how to fix this issue?

Thanks,
Adrian
  
Sam James Jan. 6, 2022, 9:59 p.m. UTC | #2
> On 6 Jan 2022, at 21:48, John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote:
> 
> Hi Adhemerval!
> 
> [snip]
> 
> FAILED: apt-pkg/CMakeFiles/apt-pkg.dir/acquire.cc.o
> /usr/bin/c++ -DAPT_DOMAIN=\"libapt-pkg6.0\" -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_WITH_GETLINE=1 -Dapt_pkg_EXPORTS -I/<<PKGBUILDDIR>>/obj-m68k-linux-gnu/include -I/<<PKGBUILDDIR>>/obj-m68k-linux-gnu/include/apt-pkg -g -O2 -ffile-prefix-map=/<<PKGBUILDDIR>>=. -specs=/usr/share/dpkg/pie-compile.specs -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2 -fPIC -fvisibility=hidden -fvisibility-inlines-hidden -Wall -Wextra -Wcast-align -Wlogical-op -Wredundant-decls -Wmissing-declarations -Wunsafe-loop-optimizations -Wctor-dtor-privacy -Wdisabled-optimization -Winit-self -Wmissing-include-dirs -Wnoexcept -Wsign-promo -Wundef -Wdouble-promotion -Wsuggest-override -Werror=suggest-override -Werror=return-type -std=gnu++17 -MD -MT apt-pkg/CMakeFiles/apt-pkg.dir/acquire.cc.o -MF apt-pkg/CMakeFiles/apt-pkg.dir/acquire.cc.o.d -o apt-pkg/CMakeFiles/apt-pkg.dir/acquire.cc.o -c /<<PKGBUILDDIR>>/apt-pkg/acquire.cc
> In file included from /usr/include/features.h:472,
>                 from /usr/include/m68k-linux-gnu/c++/11/bits/os_defines.h:39,
>                 from /usr/include/m68k-linux-gnu/c++/11/bits/c++config.h:586,
>                 from /usr/include/c++/11/cstring:41,
>                 from /<<PKGBUILDDIR>>/obj-m68k-linux-gnu/include/apt-pkg/hashes.h:17,
>                 from /<<PKGBUILDDIR>>/obj-m68k-linux-gnu/include/apt-pkg/acquire.h:68,
>                 from /<<PKGBUILDDIR>>/obj-m68k-linux-gnu/include/apt-pkg/acquire-item.h:22,
>                 from /<<PKGBUILDDIR>>/apt-pkg/acquire.cc:17:
> /usr/include/dirent.h:366:6: error: expected initializer before ‘noexcept’
>  366 |      __THROW __nonnull ((2, 4))
>      |      ^~~~~~~
> 
> See the full build log here:
> 
>> https://buildd.debian.org/status/fetch.php?pkg=apt&arch=m68k&ver=2.3.14&stamp=1641503494&raw=0
> 
> I have built and uploaded a custom version of the glibc package which includes your patch
> so that the m68k and sh4 buildds based on qemu-user will not be affected by BZ #23960.
> 
> Do you have any suggestion how to fix this issue?

Try this version of the last commit: https://github.com/zatrazz/glibc/commit/7b990b41ea23c0464c60e6a9f0707adfb46ae587 <https://github.com/zatrazz/glibc/commit/7b990b41ea23c0464c60e6a9f0707adfb46ae587>.

We had the same problem in Gentoo and azanella pushed a newer version.

Best,
sam
  
John Paul Adrian Glaubitz Jan. 6, 2022, 10:01 p.m. UTC | #3
Hi Sam!

On 1/6/22 22:59, Sam James wrote:
>> Do you have any suggestion how to fix this issue?
> 
> Try this version of the last commit: https://github.com/zatrazz/glibc/commit/7b990b41ea23c0464c60e6a9f0707adfb46ae587 <https://github.com/zatrazz/glibc/commit/7b990b41ea23c0464c60e6a9f0707adfb46ae587>.
> 
> We had the same problem in Gentoo and azanella pushed a newer version.

Perfect, thank you! I will give it a try.

Adrian
  
John Paul Adrian Glaubitz May 10, 2022, 7:30 p.m. UTC | #4
Hi!

On 1/6/22 22:59, Sam James wrote:
> Try this version of the last commit: https://github.com/zatrazz/glibc/commit/7b990b41ea23c0464c60e6a9f0707adfb46ae587 <https://github.com/zatrazz/glibc/commit/7b990b41ea23c0464c60e6a9f0707adfb46ae587>.
> 
> We had the same problem in Gentoo and azanella pushed a newer version.

The original problem seems to have returned. I can no longer build packages such as
boost1.74, openjdk-18 or globus-net-manager:

> https://buildd.debian.org/status/fetch.php?pkg=boost1.74&arch=m68k&ver=1.74.0-14&stamp=1651053703&raw=0
> https://buildd.debian.org/status/fetch.php?pkg=globus-net-manager&arch=m68k&ver=1.6-1%2Bb1&stamp=1651419562&raw=0
> https://buildd.debian.org/status/fetch.php?pkg=openjdk-18&arch=m68k&ver=18.0.1%2B10-1&stamp=1651537618&raw=0

I have not yet figured out what the problem is.

Adrian
  
Adhemerval Zanella May 12, 2022, 3:40 p.m. UTC | #5
On 10/05/2022 16:30, John Paul Adrian Glaubitz wrote:
> Hi!
> 
> On 1/6/22 22:59, Sam James wrote:
>> Try this version of the last commit: https://github.com/zatrazz/glibc/commit/7b990b41ea23c0464c60e6a9f0707adfb46ae587 <https://github.com/zatrazz/glibc/commit/7b990b41ea23c0464c60e6a9f0707adfb46ae587>.
>>
>> We had the same problem in Gentoo and azanella pushed a newer version.
> 
> The original problem seems to have returned. I can no longer build packages such as
> boost1.74, openjdk-18 or globus-net-manager:
> 
>> https://buildd.debian.org/status/fetch.php?pkg=boost1.74&arch=m68k&ver=1.74.0-14&stamp=1651053703&raw=0
>> https://buildd.debian.org/status/fetch.php?pkg=globus-net-manager&arch=m68k&ver=1.6-1%2Bb1&stamp=1651419562&raw=0
>> https://buildd.debian.org/status/fetch.php?pkg=openjdk-18&arch=m68k&ver=18.0.1%2B10-1&stamp=1651537618&raw=0
> 
> I have not yet figured out what the problem is.

I can respin my patchset to fix the dirent on non-LFS, not sure if someone
will be interested to review it.
  

Patch

diff --git a/NEWS b/NEWS
index 3c1e509744..63623dcee2 100644
--- a/NEWS
+++ b/NEWS
@@ -47,6 +47,9 @@  Deprecated and removed features, and other changes affecting compatibility:
   as a compatibility symbol.  Applications should use the getrlimit or
   prlimit.
 
+* The function getdirentries is now deprecated, applications should use
+  either getdents64, readdir64 or readdir.
+
 Changes to build and runtime requirements:
 
 * On Linux, the system administrator needs to configure /dev/pts with
diff --git a/dirent/dirent.h b/dirent/dirent.h
index 92d0925047..62bb77b4e1 100644
--- a/dirent/dirent.h
+++ b/dirent/dirent.h
@@ -348,29 +348,35 @@  extern int alphasort64 (const struct dirent64 **__e1,
 /* Read directory entries from FD into BUF, reading at most NBYTES.
    Reading starts at offset *BASEP, and *BASEP is updated with the new
    position after reading.  Returns the number of bytes read; zero when at
-   end of directory; or -1 for errors.  */
+   end of directory; or -1 for errors.
+   This is deprecated and getdents64 or readdir should be used instead.  */
 # ifndef __USE_FILE_OFFSET64
 extern __ssize_t getdirentries (int __fd, char *__restrict __buf,
 				size_t __nbytes,
 				__off_t *__restrict __basep)
-     __THROW __nonnull ((2, 4));
+     __THROW __nonnull ((2, 4))
+     __attribute_deprecated_msg__ ("Use getdents64 instead");
 # else
 #  ifdef __REDIRECT
 extern __ssize_t __REDIRECT_NTH (getdirentries,
 				 (int __fd, char *__restrict __buf,
 				  size_t __nbytes,
 				  __off64_t *__restrict __basep),
-				 getdirentries64) __nonnull ((2, 4));
+				 getdirentries64)
+     __THROW __nonnull ((2, 4))
+     __attribute_deprecated_msg__ ("Use getdents64 instead");
 #  else
 #   define getdirentries getdirentries64
 #  endif
 # endif
 
 # ifdef __USE_LARGEFILE64
+/* This is deprecated and getdents64 or readdir64 should be used instead.  */
 extern __ssize_t getdirentries64 (int __fd, char *__restrict __buf,
 				  size_t __nbytes,
 				  __off64_t *__restrict __basep)
-     __THROW __nonnull ((2, 4));
+     __THROW __nonnull ((2, 4))
+     __attribute_deprecated_msg__ ("Use getdents64 instead");
 # endif
 #endif /* Use misc.  */
 
diff --git a/sysdeps/unix/sysv/linux/Makefile b/sysdeps/unix/sysv/linux/Makefile
index 09604e128b..242b07734f 100644
--- a/sysdeps/unix/sysv/linux/Makefile
+++ b/sysdeps/unix/sysv/linux/Makefile
@@ -262,6 +262,9 @@  ifeq ($(subdir),dirent)
 sysdep_routines += getdirentries getdirentries64
 tests += tst-getdents64
 tests-internal += tst-readdir64-compat
+
+# Avoid the warning for the weak_alias for _DIRENT_MATCHES_DIRENT64
+CFLAGS-getdirentries64.c = -Wno-deprecated-declarations
 endif
 
 ifeq ($(subdir),nis)