Linux: Deprecate <sys/sysctl.h> and sysctl

Message ID 874l51hae1.fsf@oldenburg2.str.redhat.com
State New
Headers show
Series
  • Linux: Deprecate <sys/sysctl.h> and sysctl
Related show

Commit Message

Florian Weimer June 7, 2019, 12:01 p.m.
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.

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.

Comments

Adhemerval Zanella June 7, 2019, 2:17 p.m. | #1
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.
Florian Weimer June 7, 2019, 4:01 p.m. | #2
* Adhemerval Zanella:

> 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.


Right, I saw this quite a lot in existing code (although some people use
the FreeBSD MIB on Linux, which will always fail and never provide a UUID).

>> +#warning "The <sys/sysctl.h> header is deprecated and will be removed."

>> +#warning "Directly access the /proc file system instead."


Should I add this as well?

#warning "On current kernels, getentropy provides random bits without /proc."

>> 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.


They already get two warnings (from the header and the call site).  Do
we need a third warning?  It's only helpful if you don't use the header,
I think, but even with Go's cgo, you'll see the GCC warnings.

Thanks,
Florian
Adhemerval Zanella June 11, 2019, 1:15 p.m. | #3
On 07/06/2019 13:01, Florian Weimer wrote:
> * Adhemerval Zanella:

> 

>> 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.

> 

> Right, I saw this quite a lot in existing code (although some people use

> the FreeBSD MIB on Linux, which will always fail and never provide a UUID).

> 

>>> +#warning "The <sys/sysctl.h> header is deprecated and will be removed."

>>> +#warning "Directly access the /proc file system instead."

> 

> Should I add this as well?

> 

> #warning "On current kernels, getentropy provides random bits without /proc."


Not sure, I think a note on NEWS might be better fort this specific usage.

> 

>>> 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.

> 

> They already get two warnings (from the header and the call site).  Do

> we need a third warning?  It's only helpful if you don't use the header,

> I think, but even with Go's cgo, you'll see the GCC warnings.


Fair enough.

Patch LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
Florian Weimer June 12, 2019, 12:32 p.m. | #4
* Adhemerval Zanella:

> On 07/06/2019 13:01, Florian Weimer wrote:

>> * Adhemerval Zanella:

>> 

>>> 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.

>> 

>> Right, I saw this quite a lot in existing code (although some people use

>> the FreeBSD MIB on Linux, which will always fail and never provide a UUID).

>> 

>>>> +#warning "The <sys/sysctl.h> header is deprecated and will be removed."

>>>> +#warning "Directly access the /proc file system instead."

>> 

>> Should I add this as well?

>> 

>> #warning "On current kernels, getentropy provides random bits without /proc."

>

> Not sure, I think a note on NEWS might be better fort this specific

> usage.


Okay, I will remove the second #warning and add this to NEWS:

* The Linux-specific <sys/sysctl.h> header and the sysctl function have been
  deprecated and will be removed from a future version of glibc.
  Application should directly access /proc instead.  For obtaining random
  bits, the getentropy function can be used.

>>> 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.

>> 

>> They already get two warnings (from the header and the call site).  Do

>> we need a third warning?  It's only helpful if you don't use the header,

>> I think, but even with Go's cgo, you'll see the GCC warnings.

>

> Fair enough.

>

> Patch LGTM, thanks.

>

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


Thanks!

Florian

Patch

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.
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 */
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
 
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)