[29/51] tests/qtest: libqtest: Install signal handler via signal()

Commit Message

Bin Meng Aug. 24, 2022, 9:40 a.m. UTC

From: Bin Meng <[e-mail]>

At present the codes uses sigaction() to install signal handler with
a flag SA_RESETHAND. Such usage can be covered by the signal() API
that is a simplified interface to the general sigaction() facility.

Update to use signal() to install the signal handler, as it is
avaiable on Windows which we are going to support.

Signed-off-by: Bin Meng <[e-mail]>
---

 tests/qtest/libqtest.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

Comments

Marc-André Lureau Aug. 31, 2022, 2:16 p.m. UTC | #1

Hi

On Wed, Aug 24, 2022 at 2:47 PM Bin Meng <[e-mail]> wrote:

> From: Bin Meng <[e-mail]>
>
> At present the codes uses sigaction() to install signal handler with
> a flag SA_RESETHAND. Such usage can be covered by the signal() API
> that is a simplified interface to the general sigaction() facility.
>
> Update to use signal() to install the signal handler, as it is
> avaiable on Windows which we are going to support.
>
> Signed-off-by: Bin Meng <[e-mail]>
> ---
>
>  tests/qtest/libqtest.c | 14 +++-----------
>  1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 1b24a4f1f7..70d7578740 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -68,7 +68,7 @@ struct QTestState
>  QTestState *global_qtest;
>
>  static GHookList abrt_hooks;
> -static struct sigaction sigact_old;
> +static sighandler_t sighandler_old;
>
>  static int qtest_query_target_endianness(QTestState *s);
>
> @@ -181,20 +181,12 @@ static void sigabrt_handler(int signo)
>
>  static void setup_sigabrt_handler(void)
>  {
> -    struct sigaction sigact;
> -
> -    /* Catch SIGABRT to clean up on g_assert() failure */
> -    sigact = (struct sigaction){
> -        .sa_handler = sigabrt_handler,
> -        .sa_flags = SA_RESETHAND,
> -    };
> -    sigemptyset(&sigact.sa_mask);
> -    sigaction(SIGABRT, &sigact, &sigact_old);
> +    sighandler_old = signal(SIGABRT, sigabrt_handler);
>  }
>
>  static void cleanup_sigabrt_handler(void)
>  {
> -    sigaction(SIGABRT, &sigact_old, NULL);
> +    signal(SIGABRT, sighandler_old);
>  }
>
>  static bool hook_list_is_empty(GHookList *hook_list)
> --
>


We should keep the sigaction() version for !WIN32, it has notoriously less
issues, more modern etc. signal() only on win32.

Although in this particular usage, I don't think that makes much
difference...

Bin Meng Sept. 2, 2022, 3:49 p.m. UTC | #2

On Wed, Aug 31, 2022 at 10:16 PM Marc-André Lureau
<[e-mail]> wrote:
>
> Hi
>
> On Wed, Aug 24, 2022 at 2:47 PM Bin Meng <[e-mail]> wrote:
>>
>> From: Bin Meng <[e-mail]>
>>
>> At present the codes uses sigaction() to install signal handler with
>> a flag SA_RESETHAND. Such usage can be covered by the signal() API
>> that is a simplified interface to the general sigaction() facility.
>>
>> Update to use signal() to install the signal handler, as it is
>> avaiable on Windows which we are going to support.
>>
>> Signed-off-by: Bin Meng <[e-mail]>
>> ---
>>
>>  tests/qtest/libqtest.c | 14 +++-----------
>>  1 file changed, 3 insertions(+), 11 deletions(-)
>>
>> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
>> index 1b24a4f1f7..70d7578740 100644
>> --- a/tests/qtest/libqtest.c
>> +++ b/tests/qtest/libqtest.c
>> @@ -68,7 +68,7 @@ struct QTestState
>>  QTestState *global_qtest;
>>
>>  static GHookList abrt_hooks;
>> -static struct sigaction sigact_old;
>> +static sighandler_t sighandler_old;
>>
>>  static int qtest_query_target_endianness(QTestState *s);
>>
>> @@ -181,20 +181,12 @@ static void sigabrt_handler(int signo)
>>
>>  static void setup_sigabrt_handler(void)
>>  {
>> -    struct sigaction sigact;
>> -
>> -    /* Catch SIGABRT to clean up on g_assert() failure */
>> -    sigact = (struct sigaction){
>> -        .sa_handler = sigabrt_handler,
>> -        .sa_flags = SA_RESETHAND,
>> -    };
>> -    sigemptyset(&sigact.sa_mask);
>> -    sigaction(SIGABRT, &sigact, &sigact_old);
>> +    sighandler_old = signal(SIGABRT, sigabrt_handler);
>>  }
>>
>>  static void cleanup_sigabrt_handler(void)
>>  {
>> -    sigaction(SIGABRT, &sigact_old, NULL);
>> +    signal(SIGABRT, sighandler_old);
>>  }
>>
>>  static bool hook_list_is_empty(GHookList *hook_list)
>> --
>
>
>
> We should keep the sigaction() version for !WIN32, it has notoriously less issues, more modern etc. signal() only on win32.
>
> Although in this particular usage, I don't think that makes much difference...


Yes, as I mentioned in the commit message, the codes uses sigaction()
to install signal handler with a flag SA_RESETHAND, and such can be
replaced by the signal() API.

It is still a supported API in POSIX so we should be safe to use it to
simplify the code paths. Unless you are strongly against this?

Regards,
Bin

Patch

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 1b24a4f1f7..70d7578740 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -68,7 +68,7 @@  struct QTestState
 QTestState *global_qtest;
 
 static GHookList abrt_hooks;
-static struct sigaction sigact_old;
+static sighandler_t sighandler_old;
 
 static int qtest_query_target_endianness(QTestState *s);
 
@@ -181,20 +181,12 @@  static void sigabrt_handler(int signo)
 
 static void setup_sigabrt_handler(void)
 {
-    struct sigaction sigact;
-
-    /* Catch SIGABRT to clean up on g_assert() failure */
-    sigact = (struct sigaction){
-        .sa_handler = sigabrt_handler,
-        .sa_flags = SA_RESETHAND,
-    };
-    sigemptyset(&sigact.sa_mask);
-    sigaction(SIGABRT, &sigact, &sigact_old);
+    sighandler_old = signal(SIGABRT, sigabrt_handler);
 }
 
 static void cleanup_sigabrt_handler(void)
 {
-    sigaction(SIGABRT, &sigact_old, NULL);
+    signal(SIGABRT, sighandler_old);
 }
 
 static bool hook_list_is_empty(GHookList *hook_list)