Re: [PATCH v5 01/14] perf test: Refactor shell tests allowing subdirs

[Carsten Haitzler]

On 8/6/22 09:37, Leo Yan wrote:
> On Thu, Jul 28, 2022 at 03:52:43PM +0100, carsten.haitzler@xxxxxxxxxxxx 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@xxxxxxxxxx>
>
> 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