Re: [PATCH v4 1/4] Rename nptl-signals.h to internal-signals.h

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 20/02/2018 09:32, Florian Weimer wrote:
> On 02/12/2018 01:42 PM, Adhemerval Zanella wrote:
>> This patch renames the nptl-signals.h header to internal-signals.h.
>> On Linux the definitions and functions are not only NPTL related, but
>> used for other POSIX definitions as well (for instance SIGTIMER for
>> posix times, SIGSETXID for id functions, and signal block/restore
> 
> POSIX timers?
> 
>> Signed-off-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
> 
> We don't use DCO, but have copyright assignments.

Right, although I had the impression signed-off-by is mostly optional. I am
trying to find the follow-up thread about it after it was brought up in
Cauldron. Do you know if Carlos has written some wiki entry about it?

> 
>> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
>> diff --git a/sysdeps/unix/sysv/linux/nptl-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h
>> similarity index 89%
>> rename from sysdeps/unix/sysv/linux/nptl-signals.h
>> rename to sysdeps/unix/sysv/linux/internal-signals.h
>> index e789198..e007372 100644
>> --- a/sysdeps/unix/sysv/linux/nptl-signals.h
>> +++ b/sysdeps/unix/sysv/linux/internal-signals.h
>> @@ -1,4 +1,4 @@
>> -/* Special use of signals in NPTL internals.  Linux version.
>> +/* Special use of signals internally.  Linux version.
>>      Copyright (C) 2014-2018 Free Software Foundation, Inc.
>>      This file is part of the GNU C Library.
>>   @@ -16,6 +16,9 @@
>>      License along with the GNU C Library; if not, see
>>      <http://www.gnu.org/licenses/>.  */
>>   +#ifndef __INTERNAL_SIGNALS_H
>> +# define __INTERNAL_SIGNALS_H
>> +
>>   #include <signal.h>
>>   #include <sigsetops.h>
>>   @@ -35,17 +38,16 @@
>>     /* Return is sig is used internally.  */
>>   static inline int
>> -__nptl_is_internal_signal (int sig)
>> +__is_internal_signal (int sig)
>>   {
>> -  return (sig == SIGCANCEL) || (sig == SIGTIMER) || (sig == SIGSETXID);
>> +  return (sig == SIGCANCEL) || (sig == SIGSETXID);
> 
> Should this change be mentioned in the ChangeLog?  You could remove the unnecessary parens because you modify this line anyway.

I will add a entry in ChangeLog and remove the unnecessary parentheses. 

> 
>>   /* Remove internal glibc signal from the mask.  */
>>   static inline void
>> -__nptl_clear_internal_signals (sigset_t *set)
>> +__clear_internal_signals (sigset_t *set)
>>   {
>>     __sigdelset (set, SIGCANCEL);
>> -  __sigdelset (set, SIGTIMER);
> 
> Likewise, should be mentioned in the ChangeLog entry.

I will a entry as well.

> 
> Looks okay otherwise.
> 
> Thanks,
> Florian