[COMMITTED] Make sysdeps/generic/internal-signals.h less stubby.

Message ID 20180404164518.11147-1-zackw@panix.com
State New
Headers show
Series
  • [COMMITTED] Make sysdeps/generic/internal-signals.h less stubby.
Related show

Commit Message

Zack Weinberg April 4, 2018, 4:45 p.m.
Three of the functions defined by internal-signals.h were not actually
fulfilling their contracts when the sysdeps/generic version of that
file was used.  Also, the Linux version included several more headers
than the generic version, which is the root cause of a build failure
on Hurd (already addressed in another way, but I think it is proper to
make the headers match).

In principle, __libc_signal_block_all, __libc_signal_block_app, and
__libc_signal_restore_set could be defined in non-system-dependent
code, leaving only __is_internal_signal and __clear_internal_signal to
be defined by a sysdeps header, but the Linux version of
internal-signals.h wants to inline a raw rt_sigprocmask syscall, and I
don't feel like messing with that today.

Tested with cross build to i686-gnu, committed.

	* sysdeps/generic/internal-signals.h: Include signal.h,
	sigsetops.h, and stdbool.h.
	(__libc_signal_block_all): Actually block all signals.
	(__libc_signal_block_app): Likewise.
	(__libc_signal_restore_set): Actually restore the signal mask.
---
 ChangeLog                          |  8 ++++++++
 sysdeps/generic/internal-signals.h | 15 ++++++++++++---
 2 files changed, 20 insertions(+), 3 deletions(-)

-- 
2.16.3

Comments

Adhemerval Zanella April 4, 2018, 5:42 p.m. | #1
On 04/04/2018 13:45, Zack Weinberg wrote:
> Three of the functions defined by internal-signals.h were not actually

> fulfilling their contracts when the sysdeps/generic version of that

> file was used.  Also, the Linux version included several more headers

> than the generic version, which is the root cause of a build failure

> on Hurd (already addressed in another way, but I think it is proper to

> make the headers match).

> 

> In principle, __libc_signal_block_all, __libc_signal_block_app, and

> __libc_signal_restore_set could be defined in non-system-dependent

> code, leaving only __is_internal_signal and __clear_internal_signal to

> be defined by a sysdeps header, but the Linux version of

> internal-signals.h wants to inline a raw rt_sigprocmask syscall, and I

> don't feel like messing with that today.


Thanks for catching it.  If I remember correctly, I used an inlined 
rt_sigprocmask for Linux because both sigprocmask and pthread_sigmask 
filter both SIGCANCEL and SIGSETXID. We could use it for 
__libc_signal_block_app, but __libc_signal_block_all and 
__libc_signal_restore_set would require a new symbol.

> 

> Tested with cross build to i686-gnu, committed.

> 

> 	* sysdeps/generic/internal-signals.h: Include signal.h,

> 	sigsetops.h, and stdbool.h.

> 	(__libc_signal_block_all): Actually block all signals.

> 	(__libc_signal_block_app): Likewise.

> 	(__libc_signal_restore_set): Actually restore the signal mask.

> ---

>  ChangeLog                          |  8 ++++++++

>  sysdeps/generic/internal-signals.h | 15 ++++++++++++---

>  2 files changed, 20 insertions(+), 3 deletions(-)

> 

> diff --git a/ChangeLog b/ChangeLog

> index 663e80c195..43e0d6b95f 100644

> --- a/ChangeLog

> +++ b/ChangeLog

> @@ -1,3 +1,11 @@

> +2018-04-04  Zack Weinberg  <zackw@panix.com>

> +

> +	* sysdeps/generic/internal-signals.h: Include signal.h,

> +	sigsetops.h, and stdbool.h.

> +	(__libc_signal_block_all): Actually block all signals.

> +	(__libc_signal_block_app): Likewise.

> +	(__libc_signal_restore_set): Actually restore the signal mask.

> +

>  2018-04-04  Florian Weimer  <fweimer@redhat.com>

>  

>  	inet: Actually build and run tst-deadline.

> diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h

> index 17ed48c5bf..5282cffce0 100644

> --- a/sysdeps/generic/internal-signals.h

> +++ b/sysdeps/generic/internal-signals.h

> @@ -19,6 +19,10 @@

>  #ifndef __INTERNAL_SIGNALS_H

>  # define __INTERNAL_SIGNALS_H

>  

> +#include <signal.h>

> +#include <sigsetops.h>

> +#include <stdbool.h>

> +

>  static inline bool

>  __is_internal_signal (int sig)

>  {

> @@ -33,20 +37,25 @@ __clear_internal_signals (sigset_t *set)

>  static inline int

>  __libc_signal_block_all (sigset_t *set)

>  {

> -  return 0;

> +  sigset_t allset;

> +  __sigfillset (&allset);

> +  return __sigprocmask (SIG_BLOCK, &allset, set);

>  }

>  

>  static inline int

>  __libc_signal_block_app (sigset_t *set)

>  {

> -  return 0;

> +  sigset_t allset;

> +  __sigfillset (&allset);

> +  __clear_internal_signals (&allset);

> +  return __sigprocmask (SIG_BLOCK, &allset, set);

>  }

>  

>  /* Restore current process signal mask.  */

>  static inline int

>  __libc_signal_restore_set (const sigset_t *set)

>  {

> -  return 0;

> +  return __sigprocmask (SIG_SETMASK, set, NULL);

>  }

>  

>  

>
Zack Weinberg April 4, 2018, 5:48 p.m. | #2
On Wed, Apr 4, 2018 at 1:42 PM, Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
> On 04/04/2018 13:45, Zack Weinberg wrote:

>> Three of the functions defined by internal-signals.h were not actually

>> fulfilling their contracts when the sysdeps/generic version of that

>> file was used.  Also, the Linux version included several more headers

>> than the generic version, which is the root cause of a build failure

>> on Hurd (already addressed in another way, but I think it is proper to

>> make the headers match).

>>

>> In principle, __libc_signal_block_all, __libc_signal_block_app, and

>> __libc_signal_restore_set could be defined in non-system-dependent

>> code, leaving only __is_internal_signal and __clear_internal_signal to

>> be defined by a sysdeps header, but the Linux version of

>> internal-signals.h wants to inline a raw rt_sigprocmask syscall, and I

>> don't feel like messing with that today.

>

> Thanks for catching it.  If I remember correctly, I used an inlined

> rt_sigprocmask for Linux because both sigprocmask and pthread_sigmask

> filter both SIGCANCEL and SIGSETXID. We could use it for

> __libc_signal_block_app, but __libc_signal_block_all and

> __libc_signal_restore_set would require a new symbol.


Yeah, that's what I thought was going on.  I think it's fine the way
it is at least for now.

zw

Patch

diff --git a/ChangeLog b/ChangeLog
index 663e80c195..43e0d6b95f 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@ 
+2018-04-04  Zack Weinberg  <zackw@panix.com>
+
+	* sysdeps/generic/internal-signals.h: Include signal.h,
+	sigsetops.h, and stdbool.h.
+	(__libc_signal_block_all): Actually block all signals.
+	(__libc_signal_block_app): Likewise.
+	(__libc_signal_restore_set): Actually restore the signal mask.
+
 2018-04-04  Florian Weimer  <fweimer@redhat.com>
 
 	inet: Actually build and run tst-deadline.
diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h
index 17ed48c5bf..5282cffce0 100644
--- a/sysdeps/generic/internal-signals.h
+++ b/sysdeps/generic/internal-signals.h
@@ -19,6 +19,10 @@ 
 #ifndef __INTERNAL_SIGNALS_H
 # define __INTERNAL_SIGNALS_H
 
+#include <signal.h>
+#include <sigsetops.h>
+#include <stdbool.h>
+
 static inline bool
 __is_internal_signal (int sig)
 {
@@ -33,20 +37,25 @@  __clear_internal_signals (sigset_t *set)
 static inline int
 __libc_signal_block_all (sigset_t *set)
 {
-  return 0;
+  sigset_t allset;
+  __sigfillset (&allset);
+  return __sigprocmask (SIG_BLOCK, &allset, set);
 }
 
 static inline int
 __libc_signal_block_app (sigset_t *set)
 {
-  return 0;
+  sigset_t allset;
+  __sigfillset (&allset);
+  __clear_internal_signals (&allset);
+  return __sigprocmask (SIG_BLOCK, &allset, set);
 }
 
 /* Restore current process signal mask.  */
 static inline int
 __libc_signal_restore_set (const sigset_t *set)
 {
-  return 0;
+  return __sigprocmask (SIG_SETMASK, set, NULL);
 }