Re: [PATCH] Linux: Deprecate <sys/sysctl.h> and sysctl

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 07/06/2019 09:01, Florian Weimer wrote:
> Now that there are no internal users of __sysctl left, it is possible
> to add an unconditional deprecation warning to <sys/sysctl.h>.
> 
> To avoid a test failure due this warning in check-install-headers,
> skip the test for sys/sysctl.h.

LGTM.  As a side note, if I recall correctly one usage of sysctl that might
be useful and not really available depending of the kernel version is to
get some random bits from /proc/sys/kernel/random/uuid without opening /proc. 

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

> 
> 2019-06-07  Florian Weimer  <fweimer@redhat.com>
> 
> 	Linux: Deprecate sysctl.
> 	* include/sysctl.h (__sysctl): Remove declaration.
> 	* scripts/check-installed-headers.sh (sys/sysctl.h): Disable
> 	check.
> 	* sysdeps/unix/sysv/linux/sys/sysctl.h: Add deprecation warning.
> 	(sysctl): Add deprecation attribute.
> 	* sysdeps/unix/sysv/linux/sysctl.c: Include <linux/sysctl.h>
> 	directly, to avoid the deprecation warning.  Do not include
> 	<string.h>.
> 	(__sysctl): Remove hidden alias.

Ok.

> 
> diff --git a/NEWS b/NEWS
> index 00f9e855a2..fffff79576 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -61,6 +61,9 @@ Deprecated and removed features, and other changes affecting compatibility:
>  * On 32-bit Arm, support for the port-based I/O emulation and the <sys/io.h>
>    header have been removed.
>  
> +* The Linux-specific <sys/sysctl.h> header and the sysctl function have been
> +  deprecated and will be removed from a future version of glibc.
> +
>  Changes to build and runtime requirements:
>  
>  * GCC 6.2 or later is required to build the GNU C Library.

Ok.

> diff --git a/include/sys/sysctl.h b/include/sys/sysctl.h
> index 2a15e91354..fa102aa226 100644
> --- a/include/sys/sysctl.h
> +++ b/include/sys/sysctl.h
> @@ -1,13 +1,3 @@
>  #ifndef _SYS_SYSCTL_H
>  #include_next <sys/sysctl.h>
> -
> -# ifndef _ISOMAC
> -
> -/* Read or write system parameters (Linux, FreeBSD specific).  */
> -extern int __sysctl (int *__name, int __nlen, void *__oldval,
> -		     size_t *__oldlenp, void *__newval, size_t __newlen);
> -libc_hidden_proto (__sysctl)
> -
> -
> -# endif /* !_ISOMAC */
>  #endif  /* _SYS_SYSCTL_H */

Do we still need to internal file? Can't we just remove it?

> diff --git a/scripts/check-installed-headers.sh b/scripts/check-installed-headers.sh
> index e4f37d33f8..ef6ed94a2e 100644
> --- a/scripts/check-installed-headers.sh
> +++ b/scripts/check-installed-headers.sh
> @@ -53,7 +53,6 @@ trap "rm -f '$cih_test_c'" 0
>  
>  failed=0
>  is_x86_64=unknown
> -is_x32=unknown
>  for header in "$@"; do
>      # Skip various headers for which this test gets a false failure.
>      case "$header" in
> @@ -75,27 +74,10 @@ for header in "$@"; do
>          (finclude/*)
>              continue;;
>  
> -	# sys/sysctl.h is unsupported for x32.
> +	# sys/sysctl.h produces a deprecation warning and therefore
> +	# fails compilation with -Werror.
>  	(sys/sysctl.h)
> -            case "$is_x32" in
> -                (yes) continue;;
> -                (no)  ;;
> -                (unknown)
> -                    cat >"$cih_test_c" <<EOF
> -#if defined __x86_64__ && defined __ILP32__
> -# error "is x32"
> -#endif
> -EOF
> -                    if $cc_cmd -fsyntax-only "$cih_test_c" > /dev/null 2>&1
> -                    then
> -                        is_x32=no
> -                    else
> -                        is_x32=yes
> -                        continue
> -                    fi
> -                ;;
> -            esac
> -	    ;;
> +	    continue;;
>  
>          # sys/vm86.h is "unsupported on x86-64" and errors out on that target.
>          (sys/vm86.h)
> diff --git a/sysdeps/unix/sysv/linux/sys/sysctl.h b/sysdeps/unix/sysv/linux/sys/sysctl.h
> index 0f6e71ba7e..15ee9642ab 100644
> --- a/sysdeps/unix/sysv/linux/sys/sysctl.h
> +++ b/sysdeps/unix/sysv/linux/sys/sysctl.h
> @@ -18,6 +18,9 @@
>  #ifndef	_SYS_SYSCTL_H
>  #define	_SYS_SYSCTL_H	1
>  
> +#warning "The <sys/sysctl.h> header is deprecated and will be removed."
> +#warning "Directly access the /proc file system instead."
> +
>  #include <features.h>
>  #define __need_size_t
>  #include <stddef.h>
> @@ -66,7 +69,8 @@ __BEGIN_DECLS
>  
>  /* Read or write system parameters.  */
>  extern int sysctl (int *__name, int __nlen, void *__oldval,
> -		   size_t *__oldlenp, void *__newval, size_t __newlen) __THROW;
> +		   size_t *__oldlenp, void *__newval, size_t __newlen) __THROW
> +  __attribute_deprecated__;
>  
>  __END_DECLS
>  

Ok.

> diff --git a/sysdeps/unix/sysv/linux/sysctl.c b/sysdeps/unix/sysv/linux/sysctl.c
> index 33afdd918b..0f18c69abe 100644
> --- a/sysdeps/unix/sysv/linux/sysctl.c
> +++ b/sysdeps/unix/sysv/linux/sysctl.c
> @@ -17,8 +17,7 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #include <errno.h>
> -#include <string.h>	/* For the real memset prototype.  */
> -#include <sys/sysctl.h>
> +#include <linux/sysctl.h>
>  
>  #include <sysdep.h>
>  #include <sys/syscall.h>
> @@ -39,5 +38,4 @@ __sysctl (int *name, int nlen, void *oldval, size_t *oldlenp,
>  
>    return INLINE_SYSCALL (_sysctl, 1, &args);
>  }
> -libc_hidden_def (__sysctl)
>  weak_alias (__sysctl, sysctl)
> 

What about a link warning stating this interface is deprecated and it would be
removed? My idea is to warn when people are static linking as well.