Messages in this thread | | | Date | Wed, 10 Aug 2022 09:38:45 +0100 | Subject | Re: [PATCH v5 01/14] perf test: Refactor shell tests allowing subdirs | From | Carsten Haitzler <> |
| |
On 8/6/22 09:37, Leo Yan wrote: > 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/
oops. fixed. v6 will come with that.
>> + 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
| |