Re: [PATCH bpf-next 2/2] selftests/bpf: test_progs can read test lists from file

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 4/25/23 3:54 PM, Stephen Veiss wrote:
Improve test selection logic when using -a/-b/-d/-t options.
The list of tests to include or exclude can now be read from a file,
specified as @<filename>.

The file contains one name (or wildcard pattern) per line, and
comments beginning with # are ignored.

These options can be passed multiple times to read more than one file.

Signed-off-by: Stephen Veiss <sveiss@xxxxxxxx>

LGTM. A few nits below.

---
  .../selftests/bpf/prog_tests/arg_parsing.c    | 50 +++++++++++++++++++
  tools/testing/selftests/bpf/test_progs.c      | 39 +++++++++++----
  tools/testing/selftests/bpf/testing_helpers.c | 49 ++++++++++++++++++
  tools/testing/selftests/bpf/testing_helpers.h |  3 ++
  4 files changed, 132 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/arg_parsing.c b/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
index 3754cd5f8c0a..e0c6ef2dda70 100644
--- a/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
+++ b/tools/testing/selftests/bpf/prog_tests/arg_parsing.c
@@ -113,8 +113,58 @@ static void test_parse_test_list(void)
  	free_test_filter_set(&set);
  }
+static void test_parse_test_list_file(void)
+{
+	char tmpfile[80];
+	int fd;
+	FILE *fp;
+	struct test_filter_set set;
+
+	snprintf(tmpfile, sizeof(tmpfile), "/tmp/bpf_arg_parsing_test.XXXXXX");
+	fd = mkstemp(tmpfile);
+	ASSERT_GE(fd, 0, "create tmp");

If ASSERT_GE(...) is not true, we should simply return.

+
+	fp = fdopen(fd, "w");

We should check whether fp is NULL or not, if it is we should close fd and return (basically go to 'error').

+
+	fprintf(fp, "# comment\n");
+	fprintf(fp, "  test_with_spaces    \n");
+	fprintf(fp, "testA/subtest    # comment\n");
+	fprintf(fp, "testB#comment with no space\n");
+	fprintf(fp, "testB # duplicate\n");
+	fprintf(fp, "testA/subtest # subtest duplicate\n");
+	fprintf(fp, "testA/subtest2\n");
+	fprintf(fp, "testC_no_eof_newline");
+
+	if (!ASSERT_OK(ferror(fp), "prepare tmp"))
+		goto error;
+
+	if (!ASSERT_OK(fclose(fp), "close tmp"))
+		goto error;
+
+	init_test_filter_set(&set);
+
+	ASSERT_OK(parse_test_list_file(tmpfile, &set, true), "parse file");
+
+	ASSERT_EQ(set.cnt, 4, "test  count");
+	ASSERT_OK(strcmp("test_with_spaces", set.tests[0].name), "test 0 name");
+	ASSERT_EQ(set.tests[0].subtest_cnt, 0, "test 0 subtest count");
+	ASSERT_OK(strcmp("testA", set.tests[1].name), "test 1 name");
+	ASSERT_EQ(set.tests[1].subtest_cnt, 2, "test 1 subtest count");
+	ASSERT_OK(strcmp("subtest", set.tests[1].subtests[0]), "test 1 subtest 0");
+	ASSERT_OK(strcmp("subtest2", set.tests[1].subtests[1]), "test 1 subtest 1");
+	ASSERT_OK(strcmp("testB", set.tests[2].name), "test 2 name");
+	ASSERT_OK(strcmp("testC_no_eof_newline", set.tests[3].name), "test 3 name");
+
+	free_test_filter_set(&set);
+
+error:
+	remove(tmpfile);

Maybe do 'close(fd)' instead which is more intuitive?

+}
+
  void test_arg_parsing(void)
  {
  	if (test__start_subtest("test_parse_test_list"))
  		test_parse_test_list();
+	if (test__start_subtest("test_parse_test_list_file"))
+		test_parse_test_list_file();
  }
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index ea82921110da..cf80d28c76e8 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -714,7 +714,13 @@ static struct test_state test_states[ARRAY_SIZE(prog_test_defs)];
const char *argp_program_version = "test_progs 0.1";
  const char *argp_program_bug_address = "<bpf@xxxxxxxxxxxxxxx>";
-static const char argp_program_doc[] = "BPF selftests test runner";
+static const char argp_program_doc[] =
+"BPF selftests test runner\v"

What does it mean to use "\v" here?

+"Options accepting the NAMES parameter take either a comma-separated list\n"
+"of test names, or a filename prefixed with @. The file contains one name\n"
+"(or wildcard pattern) per line, and comments beginning with # are ignored.\n"
+"\n"
+"These options can be passed repeatedly to read multiple files.\n";
enum ARG_KEYS {
  	ARG_TEST_NUM = 'n',
@@ -797,6 +803,7 @@ extern int extra_prog_load_log_flags;
  static error_t parse_arg(int key, char *arg, struct argp_state *state)
  {
  	struct test_env *env = state->input;
+	int err;

Maybe initialize 'err = 0' and change later 'return 0' to 'return err',
so we don't need 'if (err) return err' below?

switch (key) {
  	case ARG_TEST_NUM: {
@@ -821,18 +828,32 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
  	}
  	case ARG_TEST_NAME_GLOB_ALLOWLIST:
  	case ARG_TEST_NAME: {
-		if (parse_test_list(arg,
-				    &env->test_selector.whitelist,
-				    key == ARG_TEST_NAME_GLOB_ALLOWLIST))
-			return -ENOMEM;
+		if (arg[0] == '@')
+			err = parse_test_list_file(arg + 1,
+						   &env->test_selector.whitelist,
+						   key == ARG_TEST_NAME_GLOB_ALLOWLIST);
+		else
+			err = parse_test_list(arg,
+					      &env->test_selector.whitelist,
+					      key == ARG_TEST_NAME_GLOB_ALLOWLIST);
+
+		if (err)
+			return err;
  		break;
  	}
  	case ARG_TEST_NAME_GLOB_DENYLIST:
  	case ARG_TEST_NAME_BLACKLIST: {
-		if (parse_test_list(arg,
-				    &env->test_selector.blacklist,
-				    key == ARG_TEST_NAME_GLOB_DENYLIST))
-			return -ENOMEM;
+		if (arg[0] == '@')
+			err = parse_test_list_file(arg + 1,
+						   &env->test_selector.blacklist,
+						   key == ARG_TEST_NAME_GLOB_DENYLIST);
+		else
+			err = parse_test_list(arg,
+					      &env->test_selector.blacklist,
+					      key == ARG_TEST_NAME_GLOB_DENYLIST);
+
+		if (err)
+			return err;
  		break;
  	}
  	case ARG_VERIFIER_STATS:
diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
index 14322371e1d8..d8bea5c2a4d6 100644
--- a/tools/testing/selftests/bpf/testing_helpers.c
+++ b/tools/testing/selftests/bpf/testing_helpers.c
@@ -1,6 +1,7 @@
  // SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
  /* Copyright (C) 2019 Netronome Systems, Inc. */
  /* Copyright (C) 2020 Facebook, Inc. */
+#include <ctype.h>
  #include <stdlib.h>
  #include <string.h>
  #include <errno.h>
@@ -167,6 +168,54 @@ static int insert_test(struct test_filter_set *set,
  	return -ENOMEM;
  }
+int parse_test_list_file(const char *path,
+			 struct test_filter_set *set,
+			 bool is_glob_pattern)
+{
+	FILE *f;
+	size_t buflen = 0;
+	char *buf = NULL, *capture_start, *capture_end, *scan_end;
+	int err;
+
+	f = fopen(path, "r");
+	if (!f) {
+		err = -errno;
+		fprintf(stderr, "Failed to open '%s': %d\n", path, err);
+		return err;
+	}
+
+	while (getline(&buf, &buflen, f) != -1) {
+		capture_start = buf;
+
+		while (isspace(*capture_start))
+			++capture_start;
+
+		capture_end = capture_start;
+		scan_end = capture_start;
+
+		while (*scan_end && *scan_end != '#') {
+			if (!isspace(*scan_end))
+				capture_end = scan_end;
+
+			++scan_end;
+		}
+
+		if (capture_end == capture_start)
+			continue;
+
+		*(++capture_end) = '\0';
+
+		err = insert_test(set, capture_start, is_glob_pattern);
+		if (err) {
+			fclose(f);
+			return err;

Set initial err = 0 and later 'return 0' => 'return err', so we
just do break here to avoid calling fclose(f) in two different
places?

+		}
+	}
+
+	fclose(f);
+	return 0;
+}
+
  int parse_test_list(const char *s,
  		    struct test_filter_set *set,
  		    bool is_glob_pattern)
diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
index eb8790f928e4..98f09bbae86f 100644
--- a/tools/testing/selftests/bpf/testing_helpers.h
+++ b/tools/testing/selftests/bpf/testing_helpers.h
@@ -20,5 +20,8 @@ struct test_filter_set;
  int parse_test_list(const char *s,
  		    struct test_filter_set *test_set,
  		    bool is_glob_pattern);
+int parse_test_list_file(const char *path,
+			 struct test_filter_set *test_set,
+			 bool is_glob_pattern);
__u64 read_perf_max_sample_freq(void);



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux