On Mon, Aug 16, 2021 at 9:47 PM Yucong Sun <fallentree@xxxxxx> wrote:
>
> This patch adds '-a' and '-d' arguments, support exact string match, as well as
> using '*' wildcard in test/subtests selection. The old '-t' '-b' arguments
> still supports partial string match, but they can't be used together yet.
>
> This patach also adds support for mulitple '-a' '-d' '-t' '-b' arguments.
>
> Caveat: Same as the current substring matching mechanism, test and subtest
> selector applies independently, 'a*/b*' will execute all tests matching "a*",
> and with subtest name matching "b*", but tests matching "a*" that has no
> subtests will also be executed.
>
> Signed-off-by: Yucong Sun <fallentree@xxxxxx>
> ---
> tools/testing/selftests/bpf/test_progs.c | 72 +++++++++++++++++++-----
> 1 file changed, 58 insertions(+), 14 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 90539b15b744..c34eb818f115 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -13,6 +13,28 @@
> #include <execinfo.h> /* backtrace */
> #include <linux/membarrier.h>
>
> +/* Adapted from perf/util/string.c */
> +static bool glob_match(const char *str, const char *pat)
> +{
> + while (*str && *pat && *pat != '*') {
> + if (*str != *pat)
> + return false;
> + str++;
> + pat++;
> + }
> + /* Check wild card */
> + if (*pat == '*') {
> + while (*pat == '*')
> + pat++;
> + if (!*pat) /* Tail wild card matches all */
> + return true;
> + while (*str)
> + if (glob_match(str++, pat))
> + return true;
> + }
> + return !*str && !*pat;
> +}
> +
> #define EXIT_NO_TEST 2
> #define EXIT_ERR_SETUP_INFRA 3
>
> @@ -55,12 +77,12 @@ static bool should_run(struct test_selector *sel, int num, const char *name)
> int i;
>
> for (i = 0; i < sel->blacklist.cnt; i++) {
> - if (strstr(name, sel->blacklist.strs[i]))
> + if (glob_match(name, sel->blacklist.strs[i]))
> return false;
> }
>
> for (i = 0; i < sel->whitelist.cnt; i++) {
> - if (strstr(name, sel->whitelist.strs[i]))
> + if (glob_match(name, sel->whitelist.strs[i]))
> return true;
> }
>
> @@ -450,6 +472,8 @@ enum ARG_KEYS {
> ARG_VERBOSE = 'v',
> ARG_GET_TEST_CNT = 'c',
> ARG_LIST_TEST_NAMES = 'l',
> + ARG_TEST_NAME_GLOB_ALLOWLIST = 'a',
> + ARG_TEST_NAME_GLOB_DENYLIST = 'd',
> };
>
> static const struct argp_option opts[] = {
> @@ -467,6 +491,10 @@ static const struct argp_option opts[] = {
> "Get number of selected top-level tests " },
> { "list", ARG_LIST_TEST_NAMES, NULL, 0,
> "List test names that would run (without running them) " },
> + { "allow", ARG_TEST_NAME_GLOB_ALLOWLIST, "NAMES", 0,
> + "Run tests with name matching the pattern (support *)." },
> + { "deny", ARG_TEST_NAME_GLOB_DENYLIST, "NAMES", 0,
> + "Don't run tests with name matching the pattern (support *)." },
> {},
> };
>
> @@ -491,7 +519,7 @@ static void free_str_set(const struct str_set *set)
> free(set->strs);
> }
>
> -static int parse_str_list(const char *s, struct str_set *set)
> +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 cnt = 0;
> @@ -500,28 +528,38 @@ static int parse_str_list(const char *s, struct str_set *set)
> if (!input)
> return -ENOMEM;
>
> - set->cnt = 0;
> - set->strs = NULL;
> -
> 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);
> + } else {
> + strs[cnt] = malloc(strlen(next) + 2 + 1);
> + if (!strs[cnt])
> + goto err;
> + sprintf(strs[cnt], "*%s*", next);
> + }
>
> - strs[cnt] = strdup(next);
> if (!strs[cnt])
> goto err;
>
> cnt++;
> }
>
> - set->cnt = cnt;
> - set->strs = (const char **)strs;
> + 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(input);
> + free(strs);
> return 0;
> err:
> - free(strs);
> + if (strs)
> + free(strs);
free(NULL) is noop, so no need to check if(strs)
You also missed the need to free all those strdup()'ed strings, so I added
for (i = 0; i < cnt; i++)
free(strs[i]);
We didn't need it originally, because eventually we call
free_str_set(), but in this case those strings will be leaked, because
we don't assign strs to set->strs.
BTW, a bit cleaner approach would be to just work on set->strs
directly, and realloc()ing it as necessary, and only at the end
updating set->cnt. You wouldn't need to memcpy and realloc one extra
time at the end, and no need for that for + free loop. But it's fine
the way it is as well.
Applied to bpf-next, thanks a lot for the improvements.
> free(input);
> return -ENOMEM;
> }
> @@ -553,29 +591,35 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
> }
> break;
> }
> + 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))
> + &env->subtest_selector.whitelist,
> + key == ARG_TEST_NAME_GLOB_ALLOWLIST))
> return -ENOMEM;
> }
> - if (parse_str_list(arg, &env->test_selector.whitelist))
> + if (parse_str_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))
> + &env->subtest_selector.blacklist,
> + key == ARG_TEST_NAME_GLOB_DENYLIST))
> return -ENOMEM;
> }
> - if (parse_str_list(arg, &env->test_selector.blacklist))
> + if (parse_str_list(arg, &env->test_selector.blacklist,
> + key == ARG_TEST_NAME_GLOB_DENYLIST))
> return -ENOMEM;
> break;
> }
> --
> 2.30.2
>
