Hi Janusz,
On 2024-03-18 at 11:13:28 +0100, Janusz Krzysztofik wrote:
> Users who store a pointer to struct igt_ktap_results, obtained from
> igt_ktap_alloc(), in a central location and then call igt_ktap_free() when
> no longer needed, now have to reset that pointer to NULL to avoid double
> free on final cleanup. For their convenience, teach igt_ktap_free() to
> accept that location as an argument and reset the pointer after freeing
> the structure.
>
> Signed-off-by: Janusz Krzysztofik <janusz.krzysztofik@xxxxxxxxxxxxxxx>
LGTM,
Reviewed-by: Kamil Konieczny <kamil.konieczny@xxxxxxxxxxxxxxx>
> ---
> lib/igt_kmod.c | 12 +++++-------
> lib/igt_ktap.c | 5 +++--
> lib/igt_ktap.h | 2 +-
> lib/tests/igt_ktap_parser.c | 24 ++++++++++++------------
> 4 files changed, 21 insertions(+), 22 deletions(-)
>
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index ca20012a97..8a6824ea7e 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -1094,7 +1094,7 @@ static void __igt_kunit_legacy(struct igt_ktest *tst,
>
> if (igt_debug_on(pthread_create(&modprobe.thread, NULL,
> modprobe_task, &modprobe))) {
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
> igt_skip("Failed to create a modprobe thread\n");
> }
>
> @@ -1197,7 +1197,7 @@ static void __igt_kunit_legacy(struct igt_ktest *tst,
> break;
> }
>
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
>
> igt_skip_on(modprobe.err);
> igt_skip_on(igt_kernel_tainted(&taints));
> @@ -1243,8 +1243,7 @@ static bool kunit_get_tests(struct igt_list_head *tests,
> err = kunit_kmsg_result_get(tests, NULL, tst->kmsg, *ktap);
> while (err == -EINPROGRESS);
>
> - igt_ktap_free(*ktap);
> - *ktap = NULL;
> + igt_ktap_free(ktap);
>
> igt_skip_on_f(err,
> "KTAP parser failed while getting a list of test cases\n");
> @@ -1405,8 +1404,7 @@ static void __igt_kunit(struct igt_ktest *tst,
> }
> }
>
> - igt_ktap_free(*ktap);
> - *ktap = NULL;
> + igt_ktap_free(ktap);
>
> igt_skip_on(modprobe.err);
> igt_skip_on(igt_kernel_tainted(&taints));
> @@ -1487,7 +1485,7 @@ void igt_kunit(const char *module_name, const char *suite, const char *opts)
> igt_fixture {
> char *suite_name = NULL, *case_name = NULL;
>
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
>
> kunit_results_free(&tests, &suite_name, &case_name);
>
> diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
> index aa7ea84476..300fb2bb5a 100644
> --- a/lib/igt_ktap.c
> +++ b/lib/igt_ktap.c
> @@ -310,7 +310,8 @@ struct igt_ktap_results *igt_ktap_alloc(struct igt_list_head *results)
> return ktap;
> }
>
> -void igt_ktap_free(struct igt_ktap_results *ktap)
> +void igt_ktap_free(struct igt_ktap_results **ktap)
> {
> - free(ktap);
> + free(*ktap);
> + *ktap = NULL;
> }
> diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
> index c422636bfc..7684e859b3 100644
> --- a/lib/igt_ktap.h
> +++ b/lib/igt_ktap.h
> @@ -41,6 +41,6 @@ struct igt_ktap_results;
>
> struct igt_ktap_results *igt_ktap_alloc(struct igt_list_head *results);
> int igt_ktap_parse(const char *buf, struct igt_ktap_results *ktap);
> -void igt_ktap_free(struct igt_ktap_results *ktap);
> +void igt_ktap_free(struct igt_ktap_results **ktap);
>
> #endif /* IGT_KTAP_H */
> diff --git a/lib/tests/igt_ktap_parser.c b/lib/tests/igt_ktap_parser.c
> index 6357bdf6a5..8c2d16080d 100644
> --- a/lib/tests/igt_ktap_parser.c
> +++ b/lib/tests/igt_ktap_parser.c
> @@ -45,7 +45,7 @@ static void ktap_list(void)
> igt_assert_eq(igt_ktap_parse(" ok 4 test_case_4 # SKIP\n", ktap), -EINPROGRESS);
> igt_assert_eq(igt_ktap_parse("ok 3 test_suite_3\n", ktap), 0);
>
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
>
> igt_assert_eq(igt_list_length(&results), 8);
>
> @@ -107,7 +107,7 @@ static void ktap_results(void)
> igt_assert_eq(igt_ktap_parse(" ok 1 test_case\n", ktap), -EINPROGRESS);
> igt_assert_eq(igt_ktap_parse("not ok 1 test_suite\n", ktap), 0);
>
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
>
> igt_assert_eq(igt_list_length(&results), 2);
>
> @@ -162,7 +162,7 @@ static void ktap_success(void)
> igt_assert_eq(igt_ktap_parse("not ok 1 test_suite\n", ktap), 0);
> igt_assert_eq(igt_list_length(&results), 2);
>
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
>
> result = igt_list_last_entry(&results, result, link);
> igt_list_del(&result->link);
> @@ -186,48 +186,48 @@ static void ktap_top_version(void)
> ktap = igt_ktap_alloc(&results);
> igt_require(ktap);
> igt_assert_eq(igt_ktap_parse("1..1\n", ktap), -EPROTO);
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
>
> ktap = igt_ktap_alloc(&results);
> igt_require(ktap);
> /* TODO: change to -EPROTO as soon as related workaround is dropped */
> igt_assert_eq(igt_ktap_parse(" KTAP version 1\n", ktap), -EINPROGRESS);
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
>
> ktap = igt_ktap_alloc(&results);
> igt_require(ktap);
> igt_assert_eq(igt_ktap_parse(" # Subtest: test_suite\n", ktap), -EPROTO);
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
>
> ktap = igt_ktap_alloc(&results);
> igt_require(ktap);
> igt_assert_eq(igt_ktap_parse(" 1..1\n", ktap), -EPROTO);
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
>
> ktap = igt_ktap_alloc(&results);
> igt_require(ktap);
> igt_assert_eq(igt_ktap_parse(" KTAP version 1\n", ktap), -EPROTO);
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
>
> ktap = igt_ktap_alloc(&results);
> igt_require(ktap);
> igt_assert_eq(igt_ktap_parse(" # Subtest: test_case\n", ktap), -EPROTO);
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
>
> ktap = igt_ktap_alloc(&results);
> igt_require(ktap);
> igt_assert_eq(igt_ktap_parse(" ok 1 parameter 1\n", ktap), -EPROTO);
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
>
> ktap = igt_ktap_alloc(&results);
> igt_require(ktap);
> igt_assert_eq(igt_ktap_parse(" ok 1 test_case\n", ktap), -EPROTO);
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
>
> ktap = igt_ktap_alloc(&results);
> igt_require(ktap);
> igt_assert_eq(igt_ktap_parse("ok 1 test_suite\n", ktap), -EPROTO);
> - igt_ktap_free(ktap);
> + igt_ktap_free(&ktap);
> }
>
> igt_main
> --
> 2.43.0
>
