All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH] lapi/signal.h: rm references to _SC_SIGRT_* and __SIGRT*
@ 2022-04-25 13:38 Richard Palethorpe via ltp
  2022-04-26 12:20 ` Cyril Hrubis
  0 siblings, 1 reply; 3+ messages in thread
From: Richard Palethorpe via ltp @ 2022-04-25 13:38 UTC (permalink / raw)
  To: ltp; +Cc: Richard Palethorpe

The old API's tst_sig.c uses_SC_SIGRT_MIN/MAX are not extant and a
quick internet search does not reveal much. It's assumed they meant
the same thing as tst_sigrt{min,max}_kern.

The fallback using __SIGRT{MIN,MAX} is also replaced with
tst_signal_is_reserved_rt. It says sigrtmin on HPPA is 37. However in
the kernel all arches have it set to 32.

In all cases __SIGRT{MIN,MAX} is replaces with constants we define as
we can not use __ variables. Two functions are introduced to make the
meaning clear when using these constants to check if a signal is
reserved and/or realtime.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Suggested-by: Petr Vorel <pvorel@suse.cz>
Suggested-by: Cyril Hrubis <chrubis@suse.cz>
---
 include/lapi/signal.h                         | 36 ++++++-------
 lib/tst_sig.c                                 | 51 +------------------
 testcases/kernel/syscalls/ptrace/ptrace05.c   |  2 +-
 .../syscalls/rt_sigsuspend/rt_sigsuspend01.c  |  2 +-
 testcases/kernel/syscalls/sighold/sighold02.c |  4 +-
 5 files changed, 23 insertions(+), 72 deletions(-)

diff --git a/include/lapi/signal.h b/include/lapi/signal.h
index 6f4a76881..aa185a5a6 100644
--- a/include/lapi/signal.h
+++ b/include/lapi/signal.h
@@ -1,24 +1,24 @@
-// SPDX-License-Identifier: GPL-2.0-or-later
+/* SPDX-License-Identifier: GPL-2.0-or-later */
 /*
- * Copyright (c) 2019 Linaro Limited. All rights reserved.
- * Author: Daniel Díaz <daniel.diaz@linaro.org>
+ * Copyright (c) 2022 SUSE LLC
  */
-
-#ifndef LAPI_SIGNAL_H__
-#define LAPI_SIGNAL_H__
-
 #include <signal.h>
 
-/*
- * Some libc implementations might differ in the definitions they include. This
- * covers those differences for all tests to successfully build.
- */
+const int tst_sigrtmin_kern = 32;
+const int tst_sigrtmax_kern = 64;
 
-#ifndef __SIGRTMIN
-# define __SIGRTMIN 32
-#endif
-#ifndef __SIGRTMAX
-# define __SIGRTMAX (_NSIG - 1)
-#endif
+/* Indicates that libc has reserved a RT signal for use by the system
+ * libraries. Usually threading, but could be anything. */
+__attribute__((warn_unused_result))
+static inline int tst_signal_is_reserved_rt(const int sig)
+{
+	return 	sig >= tst_sigrtmin_kern && sig < SIGRTMIN;
+}
 
-#endif /* LAPI_SIGNAL_H__ */
+/* Indicates that a signal is in the kernel's realtime set. This
+ * includes signals reserved by libc. */
+__attribute__((const, warn_unused_result))
+static inline int tst_signal_is_kern_rt(const int sig)
+{
+	return 	sig >= tst_sigrtmin_kern && sig < tst_sigrtmax_kern;
+}
diff --git a/lib/tst_sig.c b/lib/tst_sig.c
index 6d77aeafd..2db3561f5 100644
--- a/lib/tst_sig.c
+++ b/lib/tst_sig.c
@@ -69,7 +69,6 @@
 
 #include <errno.h>
 #include <string.h>
-#include <signal.h>
 #include <unistd.h>
 #include "test.h"
 #include "lapi/signal.h"
@@ -93,9 +92,6 @@ static void (*tst_setup_signal(int, void (*)(int))) (int);
 void tst_sig(int fork_flag, void (*handler) (), void (*cleanup) ())
 {
 	int sig;
-#ifdef _SC_SIGRT_MIN
-	long sigrtmin, sigrtmax;
-#endif
 
 	/*
 	 * save T_cleanup and handler function pointers
@@ -106,10 +102,6 @@ void tst_sig(int fork_flag, void (*handler) (), void (*cleanup) ())
 		/* use default handler */
 		handler = def_handler;
 	}
-#ifdef _SC_SIGRT_MIN
-	sigrtmin = sysconf(_SC_SIGRT_MIN);
-	sigrtmax = sysconf(_SC_SIGRT_MAX);
-#endif
 
 	/*
 	 * now loop through all signals and set the handlers
@@ -123,54 +115,13 @@ void tst_sig(int fork_flag, void (*handler) (), void (*cleanup) ())
 		 * SIGINFO is used for file quotas and should be expected
 		 */
 
-#ifdef _SC_SIGRT_MIN
-		if (sig >= sigrtmin && sig <= sigrtmax)
+		if (tst_signal_is_kern_rt(sig))
 			continue;
-#endif
 
 		switch (sig) {
 		case SIGKILL:
 		case SIGSTOP:
 		case SIGCONT:
-#if !defined(_SC_SIGRT_MIN) && defined(__SIGRTMIN) && defined(__SIGRTMAX)
-			/* Ignore all real-time signals */
-		case __SIGRTMIN:
-		case __SIGRTMIN + 1:
-		case __SIGRTMIN + 2:
-		case __SIGRTMIN + 3:
-		case __SIGRTMIN + 4:
-		case __SIGRTMIN + 5:
-		case __SIGRTMIN + 6:
-		case __SIGRTMIN + 7:
-		case __SIGRTMIN + 8:
-		case __SIGRTMIN + 9:
-		case __SIGRTMIN + 10:
-		case __SIGRTMIN + 11:
-		case __SIGRTMIN + 12:
-		case __SIGRTMIN + 13:
-		case __SIGRTMIN + 14:
-		case __SIGRTMIN + 15:
-/* __SIGRTMIN is 37 on HPPA rather than 32 *
- * as on i386, etc.                        */
-#if !defined(__hppa__)
-		case __SIGRTMAX - 15:
-		case __SIGRTMAX - 14:
-		case __SIGRTMAX - 13:
-		case __SIGRTMAX - 12:
-		case __SIGRTMAX - 11:
-#endif
-		case __SIGRTMAX - 10:
-		case __SIGRTMAX - 9:
-		case __SIGRTMAX - 8:
-		case __SIGRTMAX - 7:
-		case __SIGRTMAX - 6:
-		case __SIGRTMAX - 5:
-		case __SIGRTMAX - 4:
-		case __SIGRTMAX - 3:
-		case __SIGRTMAX - 2:
-		case __SIGRTMAX - 1:
-		case __SIGRTMAX:
-#endif
 #ifdef SIGSWAP
 		case SIGSWAP:
 #endif /* SIGSWAP */
diff --git a/testcases/kernel/syscalls/ptrace/ptrace05.c b/testcases/kernel/syscalls/ptrace/ptrace05.c
index 54cfa4d7b..1c1911877 100644
--- a/testcases/kernel/syscalls/ptrace/ptrace05.c
+++ b/testcases/kernel/syscalls/ptrace/ptrace05.c
@@ -72,7 +72,7 @@ int main(int argc, char **argv)
 
 	for (signum = start_signum; signum <= end_signum; signum++) {
 
-		if (signum >= __SIGRTMIN && signum < SIGRTMIN)
+		if (tst_signal_is_kern_rt(signum))
 			continue;
 
 		switch (child = fork()) {
diff --git a/testcases/kernel/syscalls/rt_sigsuspend/rt_sigsuspend01.c b/testcases/kernel/syscalls/rt_sigsuspend/rt_sigsuspend01.c
index 511d5fcb2..5fbfad04e 100644
--- a/testcases/kernel/syscalls/rt_sigsuspend/rt_sigsuspend01.c
+++ b/testcases/kernel/syscalls/rt_sigsuspend/rt_sigsuspend01.c
@@ -50,7 +50,7 @@ static void verify_rt_sigsuspend(void)
 
 	SAFE_RT_SIGPROCMASK(0, NULL, &set2, SIGSETSIZE);
 	for (i = 1; i < SIGRTMAX; i++) {
-		if (i >= __SIGRTMIN && i < SIGRTMIN)
+		if (tst_signal_is_reserved_rt(i))
 			continue;
 		if (sigismember(&set1, i) != sigismember(&set2, i))
 			tst_brk(TFAIL, "signal mask not preserved");
diff --git a/testcases/kernel/syscalls/sighold/sighold02.c b/testcases/kernel/syscalls/sighold/sighold02.c
index 1cfb7688b..cfa4a4765 100644
--- a/testcases/kernel/syscalls/sighold/sighold02.c
+++ b/testcases/kernel/syscalls/sighold/sighold02.c
@@ -17,7 +17,7 @@
  */
 
 #define _XOPEN_SOURCE 600
-#include <signal.h>
+#include "lapi/signal.h"
 #include "tst_test.h"
 
 #ifndef NSIG
@@ -33,7 +33,7 @@ static int sigs_map[NUMSIGS];
 
 static int skip_sig(int sig)
 {
-	if (sig >= 32 && sig < SIGRTMIN)
+	if (tst_signal_is_reserved_rt(sig))
 		return 1;
 
 	switch (sig) {
-- 
2.35.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [LTP] [PATCH] lapi/signal.h: rm references to _SC_SIGRT_* and __SIGRT*
  2022-04-25 13:38 [LTP] [PATCH] lapi/signal.h: rm references to _SC_SIGRT_* and __SIGRT* Richard Palethorpe via ltp
@ 2022-04-26 12:20 ` Cyril Hrubis
  2022-04-26 15:44   ` Richard Palethorpe
  0 siblings, 1 reply; 3+ messages in thread
From: Cyril Hrubis @ 2022-04-26 12:20 UTC (permalink / raw)
  To: Richard Palethorpe; +Cc: ltp

Hi!
> The old API's tst_sig.c uses_SC_SIGRT_MIN/MAX are not extant and a
> quick internet search does not reveal much. It's assumed they meant
> the same thing as tst_sigrt{min,max}_kern.
> 
> The fallback using __SIGRT{MIN,MAX} is also replaced with
> tst_signal_is_reserved_rt. It says sigrtmin on HPPA is 37. However in
> the kernel all arches have it set to 32.
> 
> In all cases __SIGRT{MIN,MAX} is replaces with constants we define as
> we can not use __ variables. Two functions are introduced to make the
> meaning clear when using these constants to check if a signal is
> reserved and/or realtime.
> 
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Suggested-by: Petr Vorel <pvorel@suse.cz>
> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
> ---
>  include/lapi/signal.h                         | 36 ++++++-------
>  lib/tst_sig.c                                 | 51 +------------------
>  testcases/kernel/syscalls/ptrace/ptrace05.c   |  2 +-
>  .../syscalls/rt_sigsuspend/rt_sigsuspend01.c  |  2 +-
>  testcases/kernel/syscalls/sighold/sighold02.c |  4 +-
>  5 files changed, 23 insertions(+), 72 deletions(-)
> 
> diff --git a/include/lapi/signal.h b/include/lapi/signal.h
> index 6f4a76881..aa185a5a6 100644
> --- a/include/lapi/signal.h
> +++ b/include/lapi/signal.h
> @@ -1,24 +1,24 @@
> -// SPDX-License-Identifier: GPL-2.0-or-later
> +/* SPDX-License-Identifier: GPL-2.0-or-later */

The style with "//" seems to be more common in LTP. If we want to unify
on the latter it should be done for all files and in a separate patch.

>  /*
> - * Copyright (c) 2019 Linaro Limited. All rights reserved.
> - * Author: Daniel D??az <daniel.diaz@linaro.org>
> + * Copyright (c) 2022 SUSE LLC
>   */
> -
> -#ifndef LAPI_SIGNAL_H__
> -#define LAPI_SIGNAL_H__

Why have you deleted the guards?

>  #include <signal.h>
>  
> -/*
> - * Some libc implementations might differ in the definitions they include. This
> - * covers those differences for all tests to successfully build.
> - */
> +const int tst_sigrtmin_kern = 32;
> +const int tst_sigrtmax_kern = 64;

I would go for macros here but I guess that either one works.

> -#ifndef __SIGRTMIN
> -# define __SIGRTMIN 32
> -#endif
> -#ifndef __SIGRTMAX
> -# define __SIGRTMAX (_NSIG - 1)
> -#endif
> +/* Indicates that libc has reserved a RT signal for use by the system
> + * libraries. Usually threading, but could be anything. */
> +__attribute__((warn_unused_result))
> +static inline int tst_signal_is_reserved_rt(const int sig)
> +{
> +	return 	sig >= tst_sigrtmin_kern && sig < SIGRTMIN;
              ^
	      Should be single space.
> +}
>  
> -#endif /* LAPI_SIGNAL_H__ */
> +/* Indicates that a signal is in the kernel's realtime set. This
> + * includes signals reserved by libc. */
> +__attribute__((const, warn_unused_result))
> +static inline int tst_signal_is_kern_rt(const int sig)
> +{
> +	return 	sig >= tst_sigrtmin_kern && sig < tst_sigrtmax_kern;
              ^
	      Here as well.
> +}
> diff --git a/lib/tst_sig.c b/lib/tst_sig.c
> index 6d77aeafd..2db3561f5 100644
> --- a/lib/tst_sig.c
> +++ b/lib/tst_sig.c
> @@ -69,7 +69,6 @@
>  
>  #include <errno.h>
>  #include <string.h>
> -#include <signal.h>
>  #include <unistd.h>
>  #include "test.h"
>  #include "lapi/signal.h"
> @@ -93,9 +92,6 @@ static void (*tst_setup_signal(int, void (*)(int))) (int);
>  void tst_sig(int fork_flag, void (*handler) (), void (*cleanup) ())
>  {
>  	int sig;
> -#ifdef _SC_SIGRT_MIN
> -	long sigrtmin, sigrtmax;
> -#endif
>  
>  	/*
>  	 * save T_cleanup and handler function pointers
> @@ -106,10 +102,6 @@ void tst_sig(int fork_flag, void (*handler) (), void (*cleanup) ())
>  		/* use default handler */
>  		handler = def_handler;
>  	}
> -#ifdef _SC_SIGRT_MIN
> -	sigrtmin = sysconf(_SC_SIGRT_MIN);
> -	sigrtmax = sysconf(_SC_SIGRT_MAX);
> -#endif
>  
>  	/*
>  	 * now loop through all signals and set the handlers
> @@ -123,54 +115,13 @@ void tst_sig(int fork_flag, void (*handler) (), void (*cleanup) ())
>  		 * SIGINFO is used for file quotas and should be expected
>  		 */
>  
> -#ifdef _SC_SIGRT_MIN
> -		if (sig >= sigrtmin && sig <= sigrtmax)
> +		if (tst_signal_is_kern_rt(sig))
>  			continue;
> -#endif
>  
>  		switch (sig) {
>  		case SIGKILL:
>  		case SIGSTOP:
>  		case SIGCONT:
> -#if !defined(_SC_SIGRT_MIN) && defined(__SIGRTMIN) && defined(__SIGRTMAX)
> -			/* Ignore all real-time signals */
> -		case __SIGRTMIN:
> -		case __SIGRTMIN + 1:
> -		case __SIGRTMIN + 2:
> -		case __SIGRTMIN + 3:
> -		case __SIGRTMIN + 4:
> -		case __SIGRTMIN + 5:
> -		case __SIGRTMIN + 6:
> -		case __SIGRTMIN + 7:
> -		case __SIGRTMIN + 8:
> -		case __SIGRTMIN + 9:
> -		case __SIGRTMIN + 10:
> -		case __SIGRTMIN + 11:
> -		case __SIGRTMIN + 12:
> -		case __SIGRTMIN + 13:
> -		case __SIGRTMIN + 14:
> -		case __SIGRTMIN + 15:
> -/* __SIGRTMIN is 37 on HPPA rather than 32 *
> - * as on i386, etc.                        */
> -#if !defined(__hppa__)
> -		case __SIGRTMAX - 15:
> -		case __SIGRTMAX - 14:
> -		case __SIGRTMAX - 13:
> -		case __SIGRTMAX - 12:
> -		case __SIGRTMAX - 11:
> -#endif
> -		case __SIGRTMAX - 10:
> -		case __SIGRTMAX - 9:
> -		case __SIGRTMAX - 8:
> -		case __SIGRTMAX - 7:
> -		case __SIGRTMAX - 6:
> -		case __SIGRTMAX - 5:
> -		case __SIGRTMAX - 4:
> -		case __SIGRTMAX - 3:
> -		case __SIGRTMAX - 2:
> -		case __SIGRTMAX - 1:
> -		case __SIGRTMAX:
> -#endif
>  #ifdef SIGSWAP
>  		case SIGSWAP:
>  #endif /* SIGSWAP */
> diff --git a/testcases/kernel/syscalls/ptrace/ptrace05.c b/testcases/kernel/syscalls/ptrace/ptrace05.c
> index 54cfa4d7b..1c1911877 100644
> --- a/testcases/kernel/syscalls/ptrace/ptrace05.c
> +++ b/testcases/kernel/syscalls/ptrace/ptrace05.c
> @@ -72,7 +72,7 @@ int main(int argc, char **argv)
>  
>  	for (signum = start_signum; signum <= end_signum; signum++) {
>  
> -		if (signum >= __SIGRTMIN && signum < SIGRTMIN)
> +		if (tst_signal_is_kern_rt(signum))

This should be tst_signal_is_reserve_rt() right?

>  			continue;
>  
>  		switch (child = fork()) {
> diff --git a/testcases/kernel/syscalls/rt_sigsuspend/rt_sigsuspend01.c b/testcases/kernel/syscalls/rt_sigsuspend/rt_sigsuspend01.c
> index 511d5fcb2..5fbfad04e 100644
> --- a/testcases/kernel/syscalls/rt_sigsuspend/rt_sigsuspend01.c
> +++ b/testcases/kernel/syscalls/rt_sigsuspend/rt_sigsuspend01.c
> @@ -50,7 +50,7 @@ static void verify_rt_sigsuspend(void)
>  
>  	SAFE_RT_SIGPROCMASK(0, NULL, &set2, SIGSETSIZE);
>  	for (i = 1; i < SIGRTMAX; i++) {
> -		if (i >= __SIGRTMIN && i < SIGRTMIN)
> +		if (tst_signal_is_reserved_rt(i))
>  			continue;
>  		if (sigismember(&set1, i) != sigismember(&set2, i))
>  			tst_brk(TFAIL, "signal mask not preserved");
> diff --git a/testcases/kernel/syscalls/sighold/sighold02.c b/testcases/kernel/syscalls/sighold/sighold02.c
> index 1cfb7688b..cfa4a4765 100644
> --- a/testcases/kernel/syscalls/sighold/sighold02.c
> +++ b/testcases/kernel/syscalls/sighold/sighold02.c
> @@ -17,7 +17,7 @@
>   */
>  
>  #define _XOPEN_SOURCE 600
> -#include <signal.h>
> +#include "lapi/signal.h"
>  #include "tst_test.h"
>  
>  #ifndef NSIG
> @@ -33,7 +33,7 @@ static int sigs_map[NUMSIGS];
>  
>  static int skip_sig(int sig)
>  {
> -	if (sig >= 32 && sig < SIGRTMIN)
> +	if (tst_signal_is_reserved_rt(sig))
>  		return 1;
>  
>  	switch (sig) {
> -- 
> 2.35.1
> 

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [LTP] [PATCH] lapi/signal.h: rm references to _SC_SIGRT_* and __SIGRT*
  2022-04-26 12:20 ` Cyril Hrubis
@ 2022-04-26 15:44   ` Richard Palethorpe
  0 siblings, 0 replies; 3+ messages in thread
From: Richard Palethorpe @ 2022-04-26 15:44 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: ltp

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> The old API's tst_sig.c uses_SC_SIGRT_MIN/MAX are not extant and a
>> quick internet search does not reveal much. It's assumed they meant
>> the same thing as tst_sigrt{min,max}_kern.
>> 
>> The fallback using __SIGRT{MIN,MAX} is also replaced with
>> tst_signal_is_reserved_rt. It says sigrtmin on HPPA is 37. However in
>> the kernel all arches have it set to 32.
>> 
>> In all cases __SIGRT{MIN,MAX} is replaces with constants we define as
>> we can not use __ variables. Two functions are introduced to make the
>> meaning clear when using these constants to check if a signal is
>> reserved and/or realtime.
>> 
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> Suggested-by: Petr Vorel <pvorel@suse.cz>
>> Suggested-by: Cyril Hrubis <chrubis@suse.cz>
>> ---
>>  include/lapi/signal.h                         | 36 ++++++-------
>>  lib/tst_sig.c                                 | 51 +------------------
>>  testcases/kernel/syscalls/ptrace/ptrace05.c   |  2 +-
>>  .../syscalls/rt_sigsuspend/rt_sigsuspend01.c  |  2 +-
>>  testcases/kernel/syscalls/sighold/sighold02.c |  4 +-
>>  5 files changed, 23 insertions(+), 72 deletions(-)
>> 
>> diff --git a/include/lapi/signal.h b/include/lapi/signal.h
>> index 6f4a76881..aa185a5a6 100644
>> --- a/include/lapi/signal.h
>> +++ b/include/lapi/signal.h
>> @@ -1,24 +1,24 @@
>> -// SPDX-License-Identifier: GPL-2.0-or-later
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>
> The style with "//" seems to be more common in LTP. If we want to unify
> on the latter it should be done for all files and in a separate patch.
>
>>  /*
>> - * Copyright (c) 2019 Linaro Limited. All rights reserved.
>> - * Author: Daniel D??az <daniel.diaz@linaro.org>
>> + * Copyright (c) 2022 SUSE LLC
>>   */
>> -
>> -#ifndef LAPI_SIGNAL_H__
>> -#define LAPI_SIGNAL_H__
>
> Why have you deleted the guards?

Uff, I don't think I was paying attention when writing this patch!

>
>>  #include <signal.h>
>>  
>> -/*
>> - * Some libc implementations might differ in the definitions they include. This
>> - * covers those differences for all tests to successfully build.
>> - */
>> +const int tst_sigrtmin_kern = 32;
>> +const int tst_sigrtmax_kern = 64;
>
> I would go for macros here but I guess that either one works.
>

This at least is deliberate because constant variables work better with
some tooling such as clangd. I'm not sure why, but it can't rename
macros. Although thinking about it, I'm not sure if these should be
static...

-- 
Thank you,
Richard.

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-04-26 15:51 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-25 13:38 [LTP] [PATCH] lapi/signal.h: rm references to _SC_SIGRT_* and __SIGRT* Richard Palethorpe via ltp
2022-04-26 12:20 ` Cyril Hrubis
2022-04-26 15:44   ` Richard Palethorpe

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.