diff mbox series

[v3,7/7] selftests/resctrl: Cleanup benchmark argument parsing

Message ID 20230823131556.27617-8-ilpo.jarvinen@linux.intel.com (mailing list archive)
State New
Headers show
Series selftests/resctrl: Rework benchmark command handling | expand

Commit Message

Ilpo Järvinen Aug. 23, 2023, 1:15 p.m. UTC 
Benchmark argument is handled by custom argument parsing code which is
more complicated than it needs to be.

Process benchmark argument within the normal getopt() handling and drop
entirely unnecessary ben_ind and has_ben variables. If -b is not given,
setup the default benchmark command right after the switch statement
and make -b to goto over it while it terminates the getopt() loop.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
---
 .../testing/selftests/resctrl/resctrl_tests.c | 71 ++++++++++---------
 1 file changed, 36 insertions(+), 35 deletions(-)

Comments

Maciej Wieczor-Retman Aug. 29, 2023, 12:48 p.m. UTC | #1 
Hi,

On 2023-08-23 at 16:15:56 +0300, Ilpo Järvinen wrote:
>Benchmark argument is handled by custom argument parsing code which is
>more complicated than it needs to be.
>
>Process benchmark argument within the normal getopt() handling and drop
>entirely unnecessary ben_ind and has_ben variables. If -b is not given,
>setup the default benchmark command right after the switch statement
>and make -b to goto over it while it terminates the getopt() loop.
>
>Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
>---
> .../testing/selftests/resctrl/resctrl_tests.c | 71 ++++++++++---------
> 1 file changed, 36 insertions(+), 35 deletions(-)
>
>diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
>index 94516d1f4307..ae9001ef7b0a 100644
>--- a/tools/testing/selftests/resctrl/resctrl_tests.c
>+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
>@@ -169,28 +169,35 @@ static void run_cat_test(int cpu_no, int no_of_bits)
> 
> int main(int argc, char **argv)
> {
>-	bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
>-	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
>+	bool mbm_test = true, mba_test = true, cmt_test = true;
>+	int c, cpu_no = 1, i, no_of_bits = 0;
> 	const char *benchmark_cmd[BENCHMARK_ARGS];
>-	int ben_ind, tests = 0;
> 	char *span_str = NULL;
> 	bool cat_test = true;
> 	char *skip_reason;
>+	int tests = 0;
> 	int ret;
> 
>-	for (i = 0; i < argc; i++) {
>-		if (strcmp(argv[i], "-b") == 0) {
>-			ben_ind = i + 1;
>-			argc_new = ben_ind - 1;
>-			has_ben = true;
>-			break;
>-		}
>-	}
>-
>-	while ((c = getopt(argc_new, argv, "ht:b:n:p:")) != -1) {
>+	while ((c = getopt(argc, argv, "ht:b:n:p:")) != -1) {
> 		char *token;
> 
> 		switch (c) {
>+		case 'b':
>+			/*
>+			 * First move optind back to the (first) optarg and
>+			 * then build the benchmark command using the
>+			 * remaining arguments.
>+			 */
>+			optind--;
>+			if (argc - optind >= BENCHMARK_ARGS - 1)
>+				ksft_exit_fail_msg("Too long benchmark command");

Isn't this condition off by two?

I did some testing and the maximum amount of benchmark arguments is 62
while the array of const char* has 64 spaces. Is it supposed to have
less than the maximum capacity?

Wouldn't something like this be more valid with BENCHMARK_ARGS equal to
64? :
			if (argc - optind > BENCHMARK_ARGS)
Ilpo Järvinen Aug. 29, 2023, 1:04 p.m. UTC | #2 
On Tue, 29 Aug 2023, Maciej Wieczór-Retman wrote:
> On 2023-08-23 at 16:15:56 +0300, Ilpo Järvinen wrote:
> >Benchmark argument is handled by custom argument parsing code which is
> >more complicated than it needs to be.
> >
> >Process benchmark argument within the normal getopt() handling and drop
> >entirely unnecessary ben_ind and has_ben variables. If -b is not given,
> >setup the default benchmark command right after the switch statement
> >and make -b to goto over it while it terminates the getopt() loop.
> >
> >Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> >---
> > .../testing/selftests/resctrl/resctrl_tests.c | 71 ++++++++++---------
> > 1 file changed, 36 insertions(+), 35 deletions(-)
> >
> >diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> >index 94516d1f4307..ae9001ef7b0a 100644
> >--- a/tools/testing/selftests/resctrl/resctrl_tests.c
> >+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> >@@ -169,28 +169,35 @@ static void run_cat_test(int cpu_no, int no_of_bits)
> > 
> > int main(int argc, char **argv)
> > {
> >-	bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
> >-	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
> >+	bool mbm_test = true, mba_test = true, cmt_test = true;
> >+	int c, cpu_no = 1, i, no_of_bits = 0;
> > 	const char *benchmark_cmd[BENCHMARK_ARGS];
> >-	int ben_ind, tests = 0;
> > 	char *span_str = NULL;
> > 	bool cat_test = true;
> > 	char *skip_reason;
> >+	int tests = 0;
> > 	int ret;
> > 
> >-	for (i = 0; i < argc; i++) {
> >-		if (strcmp(argv[i], "-b") == 0) {
> >-			ben_ind = i + 1;
> >-			argc_new = ben_ind - 1;
> >-			has_ben = true;
> >-			break;
> >-		}
> >-	}
> >-
> >-	while ((c = getopt(argc_new, argv, "ht:b:n:p:")) != -1) {
> >+	while ((c = getopt(argc, argv, "ht:b:n:p:")) != -1) {
> > 		char *token;
> > 
> > 		switch (c) {
> >+		case 'b':
> >+			/*
> >+			 * First move optind back to the (first) optarg and
> >+			 * then build the benchmark command using the
> >+			 * remaining arguments.
> >+			 */
> >+			optind--;
> >+			if (argc - optind >= BENCHMARK_ARGS - 1)
> >+				ksft_exit_fail_msg("Too long benchmark command");
> 
> Isn't this condition off by two?
> 
> I did some testing and the maximum amount of benchmark arguments is 62
> while the array of const char* has 64 spaces. Is it supposed to have
> less than the maximum capacity?
> 
> Wouldn't something like this be more valid with BENCHMARK_ARGS equal to
> 64? :
> 			if (argc - optind > BENCHMARK_ARGS)

Certainly not off by two as the array must be NULL terminated but it seems 
to be off-by-one (to the safe direction), yes.
Maciej Wieczor-Retman Aug. 29, 2023, 1:23 p.m. UTC | #3 
On 2023-08-29 at 16:04:29 +0300, Ilpo Järvinen wrote:
>On Tue, 29 Aug 2023, Maciej Wieczór-Retman wrote:
>> On 2023-08-23 at 16:15:56 +0300, Ilpo Järvinen wrote:
>> >Benchmark argument is handled by custom argument parsing code which is
>> >more complicated than it needs to be.
>> >
>> >Process benchmark argument within the normal getopt() handling and drop
>> >entirely unnecessary ben_ind and has_ben variables. If -b is not given,
>> >setup the default benchmark command right after the switch statement
>> >and make -b to goto over it while it terminates the getopt() loop.
>> >
>> >Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> >Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
>> >---
>> > .../testing/selftests/resctrl/resctrl_tests.c | 71 ++++++++++---------
>> > 1 file changed, 36 insertions(+), 35 deletions(-)
>> >
>> >diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
>> >index 94516d1f4307..ae9001ef7b0a 100644
>> >--- a/tools/testing/selftests/resctrl/resctrl_tests.c
>> >+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
>> >@@ -169,28 +169,35 @@ static void run_cat_test(int cpu_no, int no_of_bits)
>> > 
>> > int main(int argc, char **argv)
>> > {
>> >-	bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
>> >-	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
>> >+	bool mbm_test = true, mba_test = true, cmt_test = true;
>> >+	int c, cpu_no = 1, i, no_of_bits = 0;
>> > 	const char *benchmark_cmd[BENCHMARK_ARGS];
>> >-	int ben_ind, tests = 0;
>> > 	char *span_str = NULL;
>> > 	bool cat_test = true;
>> > 	char *skip_reason;
>> >+	int tests = 0;
>> > 	int ret;
>> > 
>> >-	for (i = 0; i < argc; i++) {
>> >-		if (strcmp(argv[i], "-b") == 0) {
>> >-			ben_ind = i + 1;
>> >-			argc_new = ben_ind - 1;
>> >-			has_ben = true;
>> >-			break;
>> >-		}
>> >-	}
>> >-
>> >-	while ((c = getopt(argc_new, argv, "ht:b:n:p:")) != -1) {
>> >+	while ((c = getopt(argc, argv, "ht:b:n:p:")) != -1) {
>> > 		char *token;
>> > 
>> > 		switch (c) {
>> >+		case 'b':
>> >+			/*
>> >+			 * First move optind back to the (first) optarg and
>> >+			 * then build the benchmark command using the
>> >+			 * remaining arguments.
>> >+			 */
>> >+			optind--;
>> >+			if (argc - optind >= BENCHMARK_ARGS - 1)
>> >+				ksft_exit_fail_msg("Too long benchmark command");
>> 
>> Isn't this condition off by two?
>> 
>> I did some testing and the maximum amount of benchmark arguments is 62
>> while the array of const char* has 64 spaces. Is it supposed to have
>> less than the maximum capacity?
>> 
>> Wouldn't something like this be more valid with BENCHMARK_ARGS equal to
>> 64? :
>> 			if (argc - optind > BENCHMARK_ARGS)
>
>Certainly not off by two as the array must be NULL terminated but it seems 
>to be off-by-one (to the safe direction), yes.

Sorry, yes, off by one, now I can see the NULL just after the loop.
diff mbox series

Patch

diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 94516d1f4307..ae9001ef7b0a 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -169,28 +169,35 @@  static void run_cat_test(int cpu_no, int no_of_bits)
 
 int main(int argc, char **argv)
 {
-	bool has_ben = false, mbm_test = true, mba_test = true, cmt_test = true;
-	int c, cpu_no = 1, argc_new = argc, i, no_of_bits = 0;
+	bool mbm_test = true, mba_test = true, cmt_test = true;
+	int c, cpu_no = 1, i, no_of_bits = 0;
 	const char *benchmark_cmd[BENCHMARK_ARGS];
-	int ben_ind, tests = 0;
 	char *span_str = NULL;
 	bool cat_test = true;
 	char *skip_reason;
+	int tests = 0;
 	int ret;
 
-	for (i = 0; i < argc; i++) {
-		if (strcmp(argv[i], "-b") == 0) {
-			ben_ind = i + 1;
-			argc_new = ben_ind - 1;
-			has_ben = true;
-			break;
-		}
-	}
-
-	while ((c = getopt(argc_new, argv, "ht:b:n:p:")) != -1) {
+	while ((c = getopt(argc, argv, "ht:b:n:p:")) != -1) {
 		char *token;
 
 		switch (c) {
+		case 'b':
+			/*
+			 * First move optind back to the (first) optarg and
+			 * then build the benchmark command using the
+			 * remaining arguments.
+			 */
+			optind--;
+			if (argc - optind >= BENCHMARK_ARGS - 1)
+				ksft_exit_fail_msg("Too long benchmark command");
+
+			/* Extract benchmark command from command line. */
+			for (i = 0; i < argc - optind; i++)
+				benchmark_cmd[i] = argv[i + optind];
+			benchmark_cmd[i] = NULL;
+
+			goto last_arg;
 		case 't':
 			token = strtok(optarg, ",");
 
@@ -240,6 +247,19 @@  int main(int argc, char **argv)
 		}
 	}
 
+	/* If no benchmark is given by "-b" argument, use fill_buf. */
+	benchmark_cmd[0] = "fill_buf";
+	ret = asprintf(&span_str, "%u", DEFAULT_SPAN);
+	if (ret < 0)
+		ksft_exit_fail_msg("Out of memory!\n");
+	benchmark_cmd[1] = span_str;
+	benchmark_cmd[2] = "1";
+	benchmark_cmd[3] = "0";
+	benchmark_cmd[4] = "false";
+	benchmark_cmd[5] = NULL;
+
+last_arg:
+
 	ksft_print_header();
 
 	/*
@@ -247,28 +267,9 @@  int main(int argc, char **argv)
 	 * 1. We write to resctrl FS
 	 * 2. We execute perf commands
 	 */
-	if (geteuid() != 0)
-		return ksft_exit_skip("Not running as root. Skipping...\n");
-
-	if (has_ben) {
-		if (argc - ben_ind >= BENCHMARK_ARGS - 1)
-			ksft_exit_fail_msg("Too long benchmark command.\n");
-
-		/* Extract benchmark command from command line. */
-		for (i = 0; i < argc - ben_ind; i++)
-			benchmark_cmd[i] = argv[i + ben_ind];
-		benchmark_cmd[i] = NULL;
-	} else {
-		/* If no benchmark is given by "-b" argument, use fill_buf. */
-		benchmark_cmd[0] = "fill_buf";
-		ret = asprintf(&span_str, "%u", DEFAULT_SPAN);
-		if (ret < 0)
-			ksft_exit_fail_msg("Out of memory!\n");
-		benchmark_cmd[1] = span_str;
-		benchmark_cmd[2] = "1";
-		benchmark_cmd[3] = "0";
-		benchmark_cmd[4] = "false";
-		benchmark_cmd[5] = NULL;
+	if (geteuid() != 0) {
+		skip_reason = "Not running as root. Skipping...\n";
+		goto free_span;
 	}
 
 	if (!check_resctrlfs_support()) {