[v2,18/39] tests/qtest: libqtest: Install signal handler via signal()

Commit Message

Bin Meng Sept. 20, 2022, 10:31 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
available on Windows which we are going to support.

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

(no changes since v1)

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

Comments

Marc-André Lureau Sept. 22, 2022, 7:55 p.m. UTC | #1

Hi

On Tue, Sep 20, 2022 at 2:32 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
> available on Windows which we are going to support.
>
> Signed-off-by: Bin Meng <[e-mail]>
> ---
>
> (no changes since v1)
>
>  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 8b804faade..f46a21fa45 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -66,7 +66,7 @@ struct QTestState
>  };
>
>  static GHookList abrt_hooks;
> -static struct sigaction sigact_old;
> +static sighandler_t sighandler_old;
>
>  static int qtest_query_target_endianness(QTestState *s);
>
> @@ -179,20 +179,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);
>

I would rather make the usage of signal() specific to WIN32, but it's up to
the maintainers to decide what's best.

Thomas, Laurent, opinions?

Thomas Huth Sept. 23, 2022, 5:54 p.m. UTC | #2

On 22/09/2022 21.55, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Sep 20, 2022 at 2:32 PM Bin Meng <[e-mail] 
> <mailto:[e-mail]>> wrote:
> 
>     From: Bin Meng <[e-mail] <mailto:[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
>     available on Windows which we are going to support.
> 
>     Signed-off-by: Bin Meng <[e-mail]
>     <mailto:[e-mail]>>
>     ---
> 
>     (no changes since v1)
> 
>       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 8b804faade..f46a21fa45 100644
>     --- a/tests/qtest/libqtest.c
>     +++ b/tests/qtest/libqtest.c
>     @@ -66,7 +66,7 @@ struct QTestState
>       };
> 
>       static GHookList abrt_hooks;
>     -static struct sigaction sigact_old;
>     +static sighandler_t sighandler_old;
> 
>       static int qtest_query_target_endianness(QTestState *s);
> 
>     @@ -179,20 +179,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);
> 
> 
> I would rather make the usage of signal() specific to WIN32, but it's up to 
> the maintainers to decide what's best.
> 
> Thomas, Laurent, opinions?

I don't mind much either way ... I'd say let's take this patch since it's 
already done and it avoids some #ifdefs in the code.

  Thomas

Patch

diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 8b804faade..f46a21fa45 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -66,7 +66,7 @@  struct QTestState
 };
 
 static GHookList abrt_hooks;
-static struct sigaction sigact_old;
+static sighandler_t sighandler_old;
 
 static int qtest_query_target_endianness(QTestState *s);
 
@@ -179,20 +179,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)