On Mon, Aug 9, 2021 at 5:17 PM Yucong Sun <fallentree@xxxxxx> wrote:
>
> This patch adds glob matching to test selector, it accepts
> simple glob pattern "*", "?", "[]" to match the test names to run.
do we really need ? and []? I've been pretty happy so far in retsnoop
with supporting just these patterns:
1. exact match ('abc')
2. prefix match ('abc*')
3. suffix match ('*abc')
4. substring match ('*abc*')
See [0] for a naive but simple implementation for that logic. So far
with test_progs I've only needed two cases from the above: exact and
substring matches. So I'm leaning towards keeping it simple, actually.
But there is also an issue of backwards compatibility. People using
`test_progs -t substr` are used to substring matching logic, so with
this change you are breaking this, which will cause frustration, most
probably. So maybe let's add a new parameter to specify these globs.
E.g., maybe `-a <glob>` for whitelisting (allowlisting), and `-d
<glob>` for blacklisting (denylisting)? Also, instead of parsing
comma-separated lists as I did initially with -t, we should probably
just allow multiple occurences of -a and -d:
./test_progs -a '*core*' -a '*linux' -d 'core_autosize'
will allow all CO-RE tests except autosize one, plus will admit
vmlinux selftest.
Also, keep in mind that there are subtests within some tests, and
those should be matches with the same logic as well:
./test_progs -a 'core_reloc/size*'
should run:
#32/58 size:OK
#32/59 size___diff_sz:OK
#32/60 size___err_ambiguous:OK
It gets a bit trickier with globs for both test and subtest, e.g.
'*core*/size*' -- should it match just core_reloc/size* tests as
above? Or also core_retro, core_extern, etc tests even though they
don't have subtests starting with 'size'? I'd say the latter is more
desirable, but I haven't checked how hard that would be to support.
[0] https://github.com/anakryiko/retsnoop/blob/2e6217f7a82f421fcf3481cc401390605066ab26/src/mass_attacher.c#L982-L1015
>
> The glob matching function is copied from perf/util/string.c
>
> Signed-off-by: Yucong Sun <fallentree@xxxxxx>
> ---
> tools/testing/selftests/bpf/test_progs.c | 94 +++++++++++++++++++++++-
> 1 file changed, 92 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 74dde0af1592..c5bffd2e78ae 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -13,6 +13,96 @@
> #include <execinfo.h> /* backtrace */
> #include <linux/membarrier.h>
>
> +// Copied from perf/util/string.c
> +
> +/* Character class matching */
> +static bool __match_charclass(const char *pat, char c, const char **npat)
> +{
> + bool complement = false, ret = true;
> +
> + if (*pat == '!') {
> + complement = true;
> + pat++;
> + }
> + if (*pat++ == c) /* First character is special */
> + goto end;
> +
> + while (*pat && *pat != ']') { /* Matching */
> + if (*pat == '-' && *(pat + 1) != ']') { /* Range */
> + if (*(pat - 1) <= c && c <= *(pat + 1))
> + goto end;
> + if (*(pat - 1) > *(pat + 1))
> + goto error;
> + pat += 2;
> + } else if (*pat++ == c)
> + goto end;
> + }
> + if (!*pat)
> + goto error;
> + ret = false;
> +
> +end:
> + while (*pat && *pat != ']') /* Searching closing */
> + pat++;
> + if (!*pat)
> + goto error;
> + *npat = pat + 1;
> + return complement ? !ret : ret;
> +
> +error:
> + return false;
> +}
> +
> +// Copied from perf/util/string.c
> +/* Glob/lazy pattern matching */
> +static bool __match_glob(const char *str, const char *pat, bool ignore_space,
> + bool case_ins)
> +{
> + while (*str && *pat && *pat != '*') {
> + if (ignore_space) {
> + /* Ignore spaces for lazy matching */
> + if (isspace(*str)) {
> + str++;
> + continue;
> + }
> + if (isspace(*pat)) {
> + pat++;
> + continue;
> + }
> + }
> + if (*pat == '?') { /* Matches any single character */
> + str++;
> + pat++;
> + continue;
> + } else if (*pat == '[') /* Character classes/Ranges */
> + if (__match_charclass(pat + 1, *str, &pat)) {
> + str++;
> + continue;
> + } else
> + return false;
> + else if (*pat == '\\') /* Escaped char match as normal char */
> + pat++;
> + if (case_ins) {
> + if (tolower(*str) != tolower(*pat))
> + return false;
> + } else 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 (__match_glob(str++, pat, ignore_space, case_ins))
> + return true;
> + }
> + return !*str && !*pat;
> +}
> +
> #define EXIT_NO_TEST 2
> #define EXIT_ERR_SETUP_INFRA 3
>
> @@ -55,12 +145,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 (__match_glob(name, sel->blacklist.strs[i], false, false))
> return false;
> }
>
> for (i = 0; i < sel->whitelist.cnt; i++) {
> - if (strstr(name, sel->whitelist.strs[i]))
> + if (__match_glob(name, sel->whitelist.strs[i], false, false))
> return true;
> }
>
> --
> 2.30.2
>
