[v2,7/9] perf tests: Run time generate shell test suites

Message ID 20240201001504.1348511-7-irogers@google.com
State New
Headers
Series [v2,1/9] perf thread_map: Skip exited threads when scanning /proc |

Commit Message

Ian Rogers Feb. 1, 2024, 12:15 a.m. UTC
  Rather than special shell test logic, do a single pass to create an
array of test suites. Hold the shell test file name in the test suite
priv field. This makes the special shell test logic in builtin-test.c
redundant so remove it.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/tests/builtin-test.c  |  91 +----------------------
 tools/perf/tests/tests-scripts.c | 120 ++++++++++++++++++-------------
 tools/perf/tests/tests-scripts.h |  10 +--
 3 files changed, 74 insertions(+), 147 deletions(-)
  

Comments

Namhyung Kim Feb. 10, 2024, 4:41 a.m. UTC | #1
On Wed, Jan 31, 2024 at 4:15 PM Ian Rogers <irogers@google.com> wrote:
>
> Rather than special shell test logic, do a single pass to create an
> array of test suites. Hold the shell test file name in the test suite
> priv field. This makes the special shell test logic in builtin-test.c
> redundant so remove it.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/tests/builtin-test.c  |  91 +----------------------
>  tools/perf/tests/tests-scripts.c | 120 ++++++++++++++++++-------------
>  tools/perf/tests/tests-scripts.h |  10 +--
>  3 files changed, 74 insertions(+), 147 deletions(-)
>
> diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> index 6d5001daaf63..c42cb40fc242 100644
> --- a/tools/perf/tests/builtin-test.c
> +++ b/tools/perf/tests/builtin-test.c
> @@ -130,6 +130,7 @@ static struct test_suite *generic_tests[] = {
>  static struct test_suite **tests[] = {
>         generic_tests,
>         arch_tests,
> +       NULL, /* shell tests created at runtime. */
>  };
>
>  static struct test_workload *workloads[] = {
> @@ -299,74 +300,12 @@ static int test_and_print(struct test_suite *t, int subtest)
>         return err;
>  }
>
> -struct shell_test {
> -       const char *file;
> -};
> -
> -static int shell_test__run(struct test_suite *test, int subdir __maybe_unused)
> -{
> -       int err;
> -       struct shell_test *st = test->priv;
> -       char *cmd;
> -
> -       asprintf(&cmd, "%s%s", st->file, verbose ? " -v" : "");
> -       if (!cmd)
> -               return TEST_FAIL;
> -       err = system(cmd);
> -       free(cmd);
> -       if (!err)
> -               return TEST_OK;
> -
> -       return WEXITSTATUS(err) == 2 ? TEST_SKIP : TEST_FAIL;
> -}
> -
> -static int run_shell_tests(int argc, const char *argv[], int i, int width,
> -                               struct intlist *skiplist)
> -{
> -       struct shell_test st;
> -       const struct script_file *files, *file;
> -
> -       files = list_script_files();
> -       if (!files)
> -               return 0;
> -       for (file = files; file->file; file++) {
> -               int curr = i++;
> -               struct test_case test_cases[] = {
> -                       {
> -                               .desc = file->desc,
> -                               .run_case = shell_test__run,
> -                       },
> -                       { .name = NULL, }
> -               };
> -               struct test_suite test_suite = {
> -                       .desc = test_cases[0].desc,
> -                       .test_cases = test_cases,
> -                       .priv = &st,
> -               };
> -               st.file = file->file;
> -
> -               if (test_suite.desc == NULL ||
> -                   !perf_test__matches(test_suite.desc, curr, argc, argv))
> -                       continue;
> -
> -               pr_info("%3d: %-*s:", i, width, test_suite.desc);
> -
> -               if (intlist__find(skiplist, i)) {
> -                       color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip (user override)\n");
> -                       continue;
> -               }
> -
> -               test_and_print(&test_suite, 0);
> -       }
> -       return 0;
> -}
> -
>  static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
>  {
>         struct test_suite *t;
>         unsigned int j, k;
>         int i = 0;
> -       int width = list_script_max_width();
> +       int width = 0;
>
>         for_each_test(j, k, t) {
>                 int len = strlen(test_description(t, -1));
> @@ -441,28 +380,6 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
>                         }
>                 }
>         }
> -
> -       return run_shell_tests(argc, argv, i, width, skiplist);
> -}
> -
> -static int perf_test__list_shell(int argc, const char **argv, int i)
> -{
> -       const struct script_file *files, *file;
> -
> -       files = list_script_files();
> -       if (!files)
> -               return 0;
> -       for (file = files; file->file; file++) {
> -               int curr = i++;
> -               struct test_suite t = {
> -                       .desc = file->desc
> -               };
> -
> -               if (!perf_test__matches(t.desc, curr, argc, argv))
> -                       continue;
> -
> -               pr_info("%3d: %s\n", i, t.desc);
> -       }
>         return 0;
>  }
>
> @@ -489,9 +406,6 @@ static int perf_test__list(int argc, const char **argv)
>                                         test_description(t, subi));
>                 }
>         }
> -
> -       perf_test__list_shell(argc, argv, i);
> -
>         return 0;
>  }
>
> @@ -551,6 +465,7 @@ int cmd_test(int argc, const char **argv)
>         /* Unbuffered output */
>         setvbuf(stdout, NULL, _IONBF, 0);
>
> +       tests[2] = create_script_test_suites();

I like this part! :)


>         argc = parse_options_subcommand(argc, argv, test_options, test_subcommands, test_usage, 0);
>         if (argc >= 1 && !strcmp(argv[0], "list"))
>                 return perf_test__list(argc - 1, argv + 1);
> diff --git a/tools/perf/tests/tests-scripts.c b/tools/perf/tests/tests-scripts.c
> index 9b3b66dd5508..b92a93c251c6 100644
> --- a/tools/perf/tests/tests-scripts.c
> +++ b/tools/perf/tests/tests-scripts.c
> @@ -26,16 +26,6 @@
>  #include "tests.h"
>  #include "util/rlimit.h"
>
> -
> -/*
> - * As this is a singleton built once for the run of the process, there is
> - * no value in trying to free it and just let it stay around until process
> - * exits when it's cleaned up.
> - */
> -static size_t files_num = 0;
> -static struct script_file *files = NULL;
> -static int files_max_width = 0;
> -
>  static int shell_tests__dir_fd(void)
>  {
>         char path[PATH_MAX], *exec_path;
> @@ -131,12 +121,31 @@ static char *strdup_check(const char *str)
>         return newstr;
>  }
>
> -static void append_script(int dir_fd, const char *name, char *desc)
> +static int shell_test__run(struct test_suite *test, int subtest __maybe_unused)
> +{
> +       const char *file = test->priv;
> +       int err;
> +       char *cmd;
> +
> +       asprintf(&cmd, "%s%s", file, verbose ? " -v" : "");
> +       if (!cmd)
> +               return TEST_FAIL;
> +       err = system(cmd);
> +       free(cmd);
> +       if (!err)
> +               return TEST_OK;
> +
> +       return WEXITSTATUS(err) == 2 ? TEST_SKIP : TEST_FAIL;
> +}
> +
> +static void append_script(int dir_fd, const char *name, char *desc,
> +                         struct test_suite ***result,
> +                         size_t *result_sz)
>  {
>         char filename[PATH_MAX], link[128];
> -       struct script_file *files_tmp;
> -       size_t files_num_tmp, len;
> -       int width;
> +       struct test_suite *test_suite, **result_tmp;
> +       struct test_case *tests;
> +       size_t len;
>
>         snprintf(link, sizeof(link), "/proc/%d/fd/%d", getpid(), dir_fd);
>         len = readlink(link, filename, sizeof(filename));
> @@ -146,33 +155,43 @@ static void append_script(int dir_fd, const char *name, char *desc)
>         }
>         filename[len++] = '/';
>         strcpy(&filename[len], name);
> -       files_num_tmp = files_num + 1;
> -       if (files_num_tmp >= SIZE_MAX) {
> -               pr_err("Too many script files\n");
> -               abort();
> +
> +       tests = calloc(2, sizeof(*tests));
> +       if (!tests) {
> +               pr_err("Out of memory while building script test suite list\n");
> +               return;
> +       }
> +       tests[0].name = strdup_check(name);
> +       tests[0].desc = strdup_check(desc);
> +       tests[0].run_case = shell_test__run;
> +
> +       test_suite = zalloc(sizeof(*test_suite));
> +       if (!test_suite) {
> +               pr_err("Out of memory while building script test suite list\n");
> +               free(tests);
> +               return;
>         }
> +       test_suite->desc = desc;
> +       test_suite->test_cases = tests;
> +       test_suite->priv = strdup_check(filename);
>         /* Realloc is good enough, though we could realloc by chunks, not that
>          * anyone will ever measure performance here */
> -       files_tmp = realloc(files,
> -                           (files_num_tmp + 1) * sizeof(struct script_file));
> -       if (files_tmp == NULL) {
> -               pr_err("Out of memory while building test list\n");
> -               abort();
> +       result_tmp = realloc(*result, (*result_sz + 1) * sizeof(*result_tmp));
> +       if (result_tmp == NULL) {
> +               pr_err("Out of memory while building script test suite list\n");
> +               free(tests);
> +               free(test_suite);
> +               return;
>         }
>         /* Add file to end and NULL terminate the struct array */
> -       files = files_tmp;
> -       files_num = files_num_tmp;
> -       files[files_num - 1].file = strdup_check(filename);
> -       files[files_num - 1].desc = desc;
> -       files[files_num].file = NULL;
> -       files[files_num].desc = NULL;
> -
> -       width = strlen(desc); /* Track max width of desc */
> -       if (width > files_max_width)
> -               files_max_width = width;
> +       *result = result_tmp;
> +       (*result)[*result_sz] = test_suite;
> +       (*result_sz)++;
>  }
>
> -static void append_scripts_in_dir(int dir_fd)
> +static void append_scripts_in_dir(int dir_fd,
> +                                 struct test_suite ***result,
> +                                 size_t *result_sz)
>  {
>         struct dirent **entlist;
>         struct dirent *ent;
> @@ -191,7 +210,7 @@ static void append_scripts_in_dir(int dir_fd)
>                         char *desc = shell_test__description(dir_fd, ent->d_name);
>
>                         if (desc) /* It has a desc line - valid script */
> -                               append_script(dir_fd, ent->d_name, desc);
> +                               append_script(dir_fd, ent->d_name, desc, result, result_sz);
>                         continue;
>                 }
>                 if (ent->d_type != DT_DIR) {
> @@ -204,32 +223,31 @@ static void append_scripts_in_dir(int dir_fd)
>                                 continue;
>                 }
>                 fd = openat(dir_fd, ent->d_name, O_PATH);
> -               append_scripts_in_dir(fd);
> +               append_scripts_in_dir(fd, result, result_sz);
>         }
>         for (i = 0; i < n_dirs; i++) /* Clean up */
>                 zfree(&entlist[i]);
>         free(entlist);
>  }
>
> -const struct script_file *list_script_files(void)
> +struct test_suite **create_script_test_suites(void)
>  {
> -       int dir_fd;
> -
> -       if (files)
> -               return files; /* Singleton - we already know our list */
> +       struct test_suite **result = NULL, **result_tmp;
> +       size_t result_sz = 0;
> +       int dir_fd = shell_tests__dir_fd(); /* Walk  dir */
>
> -       dir_fd = shell_tests__dir_fd(); /* Walk  dir */
>         if (dir_fd < 0)
>                 return NULL;

When can it fail?  If the test directory doesn't exist?
I'm afraid returning NULL here can crash perf test.
I think you need to return a NULL entry result.

Thanks,
Namhyung


>
> -       append_scripts_in_dir(dir_fd);
> +       append_scripts_in_dir(dir_fd, &result, &result_sz);
> +       result_tmp = realloc(result, (result_sz + 1) * sizeof(*result_tmp));
> +       if (result_tmp == NULL) {
> +               pr_err("Out of memory while building script test suite list\n");
> +               abort();
> +       }
> +       /* NULL terminate the test suite array. */
> +       result = result_tmp;
> +       result[result_sz] = NULL;
>         close(dir_fd);
> -
> -       return files;
> -}
> -
> -int list_script_max_width(void)
> -{
> -       list_script_files(); /* Ensure we have scanned all scripts */
> -       return files_max_width;
> +       return result;
>  }
> diff --git a/tools/perf/tests/tests-scripts.h b/tools/perf/tests/tests-scripts.h
> index 3508a293aaf9..b553ad26ea17 100644
> --- a/tools/perf/tests/tests-scripts.h
> +++ b/tools/perf/tests/tests-scripts.h
> @@ -2,14 +2,8 @@
>  #ifndef TESTS_SCRIPTS_H
>  #define TESTS_SCRIPTS_H
>
> -struct script_file {
> -       char *file;
> -       char *desc;
> -};
> +#include "tests.h"
>
> -/* List available script tests to run - singleton - never freed */
> -const struct script_file *list_script_files(void);
> -/* Get maximum width of description string */
> -int list_script_max_width(void);
> +struct test_suite **create_script_test_suites(void);
>
>  #endif /* TESTS_SCRIPTS_H */
> --
> 2.43.0.429.g432eaa2c6b-goog
>
  
Ian Rogers Feb. 12, 2024, 5:42 p.m. UTC | #2
On Fri, Feb 9, 2024 at 8:41 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Wed, Jan 31, 2024 at 4:15 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Rather than special shell test logic, do a single pass to create an
> > array of test suites. Hold the shell test file name in the test suite
> > priv field. This makes the special shell test logic in builtin-test.c
> > redundant so remove it.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/tests/builtin-test.c  |  91 +----------------------
> >  tools/perf/tests/tests-scripts.c | 120 ++++++++++++++++++-------------
> >  tools/perf/tests/tests-scripts.h |  10 +--
> >  3 files changed, 74 insertions(+), 147 deletions(-)
> >
> > diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
> > index 6d5001daaf63..c42cb40fc242 100644
> > --- a/tools/perf/tests/builtin-test.c
> > +++ b/tools/perf/tests/builtin-test.c
> > @@ -130,6 +130,7 @@ static struct test_suite *generic_tests[] = {
> >  static struct test_suite **tests[] = {
> >         generic_tests,
> >         arch_tests,
> > +       NULL, /* shell tests created at runtime. */
> >  };
> >
> >  static struct test_workload *workloads[] = {
> > @@ -299,74 +300,12 @@ static int test_and_print(struct test_suite *t, int subtest)
> >         return err;
> >  }
> >
> > -struct shell_test {
> > -       const char *file;
> > -};
> > -
> > -static int shell_test__run(struct test_suite *test, int subdir __maybe_unused)
> > -{
> > -       int err;
> > -       struct shell_test *st = test->priv;
> > -       char *cmd;
> > -
> > -       asprintf(&cmd, "%s%s", st->file, verbose ? " -v" : "");
> > -       if (!cmd)
> > -               return TEST_FAIL;
> > -       err = system(cmd);
> > -       free(cmd);
> > -       if (!err)
> > -               return TEST_OK;
> > -
> > -       return WEXITSTATUS(err) == 2 ? TEST_SKIP : TEST_FAIL;
> > -}
> > -
> > -static int run_shell_tests(int argc, const char *argv[], int i, int width,
> > -                               struct intlist *skiplist)
> > -{
> > -       struct shell_test st;
> > -       const struct script_file *files, *file;
> > -
> > -       files = list_script_files();
> > -       if (!files)
> > -               return 0;
> > -       for (file = files; file->file; file++) {
> > -               int curr = i++;
> > -               struct test_case test_cases[] = {
> > -                       {
> > -                               .desc = file->desc,
> > -                               .run_case = shell_test__run,
> > -                       },
> > -                       { .name = NULL, }
> > -               };
> > -               struct test_suite test_suite = {
> > -                       .desc = test_cases[0].desc,
> > -                       .test_cases = test_cases,
> > -                       .priv = &st,
> > -               };
> > -               st.file = file->file;
> > -
> > -               if (test_suite.desc == NULL ||
> > -                   !perf_test__matches(test_suite.desc, curr, argc, argv))
> > -                       continue;
> > -
> > -               pr_info("%3d: %-*s:", i, width, test_suite.desc);
> > -
> > -               if (intlist__find(skiplist, i)) {
> > -                       color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip (user override)\n");
> > -                       continue;
> > -               }
> > -
> > -               test_and_print(&test_suite, 0);
> > -       }
> > -       return 0;
> > -}
> > -
> >  static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
> >  {
> >         struct test_suite *t;
> >         unsigned int j, k;
> >         int i = 0;
> > -       int width = list_script_max_width();
> > +       int width = 0;
> >
> >         for_each_test(j, k, t) {
> >                 int len = strlen(test_description(t, -1));
> > @@ -441,28 +380,6 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
> >                         }
> >                 }
> >         }
> > -
> > -       return run_shell_tests(argc, argv, i, width, skiplist);
> > -}
> > -
> > -static int perf_test__list_shell(int argc, const char **argv, int i)
> > -{
> > -       const struct script_file *files, *file;
> > -
> > -       files = list_script_files();
> > -       if (!files)
> > -               return 0;
> > -       for (file = files; file->file; file++) {
> > -               int curr = i++;
> > -               struct test_suite t = {
> > -                       .desc = file->desc
> > -               };
> > -
> > -               if (!perf_test__matches(t.desc, curr, argc, argv))
> > -                       continue;
> > -
> > -               pr_info("%3d: %s\n", i, t.desc);
> > -       }
> >         return 0;
> >  }
> >
> > @@ -489,9 +406,6 @@ static int perf_test__list(int argc, const char **argv)
> >                                         test_description(t, subi));
> >                 }
> >         }
> > -
> > -       perf_test__list_shell(argc, argv, i);
> > -
> >         return 0;
> >  }
> >
> > @@ -551,6 +465,7 @@ int cmd_test(int argc, const char **argv)
> >         /* Unbuffered output */
> >         setvbuf(stdout, NULL, _IONBF, 0);
> >
> > +       tests[2] = create_script_test_suites();
>
> I like this part! :)
>
>
> >         argc = parse_options_subcommand(argc, argv, test_options, test_subcommands, test_usage, 0);
> >         if (argc >= 1 && !strcmp(argv[0], "list"))
> >                 return perf_test__list(argc - 1, argv + 1);
> > diff --git a/tools/perf/tests/tests-scripts.c b/tools/perf/tests/tests-scripts.c
> > index 9b3b66dd5508..b92a93c251c6 100644
> > --- a/tools/perf/tests/tests-scripts.c
> > +++ b/tools/perf/tests/tests-scripts.c
> > @@ -26,16 +26,6 @@
> >  #include "tests.h"
> >  #include "util/rlimit.h"
> >
> > -
> > -/*
> > - * As this is a singleton built once for the run of the process, there is
> > - * no value in trying to free it and just let it stay around until process
> > - * exits when it's cleaned up.
> > - */
> > -static size_t files_num = 0;
> > -static struct script_file *files = NULL;
> > -static int files_max_width = 0;
> > -
> >  static int shell_tests__dir_fd(void)
> >  {
> >         char path[PATH_MAX], *exec_path;
> > @@ -131,12 +121,31 @@ static char *strdup_check(const char *str)
> >         return newstr;
> >  }
> >
> > -static void append_script(int dir_fd, const char *name, char *desc)
> > +static int shell_test__run(struct test_suite *test, int subtest __maybe_unused)
> > +{
> > +       const char *file = test->priv;
> > +       int err;
> > +       char *cmd;
> > +
> > +       asprintf(&cmd, "%s%s", file, verbose ? " -v" : "");
> > +       if (!cmd)
> > +               return TEST_FAIL;
> > +       err = system(cmd);
> > +       free(cmd);
> > +       if (!err)
> > +               return TEST_OK;
> > +
> > +       return WEXITSTATUS(err) == 2 ? TEST_SKIP : TEST_FAIL;
> > +}
> > +
> > +static void append_script(int dir_fd, const char *name, char *desc,
> > +                         struct test_suite ***result,
> > +                         size_t *result_sz)
> >  {
> >         char filename[PATH_MAX], link[128];
> > -       struct script_file *files_tmp;
> > -       size_t files_num_tmp, len;
> > -       int width;
> > +       struct test_suite *test_suite, **result_tmp;
> > +       struct test_case *tests;
> > +       size_t len;
> >
> >         snprintf(link, sizeof(link), "/proc/%d/fd/%d", getpid(), dir_fd);
> >         len = readlink(link, filename, sizeof(filename));
> > @@ -146,33 +155,43 @@ static void append_script(int dir_fd, const char *name, char *desc)
> >         }
> >         filename[len++] = '/';
> >         strcpy(&filename[len], name);
> > -       files_num_tmp = files_num + 1;
> > -       if (files_num_tmp >= SIZE_MAX) {
> > -               pr_err("Too many script files\n");
> > -               abort();
> > +
> > +       tests = calloc(2, sizeof(*tests));
> > +       if (!tests) {
> > +               pr_err("Out of memory while building script test suite list\n");
> > +               return;
> > +       }
> > +       tests[0].name = strdup_check(name);
> > +       tests[0].desc = strdup_check(desc);
> > +       tests[0].run_case = shell_test__run;
> > +
> > +       test_suite = zalloc(sizeof(*test_suite));
> > +       if (!test_suite) {
> > +               pr_err("Out of memory while building script test suite list\n");
> > +               free(tests);
> > +               return;
> >         }
> > +       test_suite->desc = desc;
> > +       test_suite->test_cases = tests;
> > +       test_suite->priv = strdup_check(filename);
> >         /* Realloc is good enough, though we could realloc by chunks, not that
> >          * anyone will ever measure performance here */
> > -       files_tmp = realloc(files,
> > -                           (files_num_tmp + 1) * sizeof(struct script_file));
> > -       if (files_tmp == NULL) {
> > -               pr_err("Out of memory while building test list\n");
> > -               abort();
> > +       result_tmp = realloc(*result, (*result_sz + 1) * sizeof(*result_tmp));
> > +       if (result_tmp == NULL) {
> > +               pr_err("Out of memory while building script test suite list\n");
> > +               free(tests);
> > +               free(test_suite);
> > +               return;
> >         }
> >         /* Add file to end and NULL terminate the struct array */
> > -       files = files_tmp;
> > -       files_num = files_num_tmp;
> > -       files[files_num - 1].file = strdup_check(filename);
> > -       files[files_num - 1].desc = desc;
> > -       files[files_num].file = NULL;
> > -       files[files_num].desc = NULL;
> > -
> > -       width = strlen(desc); /* Track max width of desc */
> > -       if (width > files_max_width)
> > -               files_max_width = width;
> > +       *result = result_tmp;
> > +       (*result)[*result_sz] = test_suite;
> > +       (*result_sz)++;
> >  }
> >
> > -static void append_scripts_in_dir(int dir_fd)
> > +static void append_scripts_in_dir(int dir_fd,
> > +                                 struct test_suite ***result,
> > +                                 size_t *result_sz)
> >  {
> >         struct dirent **entlist;
> >         struct dirent *ent;
> > @@ -191,7 +210,7 @@ static void append_scripts_in_dir(int dir_fd)
> >                         char *desc = shell_test__description(dir_fd, ent->d_name);
> >
> >                         if (desc) /* It has a desc line - valid script */
> > -                               append_script(dir_fd, ent->d_name, desc);
> > +                               append_script(dir_fd, ent->d_name, desc, result, result_sz);
> >                         continue;
> >                 }
> >                 if (ent->d_type != DT_DIR) {
> > @@ -204,32 +223,31 @@ static void append_scripts_in_dir(int dir_fd)
> >                                 continue;
> >                 }
> >                 fd = openat(dir_fd, ent->d_name, O_PATH);
> > -               append_scripts_in_dir(fd);
> > +               append_scripts_in_dir(fd, result, result_sz);
> >         }
> >         for (i = 0; i < n_dirs; i++) /* Clean up */
> >                 zfree(&entlist[i]);
> >         free(entlist);
> >  }
> >
> > -const struct script_file *list_script_files(void)
> > +struct test_suite **create_script_test_suites(void)
> >  {
> > -       int dir_fd;
> > -
> > -       if (files)
> > -               return files; /* Singleton - we already know our list */
> > +       struct test_suite **result = NULL, **result_tmp;
> > +       size_t result_sz = 0;
> > +       int dir_fd = shell_tests__dir_fd(); /* Walk  dir */
> >
> > -       dir_fd = shell_tests__dir_fd(); /* Walk  dir */
> >         if (dir_fd < 0)
> >                 return NULL;
>
> When can it fail?  If the test directory doesn't exist?
> I'm afraid returning NULL here can crash perf test.
> I think you need to return a NULL entry result.

Good catch, will fix in v3.

Thanks,
Ian

> Thanks,
> Namhyung
>
>
> >
> > -       append_scripts_in_dir(dir_fd);
> > +       append_scripts_in_dir(dir_fd, &result, &result_sz);
> > +       result_tmp = realloc(result, (result_sz + 1) * sizeof(*result_tmp));
> > +       if (result_tmp == NULL) {
> > +               pr_err("Out of memory while building script test suite list\n");
> > +               abort();
> > +       }
> > +       /* NULL terminate the test suite array. */
> > +       result = result_tmp;
> > +       result[result_sz] = NULL;
> >         close(dir_fd);
> > -
> > -       return files;
> > -}
> > -
> > -int list_script_max_width(void)
> > -{
> > -       list_script_files(); /* Ensure we have scanned all scripts */
> > -       return files_max_width;
> > +       return result;
> >  }
> > diff --git a/tools/perf/tests/tests-scripts.h b/tools/perf/tests/tests-scripts.h
> > index 3508a293aaf9..b553ad26ea17 100644
> > --- a/tools/perf/tests/tests-scripts.h
> > +++ b/tools/perf/tests/tests-scripts.h
> > @@ -2,14 +2,8 @@
> >  #ifndef TESTS_SCRIPTS_H
> >  #define TESTS_SCRIPTS_H
> >
> > -struct script_file {
> > -       char *file;
> > -       char *desc;
> > -};
> > +#include "tests.h"
> >
> > -/* List available script tests to run - singleton - never freed */
> > -const struct script_file *list_script_files(void);
> > -/* Get maximum width of description string */
> > -int list_script_max_width(void);
> > +struct test_suite **create_script_test_suites(void);
> >
> >  #endif /* TESTS_SCRIPTS_H */
> > --
> > 2.43.0.429.g432eaa2c6b-goog
> >
  

Patch

diff --git a/tools/perf/tests/builtin-test.c b/tools/perf/tests/builtin-test.c
index 6d5001daaf63..c42cb40fc242 100644
--- a/tools/perf/tests/builtin-test.c
+++ b/tools/perf/tests/builtin-test.c
@@ -130,6 +130,7 @@  static struct test_suite *generic_tests[] = {
 static struct test_suite **tests[] = {
 	generic_tests,
 	arch_tests,
+	NULL, /* shell tests created at runtime. */
 };
 
 static struct test_workload *workloads[] = {
@@ -299,74 +300,12 @@  static int test_and_print(struct test_suite *t, int subtest)
 	return err;
 }
 
-struct shell_test {
-	const char *file;
-};
-
-static int shell_test__run(struct test_suite *test, int subdir __maybe_unused)
-{
-	int err;
-	struct shell_test *st = test->priv;
-	char *cmd;
-
-	asprintf(&cmd, "%s%s", st->file, verbose ? " -v" : "");
-	if (!cmd)
-		return TEST_FAIL;
-	err = system(cmd);
-	free(cmd);
-	if (!err)
-		return TEST_OK;
-
-	return WEXITSTATUS(err) == 2 ? TEST_SKIP : TEST_FAIL;
-}
-
-static int run_shell_tests(int argc, const char *argv[], int i, int width,
-				struct intlist *skiplist)
-{
-	struct shell_test st;
-	const struct script_file *files, *file;
-
-	files = list_script_files();
-	if (!files)
-		return 0;
-	for (file = files; file->file; file++) {
-		int curr = i++;
-		struct test_case test_cases[] = {
-			{
-				.desc = file->desc,
-				.run_case = shell_test__run,
-			},
-			{ .name = NULL, }
-		};
-		struct test_suite test_suite = {
-			.desc = test_cases[0].desc,
-			.test_cases = test_cases,
-			.priv = &st,
-		};
-		st.file = file->file;
-
-		if (test_suite.desc == NULL ||
-		    !perf_test__matches(test_suite.desc, curr, argc, argv))
-			continue;
-
-		pr_info("%3d: %-*s:", i, width, test_suite.desc);
-
-		if (intlist__find(skiplist, i)) {
-			color_fprintf(stderr, PERF_COLOR_YELLOW, " Skip (user override)\n");
-			continue;
-		}
-
-		test_and_print(&test_suite, 0);
-	}
-	return 0;
-}
-
 static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
 {
 	struct test_suite *t;
 	unsigned int j, k;
 	int i = 0;
-	int width = list_script_max_width();
+	int width = 0;
 
 	for_each_test(j, k, t) {
 		int len = strlen(test_description(t, -1));
@@ -441,28 +380,6 @@  static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
 			}
 		}
 	}
-
-	return run_shell_tests(argc, argv, i, width, skiplist);
-}
-
-static int perf_test__list_shell(int argc, const char **argv, int i)
-{
-	const struct script_file *files, *file;
-
-	files = list_script_files();
-	if (!files)
-		return 0;
-	for (file = files; file->file; file++) {
-		int curr = i++;
-		struct test_suite t = {
-			.desc = file->desc
-		};
-
-		if (!perf_test__matches(t.desc, curr, argc, argv))
-			continue;
-
-		pr_info("%3d: %s\n", i, t.desc);
-	}
 	return 0;
 }
 
@@ -489,9 +406,6 @@  static int perf_test__list(int argc, const char **argv)
 					test_description(t, subi));
 		}
 	}
-
-	perf_test__list_shell(argc, argv, i);
-
 	return 0;
 }
 
@@ -551,6 +465,7 @@  int cmd_test(int argc, const char **argv)
 	/* Unbuffered output */
 	setvbuf(stdout, NULL, _IONBF, 0);
 
+	tests[2] = create_script_test_suites();
 	argc = parse_options_subcommand(argc, argv, test_options, test_subcommands, test_usage, 0);
 	if (argc >= 1 && !strcmp(argv[0], "list"))
 		return perf_test__list(argc - 1, argv + 1);
diff --git a/tools/perf/tests/tests-scripts.c b/tools/perf/tests/tests-scripts.c
index 9b3b66dd5508..b92a93c251c6 100644
--- a/tools/perf/tests/tests-scripts.c
+++ b/tools/perf/tests/tests-scripts.c
@@ -26,16 +26,6 @@ 
 #include "tests.h"
 #include "util/rlimit.h"
 
-
-/*
- * As this is a singleton built once for the run of the process, there is
- * no value in trying to free it and just let it stay around until process
- * exits when it's cleaned up.
- */
-static size_t files_num = 0;
-static struct script_file *files = NULL;
-static int files_max_width = 0;
-
 static int shell_tests__dir_fd(void)
 {
 	char path[PATH_MAX], *exec_path;
@@ -131,12 +121,31 @@  static char *strdup_check(const char *str)
 	return newstr;
 }
 
-static void append_script(int dir_fd, const char *name, char *desc)
+static int shell_test__run(struct test_suite *test, int subtest __maybe_unused)
+{
+	const char *file = test->priv;
+	int err;
+	char *cmd;
+
+	asprintf(&cmd, "%s%s", file, verbose ? " -v" : "");
+	if (!cmd)
+		return TEST_FAIL;
+	err = system(cmd);
+	free(cmd);
+	if (!err)
+		return TEST_OK;
+
+	return WEXITSTATUS(err) == 2 ? TEST_SKIP : TEST_FAIL;
+}
+
+static void append_script(int dir_fd, const char *name, char *desc,
+			  struct test_suite ***result,
+			  size_t *result_sz)
 {
 	char filename[PATH_MAX], link[128];
-	struct script_file *files_tmp;
-	size_t files_num_tmp, len;
-	int width;
+	struct test_suite *test_suite, **result_tmp;
+	struct test_case *tests;
+	size_t len;
 
 	snprintf(link, sizeof(link), "/proc/%d/fd/%d", getpid(), dir_fd);
 	len = readlink(link, filename, sizeof(filename));
@@ -146,33 +155,43 @@  static void append_script(int dir_fd, const char *name, char *desc)
 	}
 	filename[len++] = '/';
 	strcpy(&filename[len], name);
-	files_num_tmp = files_num + 1;
-	if (files_num_tmp >= SIZE_MAX) {
-		pr_err("Too many script files\n");
-		abort();
+
+	tests = calloc(2, sizeof(*tests));
+	if (!tests) {
+		pr_err("Out of memory while building script test suite list\n");
+		return;
+	}
+	tests[0].name = strdup_check(name);
+	tests[0].desc = strdup_check(desc);
+	tests[0].run_case = shell_test__run;
+
+	test_suite = zalloc(sizeof(*test_suite));
+	if (!test_suite) {
+		pr_err("Out of memory while building script test suite list\n");
+		free(tests);
+		return;
 	}
+	test_suite->desc = desc;
+	test_suite->test_cases = tests;
+	test_suite->priv = strdup_check(filename);
 	/* Realloc is good enough, though we could realloc by chunks, not that
 	 * anyone will ever measure performance here */
-	files_tmp = realloc(files,
-			    (files_num_tmp + 1) * sizeof(struct script_file));
-	if (files_tmp == NULL) {
-		pr_err("Out of memory while building test list\n");
-		abort();
+	result_tmp = realloc(*result, (*result_sz + 1) * sizeof(*result_tmp));
+	if (result_tmp == NULL) {
+		pr_err("Out of memory while building script test suite list\n");
+		free(tests);
+		free(test_suite);
+		return;
 	}
 	/* Add file to end and NULL terminate the struct array */
-	files = files_tmp;
-	files_num = files_num_tmp;
-	files[files_num - 1].file = strdup_check(filename);
-	files[files_num - 1].desc = desc;
-	files[files_num].file = NULL;
-	files[files_num].desc = NULL;
-
-	width = strlen(desc); /* Track max width of desc */
-	if (width > files_max_width)
-		files_max_width = width;
+	*result = result_tmp;
+	(*result)[*result_sz] = test_suite;
+	(*result_sz)++;
 }
 
-static void append_scripts_in_dir(int dir_fd)
+static void append_scripts_in_dir(int dir_fd,
+				  struct test_suite ***result,
+				  size_t *result_sz)
 {
 	struct dirent **entlist;
 	struct dirent *ent;
@@ -191,7 +210,7 @@  static void append_scripts_in_dir(int dir_fd)
 			char *desc = shell_test__description(dir_fd, ent->d_name);
 
 			if (desc) /* It has a desc line - valid script */
-				append_script(dir_fd, ent->d_name, desc);
+				append_script(dir_fd, ent->d_name, desc, result, result_sz);
 			continue;
 		}
 		if (ent->d_type != DT_DIR) {
@@ -204,32 +223,31 @@  static void append_scripts_in_dir(int dir_fd)
 				continue;
 		}
 		fd = openat(dir_fd, ent->d_name, O_PATH);
-		append_scripts_in_dir(fd);
+		append_scripts_in_dir(fd, result, result_sz);
 	}
 	for (i = 0; i < n_dirs; i++) /* Clean up */
 		zfree(&entlist[i]);
 	free(entlist);
 }
 
-const struct script_file *list_script_files(void)
+struct test_suite **create_script_test_suites(void)
 {
-	int dir_fd;
-
-	if (files)
-		return files; /* Singleton - we already know our list */
+	struct test_suite **result = NULL, **result_tmp;
+	size_t result_sz = 0;
+	int dir_fd = shell_tests__dir_fd(); /* Walk  dir */
 
-	dir_fd = shell_tests__dir_fd(); /* Walk  dir */
 	if (dir_fd < 0)
 		return NULL;
 
-	append_scripts_in_dir(dir_fd);
+	append_scripts_in_dir(dir_fd, &result, &result_sz);
+	result_tmp = realloc(result, (result_sz + 1) * sizeof(*result_tmp));
+	if (result_tmp == NULL) {
+		pr_err("Out of memory while building script test suite list\n");
+		abort();
+	}
+	/* NULL terminate the test suite array. */
+	result = result_tmp;
+	result[result_sz] = NULL;
 	close(dir_fd);
-
-	return files;
-}
-
-int list_script_max_width(void)
-{
-	list_script_files(); /* Ensure we have scanned all scripts */
-	return files_max_width;
+	return result;
 }
diff --git a/tools/perf/tests/tests-scripts.h b/tools/perf/tests/tests-scripts.h
index 3508a293aaf9..b553ad26ea17 100644
--- a/tools/perf/tests/tests-scripts.h
+++ b/tools/perf/tests/tests-scripts.h
@@ -2,14 +2,8 @@ 
 #ifndef TESTS_SCRIPTS_H
 #define TESTS_SCRIPTS_H
 
-struct script_file {
-	char *file;
-	char *desc;
-};
+#include "tests.h"
 
-/* List available script tests to run - singleton - never freed */
-const struct script_file *list_script_files(void);
-/* Get maximum width of description string */
-int list_script_max_width(void);
+struct test_suite **create_script_test_suites(void);
 
 #endif /* TESTS_SCRIPTS_H */