| Message ID | 20221006151927.2079583-11-bmeng.cn@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests/qtest: Enable running qtest on Windows | expand |
On 06/10/2022 17.19, Bin Meng wrote: > From: Bin Meng <bin.meng@windriver.com> > > 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 <bin.meng@windriver.com> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > > Changes in v5: > - Replace sighandler_t with its actual definition, since it is not > available on BSD hosts > > 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 8228262938..54e5f64f20 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 void (*sighandler_old)(int); > > 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) Hmm, did you notice the error from checkpatch.pl ? ERROR: use sigaction to establish signal handlers; signal is not portable ... rationale is given in the commit description here: https://gitlab.com/qemu-project/qemu/-/commit/e8c2091d4c4dd ... but since we likely don't care about continuing running after the first signal has been delivered, I guess it's ok here to use signal() instead of sigaction? Thomas
On Tue, Oct 11, 2022 at 10:14 PM Thomas Huth <thuth@redhat.com> wrote: > > On 06/10/2022 17.19, Bin Meng wrote: > > From: Bin Meng <bin.meng@windriver.com> > > > > 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 <bin.meng@windriver.com> > > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > > > Changes in v5: > > - Replace sighandler_t with its actual definition, since it is not > > available on BSD hosts > > > > 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 8228262938..54e5f64f20 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 void (*sighandler_old)(int); > > > > 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) > > Hmm, did you notice the error from checkpatch.pl ? > > ERROR: use sigaction to establish signal handlers; signal is not portable > > ... rationale is given in the commit description here: > > https://gitlab.com/qemu-project/qemu/-/commit/e8c2091d4c4dd Yes, I noticed this checkpatch warning. > > ... but since we likely don't care about continuing running after the first > signal has been delivered, I guess it's ok here to use signal() instead of > sigaction? > I think so. I mentioned in the commit message that the code is using SA_RESETHAND for sigaction, and such usage can be replaced with signal(). Regards, Bin
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index 8228262938..54e5f64f20 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 void (*sighandler_old)(int); 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)