| Message ID | 20220405204158.2496618-1-mykolal@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] selftests/bpf: Improve by-name subtest selection logic in prog_tests | expand |
On Tue, Apr 5, 2022 at 1:45 PM Mykola Lysenko <mykolal@fb.com> wrote: > > Improve subtest selection logic when using -t/-a/-d parameters. > In particular, more than one subtest can be specified or a > combination of tests / subtests. > > -a send_signal -d send_signal/send_signal_nmi* - runs send_signal > test without nmi tests > > -a send_signal/send_signal_nmi*,find_vma - runs two send_signal > subtests and find_vma test > Only somewhat related, but while we are at this topic. Can you please check whether equivalent approach works: -a send_signal/send_signal_nmi* -a find_vma i.e., multi -a/-d/-t/-b are concatenating their selectors. > This will allow us to have granular control over which subtests > to disable in the CI system instead of disabling whole tests. > > Also, add new selftest to avoid possible regression when > changing prog_test test name selection logic. > > Signed-off-by: Mykola Lysenko <mykolal@fb.com> > --- > .../selftests/bpf/prog_tests/arg_parsing.c | 88 ++++++++++ > tools/testing/selftests/bpf/test_progs.c | 157 +++++++++--------- > tools/testing/selftests/bpf/test_progs.h | 16 +- > tools/testing/selftests/bpf/testing_helpers.c | 87 ++++++++++ > tools/testing/selftests/bpf/testing_helpers.h | 6 + > 5 files changed, 266 insertions(+), 88 deletions(-) > create mode 100644 tools/testing/selftests/bpf/prog_tests/arg_parsing.c > [...] > +static void test_parse_test_list(void) > +{ > + struct test_set test_set; > + > + init_test_set(&test_set); > + > + parse_test_list("arg_parsing", &test_set, true); ASSER_OK() return value? > + if (CHECK(test_set.cnt != 1, "parse_test_list subtest argument", "Unexpected number of tests in num table %d\n", test_set.cnt)) please don't use CHECK()s in new tests, we only add ASSERT_xxx() now. Also line length should be under 100 characters (except for the case when we have a long string literal, we don't break string literals, no matter how long). > + goto error; > + if (!ASSERT_OK_PTR(test_set.tests, "tests__initialized")) > + goto error; > + if (CHECK(!test_set.tests[0].whole_test, "parse_test_list subtest argument", "Expected test 0 to be initialized")) > + goto error; > + if (CHECK(strcmp("arg_parsing", test_set.tests[0].name), "parse_test_list subtest argument", "Expected test 0 to be initialized")) > + goto error; > + free_test_set(&test_set); > + > + parse_test_list("arg_parsing,bpf_cookie", &test_set, true); > + if (CHECK(test_set.cnt != 2, "parse_test_list subtest argument", "Unexpected number of tests in num table %d\n", test_set.cnt)) > + goto error; > + if (!ASSERT_OK_PTR(test_set.tests, "tests__initialized")) > + goto error; > + if (CHECK(!test_set.tests[0].whole_test, "parse_test_list subtest argument", "Expected test 0 to be fully runnable")) > + goto error; > + if (CHECK(!test_set.tests[1].whole_test, "parse_test_list subtest argument", "Expected test 1 to be fully runnable")) > + goto error; nit: I think there is no need to goto error after each check. We need goto error if subsequent checks can crash due to some invalid state. In this case, validating the value of few independent values is ok to always check without goto error jumps. Makes tests shorter and simpler. > + if (CHECK(strcmp("arg_parsing", test_set.tests[0].name), "parse_test_list subtest argument", "Expected test 0 to be arg_parsing")) > + goto error; > + if (CHECK(strcmp("bpf_cookie", test_set.tests[1].name), "parse_test_list subtest argument", "Expected test 1 to be bpf_cookie")) > + goto error; > + free_test_set(&test_set); > + > + parse_test_list("arg_parsing/test_parse_test_list,bpf_cookie", &test_set, true); > + if (CHECK(test_set.cnt != 2, "parse_test_list subtest argument", "Unexpected number of tests in num table %d\n", test_set.cnt)) > + goto error; [...] > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > index 0a4b45d7b515..671e37cada4b 100644 > --- a/tools/testing/selftests/bpf/test_progs.c > +++ b/tools/testing/selftests/bpf/test_progs.c > @@ -3,6 +3,7 @@ > */ > #define _GNU_SOURCE > #include "test_progs.h" > +#include "testing_helpers.h" > #include "cgroup_helpers.h" > #include <argp.h> > #include <pthread.h> > @@ -82,14 +83,14 @@ int usleep(useconds_t usec) > static bool should_run(struct test_selector *sel, int num, const char *name) > { > int i; > - keep empty line between variable declaration block and first statement > for (i = 0; i < sel->blacklist.cnt; i++) { > - if (glob_match(name, sel->blacklist.strs[i])) > + if (glob_match(name, sel->blacklist.tests[i].name) && > + sel->blacklist.tests[i].whole_test) > return false; > } > > for (i = 0; i < sel->whitelist.cnt; i++) { > - if (glob_match(name, sel->whitelist.strs[i])) > + if (glob_match(name, sel->whitelist.tests[i].name)) > return true; > } > [...] > -bool test__start_subtest(const char *name) > +bool test__start_subtest(const char *subtest_name) > { > struct prog_test_def *test = env.test; > > @@ -205,17 +246,21 @@ bool test__start_subtest(const char *name) > > test->subtest_num++; > > - if (!name || !name[0]) { > + if (!subtest_name || !subtest_name[0]) { > fprintf(env.stderr, > "Subtest #%d didn't provide sub-test name!\n", > test->subtest_num); > return false; > } > > - if (!should_run(&env.subtest_selector, test->subtest_num, name)) > + if (!should_run_subtest(&env.test_selector, > + &env.subtest_selector, we don't have subtest_selector anymore, do we? also, do you think that maybe combining should_run and should_rub_subtest would be a good idea? You can distinguish by having subtest_name NULL when you need to check only test? > + test->subtest_num, > + test->test_name, > + subtest_name)) > return false; > > - test->subtest_name = strdup(name); > + test->subtest_name = strdup(subtest_name); > if (!test->subtest_name) { > fprintf(env.stderr, > "Subtest #%d: failed to copy subtest name!\n", > @@ -527,63 +572,29 @@ static int libbpf_print_fn(enum libbpf_print_level level, > return 0; > } > [...] > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h > index eec4c7385b14..6a465a98341e 100644 > --- a/tools/testing/selftests/bpf/test_progs.h > +++ b/tools/testing/selftests/bpf/test_progs.h > @@ -37,7 +37,6 @@ typedef __u16 __sum16; > #include <bpf/bpf_endian.h> > #include "trace_helpers.h" > #include "testing_helpers.h" > -#include "flow_dissector_load.h" > > enum verbosity { > VERBOSE_NONE, > @@ -46,14 +45,21 @@ enum verbosity { > VERBOSE_SUPER, > }; > > -struct str_set { > - const char **strs; > +struct prog_test { > + char *name; > + char **subtests; > + int subtest_cnt; > + bool whole_test; > +}; > + > +struct test_set { > + struct prog_test *tests; prog_test is a bad name, IMO. it's a "test filter", right? Let's call it that? test_set isn't most accurate as well, maybe test_filter_set or test_set_filter? > int cnt; > }; > > struct test_selector { > - struct str_set whitelist; > - struct str_set blacklist; > + struct test_set whitelist; > + struct test_set blacklist; > bool *num_set; > int num_set_len; > }; > diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c > index 795b6798ccee..d2160d2a1303 100644 > --- a/tools/testing/selftests/bpf/testing_helpers.c > +++ b/tools/testing/selftests/bpf/testing_helpers.c > @@ -6,6 +6,7 @@ > #include <errno.h> > #include <bpf/bpf.h> > #include <bpf/libbpf.h> > +#include "test_progs.h" > #include "testing_helpers.h" > > int parse_num_list(const char *s, bool **num_set, int *num_set_len) > @@ -69,6 +70,92 @@ int parse_num_list(const char *s, bool **num_set, int *num_set_len) > return 0; > } > > +int parse_test_list(const char *s, > + struct test_set *test_set, > + bool is_glob_pattern) > +{ > + char *input, *state = NULL, *next; > + struct prog_test *tmp, *tests = NULL; > + int i, j, cnt = 0; > + > + input = strdup(s); > + if (!input) > + return -ENOMEM; > + > + while ((next = strtok_r(state ? NULL : input, ",", &state))) { > + char *subtest_str = strchr(next, '/'); > + char *pattern = NULL; > + > + tmp = realloc(tests, sizeof(*tests) * (cnt + 1)); > + if (!tmp) > + goto err; > + tests = tmp; > + > + tests[cnt].subtest_cnt = 0; > + tests[cnt].subtests = NULL; > + tests[cnt].whole_test = false; > + > + if (subtest_str) { > + char **tmp_subtests = NULL; need an empty line between variable declarations and statements > + *subtest_str = '\0'; > + int subtest_cnt = tests[cnt].subtest_cnt; > + > + tmp_subtests = realloc(tests[cnt].subtests, > + sizeof(*tmp_subtests) * > + (subtest_cnt + 1)); > + if (!tmp_subtests) > + goto err; > + tests[cnt].subtests = tmp_subtests; > + > + tests[cnt].subtests[subtest_cnt] = strdup(subtest_str + 1); > + if (!tests[cnt].subtests[subtest_cnt]) > + goto err; > + > + tests[cnt].subtest_cnt++; > + } else { > + tests[cnt].whole_test = true; isn't whole_test equivalent to subtest_cnt > 0? Why keeping extra bool of state then? > + } > + > + if (is_glob_pattern) { > + pattern = "%s"; > + tests[cnt].name = malloc(strlen(next) + 1); > + } else { > + pattern = "*%s*"; > + tests[cnt].name = malloc(strlen(next) + 2 + 1); > + } > + > + if (!tests[cnt].name) > + goto err; > + > + sprintf(tests[cnt].name, pattern, next); > + > + cnt++; > + } > + > + tmp = realloc(test_set->tests, sizeof(*tests) * (cnt + test_set->cnt)); > + if (!tmp) > + goto err; > + > + memcpy(tmp + test_set->cnt, tests, sizeof(*tests) * cnt); > + test_set->tests = tmp; > + test_set->cnt += cnt; > + > + free(tests); > + free(input); > + return 0; > + > +err: > + for (i = 0; i < cnt; i++) { > + for (j = 0; j < tests[i].subtest_cnt; j++) > + free(tests[i].subtests[j]); > + > + free(tests[i].name); > + } > + free(tests); > + free(input); > + return -ENOMEM; > +} > + > __u32 link_info_prog_id(const struct bpf_link *link, struct bpf_link_info *info) > { > __u32 info_len = sizeof(*info); > diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h > index f46ebc476ee8..d2f502184cd1 100644 > --- a/tools/testing/selftests/bpf/testing_helpers.h > +++ b/tools/testing/selftests/bpf/testing_helpers.h > @@ -12,3 +12,9 @@ int bpf_test_load_program(enum bpf_prog_type type, const struct bpf_insn *insns, > size_t insns_cnt, const char *license, > __u32 kern_version, char *log_buf, > size_t log_buf_sz); > + > +/* > + * below function is exported for testing in prog_test test > + */ > +struct test_set; > +int parse_test_list(const char *s, struct test_set *test_set, bool is_glob_pattern); > -- > 2.30.2 >
Thanks for the super quick review Andrii! > On Apr 5, 2022, at 4:14 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Apr 5, 2022 at 1:45 PM Mykola Lysenko <mykolal@fb.com> wrote: >> >> Improve subtest selection logic when using -t/-a/-d parameters. >> In particular, more than one subtest can be specified or a >> combination of tests / subtests. >> >> -a send_signal -d send_signal/send_signal_nmi* - runs send_signal >> test without nmi tests >> >> -a send_signal/send_signal_nmi*,find_vma - runs two send_signal >> subtests and find_vma test >> > > Only somewhat related, but while we are at this topic. Can you please > check whether equivalent approach works: > > -a send_signal/send_signal_nmi* -a find_vma > > i.e., multi -a/-d/-t/-b are concatenating their selectors. Yes, these options can be combined. I will add a test for the sequence of parse_test_list calls in v2. > >> This will allow us to have granular control over which subtests >> to disable in the CI system instead of disabling whole tests. >> >> Also, add new selftest to avoid possible regression when >> changing prog_test test name selection logic. >> >> Signed-off-by: Mykola Lysenko <mykolal@fb.com> >> --- >> .../selftests/bpf/prog_tests/arg_parsing.c | 88 ++++++++++ >> tools/testing/selftests/bpf/test_progs.c | 157 +++++++++--------- >> tools/testing/selftests/bpf/test_progs.h | 16 +- >> tools/testing/selftests/bpf/testing_helpers.c | 87 ++++++++++ >> tools/testing/selftests/bpf/testing_helpers.h | 6 + >> 5 files changed, 266 insertions(+), 88 deletions(-) >> create mode 100644 tools/testing/selftests/bpf/prog_tests/arg_parsing.c >> > > [...] > >> +static void test_parse_test_list(void) >> +{ >> + struct test_set test_set; >> + >> + init_test_set(&test_set); >> + >> + parse_test_list("arg_parsing", &test_set, true); > > ASSER_OK() return value? Will fix > >> + if (CHECK(test_set.cnt != 1, "parse_test_list subtest argument", "Unexpected number of tests in num table %d\n", test_set.cnt)) > > please don't use CHECK()s in new tests, we only add ASSERT_xxx() now. > Also line length should be under 100 characters (except for the case > when we have a long string literal, we don't break string literals, no > matter how long). Will fix > >> + goto error; >> + if (!ASSERT_OK_PTR(test_set.tests, "tests__initialized")) >> + goto error; >> + if (CHECK(!test_set.tests[0].whole_test, "parse_test_list subtest argument", "Expected test 0 to be initialized")) >> + goto error; >> + if (CHECK(strcmp("arg_parsing", test_set.tests[0].name), "parse_test_list subtest argument", "Expected test 0 to be initialized")) >> + goto error; >> + free_test_set(&test_set); >> + >> + parse_test_list("arg_parsing,bpf_cookie", &test_set, true); >> + if (CHECK(test_set.cnt != 2, "parse_test_list subtest argument", "Unexpected number of tests in num table %d\n", test_set.cnt)) >> + goto error; >> + if (!ASSERT_OK_PTR(test_set.tests, "tests__initialized")) >> + goto error; >> + if (CHECK(!test_set.tests[0].whole_test, "parse_test_list subtest argument", "Expected test 0 to be fully runnable")) >> + goto error; >> + if (CHECK(!test_set.tests[1].whole_test, "parse_test_list subtest argument", "Expected test 1 to be fully runnable")) >> + goto error; > > nit: I think there is no need to goto error after each check. We need > goto error if subsequent checks can crash due to some invalid state. > In this case, validating the value of few independent values is ok to > always check without goto error jumps. Makes tests shorter and > simpler. Will fix > > >> + if (CHECK(strcmp("arg_parsing", test_set.tests[0].name), "parse_test_list subtest argument", "Expected test 0 to be arg_parsing")) >> + goto error; >> + if (CHECK(strcmp("bpf_cookie", test_set.tests[1].name), "parse_test_list subtest argument", "Expected test 1 to be bpf_cookie")) >> + goto error; >> + free_test_set(&test_set); >> + >> + parse_test_list("arg_parsing/test_parse_test_list,bpf_cookie", &test_set, true); >> + if (CHECK(test_set.cnt != 2, "parse_test_list subtest argument", "Unexpected number of tests in num table %d\n", test_set.cnt)) >> + goto error; > > [...] > >> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c >> index 0a4b45d7b515..671e37cada4b 100644 >> --- a/tools/testing/selftests/bpf/test_progs.c >> +++ b/tools/testing/selftests/bpf/test_progs.c >> @@ -3,6 +3,7 @@ >> */ >> #define _GNU_SOURCE >> #include "test_progs.h" >> +#include "testing_helpers.h" >> #include "cgroup_helpers.h" >> #include <argp.h> >> #include <pthread.h> >> @@ -82,14 +83,14 @@ int usleep(useconds_t usec) >> static bool should_run(struct test_selector *sel, int num, const char *name) >> { >> int i; >> - > > keep empty line between variable declaration block and first statement Will fix, curious though why it is not caught by the script. Probably because this line is not modified > >> for (i = 0; i < sel->blacklist.cnt; i++) { >> - if (glob_match(name, sel->blacklist.strs[i])) >> + if (glob_match(name, sel->blacklist.tests[i].name) && >> + sel->blacklist.tests[i].whole_test) >> return false; >> } >> >> for (i = 0; i < sel->whitelist.cnt; i++) { >> - if (glob_match(name, sel->whitelist.strs[i])) >> + if (glob_match(name, sel->whitelist.tests[i].name)) >> return true; >> } >> > > [...] > >> -bool test__start_subtest(const char *name) >> +bool test__start_subtest(const char *subtest_name) >> { >> struct prog_test_def *test = env.test; >> >> @@ -205,17 +246,21 @@ bool test__start_subtest(const char *name) >> >> test->subtest_num++; >> >> - if (!name || !name[0]) { >> + if (!subtest_name || !subtest_name[0]) { >> fprintf(env.stderr, >> "Subtest #%d didn't provide sub-test name!\n", >> test->subtest_num); >> return false; >> } >> >> - if (!should_run(&env.subtest_selector, test->subtest_num, name)) >> + if (!should_run_subtest(&env.test_selector, >> + &env.subtest_selector, > > we don't have subtest_selector anymore, do we? Unfortunately, we do use subtest selector for -n option. I do have a change for -n option, but I want to review it separately. Otherwise it makes the change way to big > > also, do you think that maybe combining should_run and > should_rub_subtest would be a good idea? You can distinguish by having > subtest_name NULL when you need to check only test? Similar to above, because of -n option, combining two makes logic too convoluted. Happy to address it as a separate change once we figure out -n option behavior > >> + test->subtest_num, >> + test->test_name, >> + subtest_name)) >> return false; >> >> - test->subtest_name = strdup(name); >> + test->subtest_name = strdup(subtest_name); >> if (!test->subtest_name) { >> fprintf(env.stderr, >> "Subtest #%d: failed to copy subtest name!\n", >> @@ -527,63 +572,29 @@ static int libbpf_print_fn(enum libbpf_print_level level, >> return 0; >> } >> > > [...] > >> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h >> index eec4c7385b14..6a465a98341e 100644 >> --- a/tools/testing/selftests/bpf/test_progs.h >> +++ b/tools/testing/selftests/bpf/test_progs.h >> @@ -37,7 +37,6 @@ typedef __u16 __sum16; >> #include <bpf/bpf_endian.h> >> #include "trace_helpers.h" >> #include "testing_helpers.h" >> -#include "flow_dissector_load.h" >> >> enum verbosity { >> VERBOSE_NONE, >> @@ -46,14 +45,21 @@ enum verbosity { >> VERBOSE_SUPER, >> }; >> >> -struct str_set { >> - const char **strs; >> +struct prog_test { >> + char *name; >> + char **subtests; >> + int subtest_cnt; >> + bool whole_test; >> +}; >> + >> +struct test_set { >> + struct prog_test *tests; > > prog_test is a bad name, IMO. it's a "test filter", right? Let's call it that? > > test_set isn't most accurate as well, maybe test_filter_set or test_set_filter? Will change > >> int cnt; >> }; >> >> struct test_selector { >> - struct str_set whitelist; >> - struct str_set blacklist; >> + struct test_set whitelist; >> + struct test_set blacklist; >> bool *num_set; >> int num_set_len; >> }; >> diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c >> index 795b6798ccee..d2160d2a1303 100644 >> --- a/tools/testing/selftests/bpf/testing_helpers.c >> +++ b/tools/testing/selftests/bpf/testing_helpers.c >> @@ -6,6 +6,7 @@ >> #include <errno.h> >> #include <bpf/bpf.h> >> #include <bpf/libbpf.h> >> +#include "test_progs.h" >> #include "testing_helpers.h" >> >> int parse_num_list(const char *s, bool **num_set, int *num_set_len) >> @@ -69,6 +70,92 @@ int parse_num_list(const char *s, bool **num_set, int *num_set_len) >> return 0; >> } >> >> +int parse_test_list(const char *s, >> + struct test_set *test_set, >> + bool is_glob_pattern) >> +{ >> + char *input, *state = NULL, *next; >> + struct prog_test *tmp, *tests = NULL; >> + int i, j, cnt = 0; >> + >> + input = strdup(s); >> + if (!input) >> + return -ENOMEM; >> + >> + while ((next = strtok_r(state ? NULL : input, ",", &state))) { >> + char *subtest_str = strchr(next, '/'); >> + char *pattern = NULL; >> + >> + tmp = realloc(tests, sizeof(*tests) * (cnt + 1)); >> + if (!tmp) >> + goto err; >> + tests = tmp; >> + >> + tests[cnt].subtest_cnt = 0; >> + tests[cnt].subtests = NULL; >> + tests[cnt].whole_test = false; >> + >> + if (subtest_str) { >> + char **tmp_subtests = NULL; > > need an empty line between variable declarations and statements Will fix > >> + *subtest_str = '\0'; >> + int subtest_cnt = tests[cnt].subtest_cnt; >> + >> + tmp_subtests = realloc(tests[cnt].subtests, >> + sizeof(*tmp_subtests) * >> + (subtest_cnt + 1)); >> + if (!tmp_subtests) >> + goto err; >> + tests[cnt].subtests = tmp_subtests; >> + >> + tests[cnt].subtests[subtest_cnt] = strdup(subtest_str + 1); >> + if (!tests[cnt].subtests[subtest_cnt]) >> + goto err; >> + >> + tests[cnt].subtest_cnt++; >> + } else { >> + tests[cnt].whole_test = true; > > isn't whole_test equivalent to subtest_cnt > 0? Why keeping extra bool > of state then? Will fix. Overthought it here a bit > >> + } >> + >> + if (is_glob_pattern) { >> + pattern = "%s"; >> + tests[cnt].name = malloc(strlen(next) + 1); >> + } else { >> + pattern = "*%s*"; >> + tests[cnt].name = malloc(strlen(next) + 2 + 1); >> + } >> + >> + if (!tests[cnt].name) >> + goto err; >> + >> + sprintf(tests[cnt].name, pattern, next); >> + >> + cnt++; >> + } >> + >> + tmp = realloc(test_set->tests, sizeof(*tests) * (cnt + test_set->cnt)); >> + if (!tmp) >> + goto err; >> + >> + memcpy(tmp + test_set->cnt, tests, sizeof(*tests) * cnt); >> + test_set->tests = tmp; >> + test_set->cnt += cnt; >> + >> + free(tests); >> + free(input); >> + return 0; >> + >> +err: >> + for (i = 0; i < cnt; i++) { >> + for (j = 0; j < tests[i].subtest_cnt; j++) >> + free(tests[i].subtests[j]); >> + >> + free(tests[i].name); >> + } >> + free(tests); >> + free(input); >> + return -ENOMEM; >> +} >> + >> __u32 link_info_prog_id(const struct bpf_link *link, struct bpf_link_info *info) >> { >> __u32 info_len = sizeof(*info); >> diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h >> index f46ebc476ee8..d2f502184cd1 100644 >> --- a/tools/testing/selftests/bpf/testing_helpers.h >> +++ b/tools/testing/selftests/bpf/testing_helpers.h >> @@ -12,3 +12,9 @@ int bpf_test_load_program(enum bpf_prog_type type, const struct bpf_insn *insns, >> size_t insns_cnt, const char *license, >> __u32 kern_version, char *log_buf, >> size_t log_buf_sz); >> + >> +/* >> + * below function is exported for testing in prog_test test >> + */ >> +struct test_set; >> +int parse_test_list(const char *s, struct test_set *test_set, bool is_glob_pattern); >> -- >> 2.30.2
diff --git a/tools/testing/selftests/bpf/prog_tests/arg_parsing.c b/tools/testing/selftests/bpf/prog_tests/arg_parsing.c new file mode 100644 index 000000000000..f41a532e6194 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/arg_parsing.c @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) + +#include "test_progs.h" +#include "testing_helpers.h" + +static int duration; + +static void init_test_set(struct test_set *test_set) +{ + test_set->cnt = 0; + test_set->tests = NULL; +} + +static void free_test_set(struct test_set *test_set) +{ + int i, j; + + for (i = 0; i < test_set->cnt; i++) { + for (j = 0; j < test_set->tests[i].subtest_cnt; j++) + free((void *)test_set->tests[i].subtests[j]); + free(test_set->tests[i].subtests); + free(test_set->tests[i].name); + } + + free(test_set->tests); + init_test_set(test_set); +} + +static void test_parse_test_list(void) +{ + struct test_set test_set; + + init_test_set(&test_set); + + parse_test_list("arg_parsing", &test_set, true); + if (CHECK(test_set.cnt != 1, "parse_test_list subtest argument", "Unexpected number of tests in num table %d\n", test_set.cnt)) + goto error; + if (!ASSERT_OK_PTR(test_set.tests, "tests__initialized")) + goto error; + if (CHECK(!test_set.tests[0].whole_test, "parse_test_list subtest argument", "Expected test 0 to be initialized")) + goto error; + if (CHECK(strcmp("arg_parsing", test_set.tests[0].name), "parse_test_list subtest argument", "Expected test 0 to be initialized")) + goto error; + free_test_set(&test_set); + + parse_test_list("arg_parsing,bpf_cookie", &test_set, true); + if (CHECK(test_set.cnt != 2, "parse_test_list subtest argument", "Unexpected number of tests in num table %d\n", test_set.cnt)) + goto error; + if (!ASSERT_OK_PTR(test_set.tests, "tests__initialized")) + goto error; + if (CHECK(!test_set.tests[0].whole_test, "parse_test_list subtest argument", "Expected test 0 to be fully runnable")) + goto error; + if (CHECK(!test_set.tests[1].whole_test, "parse_test_list subtest argument", "Expected test 1 to be fully runnable")) + goto error; + if (CHECK(strcmp("arg_parsing", test_set.tests[0].name), "parse_test_list subtest argument", "Expected test 0 to be arg_parsing")) + goto error; + if (CHECK(strcmp("bpf_cookie", test_set.tests[1].name), "parse_test_list subtest argument", "Expected test 1 to be bpf_cookie")) + goto error; + free_test_set(&test_set); + + parse_test_list("arg_parsing/test_parse_test_list,bpf_cookie", &test_set, true); + if (CHECK(test_set.cnt != 2, "parse_test_list subtest argument", "Unexpected number of tests in num table %d\n", test_set.cnt)) + goto error; + if (!ASSERT_OK_PTR(test_set.tests, "tests__initialized")) + goto error; + if (CHECK(test_set.tests[0].whole_test, "parse_test_list no subtest argument", "Expected test 0 to be fully runnable")) + goto error; + if (CHECK(!test_set.tests[1].whole_test, "parse_test_list no subtest argument", "Expected test 0 to be fully runnable")) + goto error; + if (CHECK(strcmp("arg_parsing", test_set.tests[0].name), "parse_test_list subtest argument", "Expected test 0 to be arg_parsing")) + goto error; + if (CHECK(test_set.tests[0].subtest_cnt != 1, "parse_test_list subtest number", "Unexpected number of subtests for arg_parsing %d\n", test_set.tests[0].subtest_cnt)) + goto error; + if (CHECK(strcmp("test_parse_test_list", test_set.tests[0].subtests[0]), "parse_test_list subtest name", "Expected test 0 first subtest to be to be test_parse_test_list")) + goto error; + if (CHECK(test_set.tests[1].subtest_cnt != 0, "parse_test_list subtest number", "Unexpected number of subtests for bpf_cookie %d\n", test_set.tests[1].subtest_cnt)) + goto error; + if (CHECK(strcmp("bpf_cookie", test_set.tests[1].name), "parse_test_list subtest argument", "Expected test 1 to be bpf_cookie")) + goto error; +error: + free_test_set(&test_set); +} + +void test_arg_parsing(void) +{ + if (test__start_subtest("test_parse_test_list")) + test_parse_test_list(); +} diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index 0a4b45d7b515..671e37cada4b 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -3,6 +3,7 @@ */ #define _GNU_SOURCE #include "test_progs.h" +#include "testing_helpers.h" #include "cgroup_helpers.h" #include <argp.h> #include <pthread.h> @@ -82,14 +83,14 @@ int usleep(useconds_t usec) static bool should_run(struct test_selector *sel, int num, const char *name) { int i; - for (i = 0; i < sel->blacklist.cnt; i++) { - if (glob_match(name, sel->blacklist.strs[i])) + if (glob_match(name, sel->blacklist.tests[i].name) && + sel->blacklist.tests[i].whole_test) return false; } for (i = 0; i < sel->whitelist.cnt; i++) { - if (glob_match(name, sel->whitelist.strs[i])) + if (glob_match(name, sel->whitelist.tests[i].name)) return true; } @@ -99,6 +100,46 @@ static bool should_run(struct test_selector *sel, int num, const char *name) return num < sel->num_set_len && sel->num_set[num]; } +static bool should_run_subtest(struct test_selector *sel, + struct test_selector *subtest_sel, + int subtest_num, + const char *test_name, + const char *subtest_name) +{ + int i, j; + + for (i = 0; i < sel->blacklist.cnt; i++) { + if (glob_match(test_name, sel->blacklist.tests[i].name)) { + if (!sel->blacklist.tests[i].subtest_cnt) + return false; + + for (j = 0; j < sel->blacklist.tests[i].subtest_cnt; j++) { + if (glob_match(subtest_name, + sel->blacklist.tests[i].subtests[j])) + return false; + } + } + } + + for (i = 0; i < sel->whitelist.cnt; i++) { + if (glob_match(test_name, sel->whitelist.tests[i].name)) { + if (!sel->whitelist.tests[i].subtest_cnt) + return true; + + for (j = 0; j < sel->whitelist.tests[i].subtest_cnt; j++) { + if (glob_match(subtest_name, + sel->whitelist.tests[i].subtests[j])) + return true; + } + } + } + + if (!sel->whitelist.cnt && !subtest_sel->num_set) + return true; + + return subtest_num < subtest_sel->num_set_len && subtest_sel->num_set[subtest_num]; +} + static void dump_test_log(const struct prog_test_def *test, bool failed) { if (stdout == env.stdout) @@ -196,7 +237,7 @@ void test__end_subtest(void) test->subtest_name = NULL; } -bool test__start_subtest(const char *name) +bool test__start_subtest(const char *subtest_name) { struct prog_test_def *test = env.test; @@ -205,17 +246,21 @@ bool test__start_subtest(const char *name) test->subtest_num++; - if (!name || !name[0]) { + if (!subtest_name || !subtest_name[0]) { fprintf(env.stderr, "Subtest #%d didn't provide sub-test name!\n", test->subtest_num); return false; } - if (!should_run(&env.subtest_selector, test->subtest_num, name)) + if (!should_run_subtest(&env.test_selector, + &env.subtest_selector, + test->subtest_num, + test->test_name, + subtest_name)) return false; - test->subtest_name = strdup(name); + test->subtest_name = strdup(subtest_name); if (!test->subtest_name) { fprintf(env.stderr, "Subtest #%d: failed to copy subtest name!\n", @@ -527,63 +572,29 @@ static int libbpf_print_fn(enum libbpf_print_level level, return 0; } -static void free_str_set(const struct str_set *set) +static void free_test_set(const struct test_set *set) { - int i; + int i, j; if (!set) return; - for (i = 0; i < set->cnt; i++) - free((void *)set->strs[i]); - free(set->strs); -} - -static int parse_str_list(const char *s, struct str_set *set, bool is_glob_pattern) -{ - char *input, *state = NULL, *next, **tmp, **strs = NULL; - int i, cnt = 0; + for (i = 0; i < set->cnt; i++) { + free((void *)set->tests[i].name); + for (j = 0; j < set->tests[i].subtest_cnt; j++) + free((void *)set->tests[i].subtests[j]); - input = strdup(s); - if (!input) - return -ENOMEM; - - while ((next = strtok_r(state ? NULL : input, ",", &state))) { - tmp = realloc(strs, sizeof(*strs) * (cnt + 1)); - if (!tmp) - goto err; - strs = tmp; - - if (is_glob_pattern) { - strs[cnt] = strdup(next); - if (!strs[cnt]) - goto err; - } else { - strs[cnt] = malloc(strlen(next) + 2 + 1); - if (!strs[cnt]) - goto err; - sprintf(strs[cnt], "*%s*", next); - } - - cnt++; + free((void *)set->tests[i].subtests); } - tmp = realloc(set->strs, sizeof(*strs) * (cnt + set->cnt)); - if (!tmp) - goto err; - memcpy(tmp + set->cnt, strs, sizeof(*strs) * cnt); - set->strs = (const char **)tmp; - set->cnt += cnt; + free((void *)set->tests); +} - free(input); - free(strs); - return 0; -err: - for (i = 0; i < cnt; i++) - free(strs[i]); - free(strs); - free(input); - return -ENOMEM; +static void free_test_selector(struct test_selector *test_selector) +{ + free_test_set(&test_selector->blacklist); + free_test_set(&test_selector->whitelist); + free(test_selector->num_set); } extern int extra_prog_load_log_flags; @@ -615,33 +626,17 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state) } case ARG_TEST_NAME_GLOB_ALLOWLIST: case ARG_TEST_NAME: { - char *subtest_str = strchr(arg, '/'); - - if (subtest_str) { - *subtest_str = '\0'; - if (parse_str_list(subtest_str + 1, - &env->subtest_selector.whitelist, - key == ARG_TEST_NAME_GLOB_ALLOWLIST)) - return -ENOMEM; - } - if (parse_str_list(arg, &env->test_selector.whitelist, - key == ARG_TEST_NAME_GLOB_ALLOWLIST)) + if (parse_test_list(arg, + &env->test_selector.whitelist, + key == ARG_TEST_NAME_GLOB_ALLOWLIST)) return -ENOMEM; break; } case ARG_TEST_NAME_GLOB_DENYLIST: case ARG_TEST_NAME_BLACKLIST: { - char *subtest_str = strchr(arg, '/'); - - if (subtest_str) { - *subtest_str = '\0'; - if (parse_str_list(subtest_str + 1, - &env->subtest_selector.blacklist, - key == ARG_TEST_NAME_GLOB_DENYLIST)) - return -ENOMEM; - } - if (parse_str_list(arg, &env->test_selector.blacklist, - key == ARG_TEST_NAME_GLOB_DENYLIST)) + if (parse_test_list(arg, + &env->test_selector.blacklist, + key == ARG_TEST_NAME_GLOB_DENYLIST)) return -ENOMEM; break; } @@ -1493,12 +1488,8 @@ int main(int argc, char **argv) out: if (!env.list_test_names && env.has_testmod) unload_bpf_testmod(); - free_str_set(&env.test_selector.blacklist); - free_str_set(&env.test_selector.whitelist); - free(env.test_selector.num_set); - free_str_set(&env.subtest_selector.blacklist); - free_str_set(&env.subtest_selector.whitelist); - free(env.subtest_selector.num_set); + + free_test_selector(&env.test_selector); if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0) return EXIT_NO_TEST; diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index eec4c7385b14..6a465a98341e 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -37,7 +37,6 @@ typedef __u16 __sum16; #include <bpf/bpf_endian.h> #include "trace_helpers.h" #include "testing_helpers.h" -#include "flow_dissector_load.h" enum verbosity { VERBOSE_NONE, @@ -46,14 +45,21 @@ enum verbosity { VERBOSE_SUPER, }; -struct str_set { - const char **strs; +struct prog_test { + char *name; + char **subtests; + int subtest_cnt; + bool whole_test; +}; + +struct test_set { + struct prog_test *tests; int cnt; }; struct test_selector { - struct str_set whitelist; - struct str_set blacklist; + struct test_set whitelist; + struct test_set blacklist; bool *num_set; int num_set_len; }; diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c index 795b6798ccee..d2160d2a1303 100644 --- a/tools/testing/selftests/bpf/testing_helpers.c +++ b/tools/testing/selftests/bpf/testing_helpers.c @@ -6,6 +6,7 @@ #include <errno.h> #include <bpf/bpf.h> #include <bpf/libbpf.h> +#include "test_progs.h" #include "testing_helpers.h" int parse_num_list(const char *s, bool **num_set, int *num_set_len) @@ -69,6 +70,92 @@ int parse_num_list(const char *s, bool **num_set, int *num_set_len) return 0; } +int parse_test_list(const char *s, + struct test_set *test_set, + bool is_glob_pattern) +{ + char *input, *state = NULL, *next; + struct prog_test *tmp, *tests = NULL; + int i, j, cnt = 0; + + input = strdup(s); + if (!input) + return -ENOMEM; + + while ((next = strtok_r(state ? NULL : input, ",", &state))) { + char *subtest_str = strchr(next, '/'); + char *pattern = NULL; + + tmp = realloc(tests, sizeof(*tests) * (cnt + 1)); + if (!tmp) + goto err; + tests = tmp; + + tests[cnt].subtest_cnt = 0; + tests[cnt].subtests = NULL; + tests[cnt].whole_test = false; + + if (subtest_str) { + char **tmp_subtests = NULL; + *subtest_str = '\0'; + int subtest_cnt = tests[cnt].subtest_cnt; + + tmp_subtests = realloc(tests[cnt].subtests, + sizeof(*tmp_subtests) * + (subtest_cnt + 1)); + if (!tmp_subtests) + goto err; + tests[cnt].subtests = tmp_subtests; + + tests[cnt].subtests[subtest_cnt] = strdup(subtest_str + 1); + if (!tests[cnt].subtests[subtest_cnt]) + goto err; + + tests[cnt].subtest_cnt++; + } else { + tests[cnt].whole_test = true; + } + + if (is_glob_pattern) { + pattern = "%s"; + tests[cnt].name = malloc(strlen(next) + 1); + } else { + pattern = "*%s*"; + tests[cnt].name = malloc(strlen(next) + 2 + 1); + } + + if (!tests[cnt].name) + goto err; + + sprintf(tests[cnt].name, pattern, next); + + cnt++; + } + + tmp = realloc(test_set->tests, sizeof(*tests) * (cnt + test_set->cnt)); + if (!tmp) + goto err; + + memcpy(tmp + test_set->cnt, tests, sizeof(*tests) * cnt); + test_set->tests = tmp; + test_set->cnt += cnt; + + free(tests); + free(input); + return 0; + +err: + for (i = 0; i < cnt; i++) { + for (j = 0; j < tests[i].subtest_cnt; j++) + free(tests[i].subtests[j]); + + free(tests[i].name); + } + free(tests); + free(input); + return -ENOMEM; +} + __u32 link_info_prog_id(const struct bpf_link *link, struct bpf_link_info *info) { __u32 info_len = sizeof(*info); diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h index f46ebc476ee8..d2f502184cd1 100644 --- a/tools/testing/selftests/bpf/testing_helpers.h +++ b/tools/testing/selftests/bpf/testing_helpers.h @@ -12,3 +12,9 @@ int bpf_test_load_program(enum bpf_prog_type type, const struct bpf_insn *insns, size_t insns_cnt, const char *license, __u32 kern_version, char *log_buf, size_t log_buf_sz); + +/* + * below function is exported for testing in prog_test test + */ +struct test_set; +int parse_test_list(const char *s, struct test_set *test_set, bool is_glob_pattern);
Improve subtest selection logic when using -t/-a/-d parameters. In particular, more than one subtest can be specified or a combination of tests / subtests. -a send_signal -d send_signal/send_signal_nmi* - runs send_signal test without nmi tests -a send_signal/send_signal_nmi*,find_vma - runs two send_signal subtests and find_vma test This will allow us to have granular control over which subtests to disable in the CI system instead of disabling whole tests. Also, add new selftest to avoid possible regression when changing prog_test test name selection logic. Signed-off-by: Mykola Lysenko <mykolal@fb.com> --- .../selftests/bpf/prog_tests/arg_parsing.c | 88 ++++++++++ tools/testing/selftests/bpf/test_progs.c | 157 +++++++++--------- tools/testing/selftests/bpf/test_progs.h | 16 +- tools/testing/selftests/bpf/testing_helpers.c | 87 ++++++++++ tools/testing/selftests/bpf/testing_helpers.h | 6 + 5 files changed, 266 insertions(+), 88 deletions(-) create mode 100644 tools/testing/selftests/bpf/prog_tests/arg_parsing.c