Re: [PATCH v2 04/12] libqtest: add qtest_remove_abrt_handler()

[Wainer dos Santos Moschetta]

On 12/7/20 2:20 PM, Stefan Hajnoczi wrote:
> Add a function to remove previously-added abrt handler functions.
>
> Now that a symmetric pair of add/remove functions exists we can also
> balance the SIGABRT handler installation. The signal handler was
> installed each time qtest_add_abrt_handler() was called. Now it is
> installed when the abrt handler list becomes non-empty and removed again
> when the list becomes empty.
>
> The qtest_remove_abrt_handler() function will be used by
> vhost-user-blk-test.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   tests/qtest/libqos/libqtest.h | 18 ++++++++++++++++++
>   tests/qtest/libqtest.c        | 35 +++++++++++++++++++++++++++++------
>   2 files changed, 47 insertions(+), 6 deletions(-)


Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


> diff --git a/tests/qtest/libqos/libqtest.h b/tests/qtest/libqos/libqtest.h
> index 51287b9276..a68dcd79d4 100644
> --- a/tests/qtest/libqos/libqtest.h
> +++ b/tests/qtest/libqos/libqtest.h
> @@ -649,8 +649,26 @@ void qtest_add_data_func_full(const char *str, void *data,
>           g_free(path); \
>       } while (0)
>   
> +/**
> + * qtest_add_abrt_handler:
> + * @fn: Handler function
> + * @data: Argument that is passed to the handler
> + *
> + * Add a handler function that is invoked on SIGABRT. This can be used to
> + * terminate processes and perform other cleanup. The handler can be removed
> + * with qtest_remove_abrt_handler().
> + */
>   void qtest_add_abrt_handler(GHookFunc fn, const void *data);
>   
> +/**
> + * qtest_remove_abrt_handler:
> + * @data: Argument previously passed to qtest_add_abrt_handler()
> + *
> + * Remove an abrt handler that was previously added with
> + * qtest_add_abrt_handler().
> + */
> +void qtest_remove_abrt_handler(void *data);
> +
>   /**
>    * qtest_qmp_assert_success:
>    * @qts: QTestState instance to operate on
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index cc2cec4a35..7cf247baf0 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -196,15 +196,30 @@ static void cleanup_sigabrt_handler(void)
>       sigaction(SIGABRT, &sigact_old, NULL);
>   }
>   
> +static bool hook_list_is_empty(GHookList *hook_list)
> +{
> +    GHook *hook = g_hook_first_valid(hook_list, TRUE);
> +
> +    if (!hook) {
> +        return false;
> +    }
> +
> +    g_hook_unref(hook_list, hook);
> +    return true;
> +}
> +
>   void qtest_add_abrt_handler(GHookFunc fn, const void *data)
>   {
>       GHook *hook;
>   
> -    /* Only install SIGABRT handler once */
>       if (!abrt_hooks.is_setup) {
>           g_hook_list_init(&abrt_hooks, sizeof(GHook));
>       }
> -    setup_sigabrt_handler();
> +
> +    /* Only install SIGABRT handler once */
> +    if (hook_list_is_empty(&abrt_hooks)) {
> +        setup_sigabrt_handler();
> +    }
>   
>       hook = g_hook_alloc(&abrt_hooks);
>       hook->func = fn;
> @@ -213,6 +228,17 @@ void qtest_add_abrt_handler(GHookFunc fn, const void *data)
>       g_hook_prepend(&abrt_hooks, hook);
>   }
>   
> +void qtest_remove_abrt_handler(void *data)
> +{
> +    GHook *hook = g_hook_find_data(&abrt_hooks, TRUE, data);
> +    g_hook_destroy_link(&abrt_hooks, hook);
> +
> +    /* Uninstall SIGABRT handler on last instance */
> +    if (hook_list_is_empty(&abrt_hooks)) {
> +        cleanup_sigabrt_handler();
> +    }
> +}
> +
>   static const char *qtest_qemu_binary(void)
>   {
>       const char *qemu_bin;
> @@ -369,10 +395,7 @@ QTestState *qtest_init_with_serial(const char *extra_args, 
> int *sock_fd)
>   
>   void qtest_quit(QTestState *s)
>   {
> -    g_hook_destroy_link(&abrt_hooks, g_hook_find_data(&abrt_hooks, TRUE, s));
> -
> -    /* Uninstall SIGABRT handler on last instance */
> -    cleanup_sigabrt_handler();
> +    qtest_remove_abrt_handler(s);
>   
>       qtest_kill_qemu(s);
>       close(s->fd);