On Sun, 3 Sept 2023 at 15:11, 'Jinjie Ruan' via KUnit Development
<kunit-dev@xxxxxxxxxxxxxxxx> wrote:
>
> Take the last kfree(parsed_filters) and add it to be the first. Take
> the first kfree(copy) and add it to be the last. The Best practice is to
> return these errors reversely.
>
> And as David suggested, add several labels which target only the things
> which actually have been allocated so far.
>
> Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes")
> Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()")
> Signed-off-by: Jinjie Ruan <ruanjinjie@xxxxxxxxxx>
> Reviewed-by: Rae Moar <rmoar@xxxxxxxxxx>
> Suggested-by: David Gow <davidgow@xxxxxxxxxx>
> ---
> v2:
> - Add err path labels.
> - Update the commit message and title.
> ---
This looks good to me. The kernel test robot does complain about
unused labels, so it might make sense to wait to introduce the labels
in the later patches (or merge patches 2,3,4 together), but personally
I'm okay with it as-is, as these should be merged in one go.
Reviewed-by: David Gow <davidgow@xxxxxxxxxx>
-- David
> lib/kunit/executor.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 5181aa2e760b..0eda42b0c9bb 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -166,7 +166,7 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
> for (j = 0; j < filter_count; j++)
> parsed_filters[j] = kunit_next_attr_filter(&filters, err);
> if (*err)
> - goto err;
> + goto free_parsed_filters;
> }
>
> for (i = 0; &suite_set->start[i] != suite_set->end; i++) {
> @@ -178,7 +178,7 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
> parsed_glob.test_glob);
> if (IS_ERR(filtered_suite)) {
> *err = PTR_ERR(filtered_suite);
> - goto err;
> + goto free_parsed_filters;
> }
> }
> if (filter_count > 0 && parsed_filters != NULL) {
> @@ -195,10 +195,11 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
> filtered_suite = new_filtered_suite;
>
> if (*err)
> - goto err;
> + goto free_parsed_filters;
> +
> if (IS_ERR(filtered_suite)) {
> *err = PTR_ERR(filtered_suite);
> - goto err;
> + goto free_parsed_filters;
> }
> if (!filtered_suite)
> break;
> @@ -213,17 +214,19 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
> filtered.start = copy_start;
> filtered.end = copy;
>
> -err:
> - if (*err)
> - kfree(copy);
> +free_parsed_filters:
> + if (filter_count)
> + kfree(parsed_filters);
>
> +free_parsed_glob:
> if (filter_glob) {
> kfree(parsed_glob.suite_glob);
> kfree(parsed_glob.test_glob);
> }
>
> - if (filter_count)
> - kfree(parsed_filters);
> +free_copy:
> + if (*err)
> + kfree(copy);
>
> return filtered;
> }
> --
> 2.34.1
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230903071028.1518913-3-ruanjinjie%40huawei.com.
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
