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

From: Leo Yan <leo.yan@linaro.org>
To: carsten.haitzler@foss.arm.com
Cc: linux-kernel@vger.kernel.org, coresight@lists.linaro.org,
	suzuki.poulose@arm.com, mathieu.poirier@linaro.org,
	mike.leach@linaro.org, linux-perf-users@vger.kernel.org,
	acme@kernel.org
Subject: Re: [PATCH v5 01/14] perf test: Refactor shell tests allowing subdirs
Date: Sat, 6 Aug 2022 16:37:12 +0800	[thread overview]
Message-ID: <20220806083712.GA124146@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <20220728145256.2985298-2-carsten.haitzler@foss.arm.com>

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