lkml.org 
[lkml]   [2022]   [Aug]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 01/14] perf test: Refactor shell tests allowing subdirs
On Thu, Jul 28, 2022 at 03:52:43PM +0100, carsten.haitzler@foss.arm.com wrote:

[...]

> +int list_script_max_width(void)
> +{
> + list_script_files(); /* Ensure we have scanned all scriptd */

s/scriptd/scripts/

> + return files_max_width;
> +}

[...]

> struct shell_test {
> const char *dir;
> const char *file;
> @@ -385,33 +302,17 @@ static int shell_test__run(struct test_suite *test, int subdir __maybe_unused)
> static int run_shell_tests(int argc, const char *argv[], int i, int width,
> struct intlist *skiplist)
> {
> - struct dirent **entlist;
> - struct dirent *ent;
> - int n_dirs, e;
> - char path_dir[PATH_MAX];
> - struct shell_test st = {
> - .dir = shell_tests__dir(path_dir, sizeof(path_dir)),
> - };
> -
> - if (st.dir == NULL)
> - return -1;
> + struct shell_test st;
> + const struct script_file *files, *file;
>
> - n_dirs = scandir(st.dir, &entlist, NULL, alphasort);
> - if (n_dirs == -1) {
> - pr_err("failed to open shell test directory: %s\n",
> - st.dir);
> - return -1;
> - }
> -
> - for_each_shell_test(entlist, n_dirs, st.dir, ent) {
> + files = list_script_files();
> + if (!files)
> + return 0;
> + for (file = files; file->dir; file++) {
> int curr = i++;
> - char desc[256];
> struct test_case test_cases[] = {
> {
> - .desc = shell_test__description(desc,
> - sizeof(desc),
> - st.dir,
> - ent->d_name),
> + .desc = file->desc,
> .run_case = shell_test__run,
> },
> { .name = NULL, }
> @@ -421,12 +322,13 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width,
> .test_cases = test_cases,
> .priv = &st,
> };
> + st.dir = file->dir;
>
> if (test_suite.desc == NULL ||
> !perf_test__matches(test_suite.desc, curr, argc, argv))
> continue;
>
> - st.file = ent->d_name;
> + st.file = file->file;

I am just wandering if we can remove "st" in this function, finally I
found you are right, the "st" (struct shell_test) will be used in the
function shell_test__run(), so let's keep as it is.

> pr_info("%3d: %-*s:", i, width, test_suite.desc);
>
> if (intlist__find(skiplist, i)) {
> @@ -436,10 +338,6 @@ static int run_shell_tests(int argc, const char *argv[], int i, int width,
>
> test_and_print(&test_suite, 0);
> }
> -
> - for (e = 0; e < n_dirs; e++)
> - zfree(&entlist[e]);
> - free(entlist);
> return 0;
> }
>
> @@ -448,7 +346,7 @@ 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 = shell_tests__max_desc_width();
> + int width = list_script_max_width();
>
> for_each_test(j, k, t) {
> int len = strlen(test_description(t, -1));
> @@ -529,36 +427,22 @@ static int __cmd_test(int argc, const char *argv[], struct intlist *skiplist)
>
> static int perf_test__list_shell(int argc, const char **argv, int i)
> {
> - struct dirent **entlist;
> - struct dirent *ent;
> - int n_dirs, e;
> - char path_dir[PATH_MAX];
> - const char *path = shell_tests__dir(path_dir, sizeof(path_dir));
> -
> - if (path == NULL)
> - return -1;
> + const struct script_file *files, *file;
>
> - n_dirs = scandir(path, &entlist, NULL, alphasort);
> - if (n_dirs == -1)
> - return -1;
> -
> - for_each_shell_test(entlist, n_dirs, path, ent) {
> + files = list_script_files();
> + if (!files)
> + return 0;
> + for (file = files; file->dir; file++) {
> int curr = i++;
> - char bf[256];
> struct test_suite t = {
> - .desc = shell_test__description(bf, sizeof(bf), path, ent->d_name),
> + .desc = file->desc
> };
>
> if (!perf_test__matches(t.desc, curr, argc, argv))
> continue;
>
> pr_info("%3d: %s\n", i, t.desc);
> -
> }
> -
> - for (e = 0; e < n_dirs; e++)
> - zfree(&entlist[e]);
> - free(entlist);
> return 0;
> }

Except a minor typo, the patch looks good to me, it's a good
refactoring and enhancement for shell script testing.

I reviewed the change one by one line, at least I cannot find any logic
error.

With typo fixing:

Reviewed-by: Leo Yan <leo.yan@linaro.org>

I'd leave this patch for maintainers to review it. Just a caveat, given
it's a big patch, as Carsten replied it's good that take the patch as a
total new code for searching shell scripts, this would be easier for
understanding the change.

Thanks,
Leo

\
 
 \ /
  Last update: 2022-08-06 10:38    [W:1.920 / U:0.568 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site