Re: [PATCH bpf-next 2/2] selftests/bpf: exercise veristat filtering logic in a selftest

On Tue, 2026-08-11 at 18:56 +0000, [email protected] wrote:
> > Test cases for veristat file/prog name filtering logic.
> > Check various formulations for any (*foo*), file (*foo*/),
> > prog (/*bar*) and file/prog (*foo*/bar) filters, alongside
> > errorneous filters and mixed allow/deny filter expressions.
> 
> This isn't a bug, but there's a typo: "errorneous" should be "erroneous".

Oops

> Also, is the prog filter form meant to be "/bar" rather than "/*bar*" to
> match the cases in the table?

Nope, *bar* is fine.

> 
> > diff --git a/tools/testing/selftests/bpf/prog_tests/test_veristat.c b/tools/testing/selftests/bpf/prog_tests/test_veristat.c
> > index 9aff08ac55c0..e1eda4ae755e 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/test_veristat.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/test_veristat.c
> > @@ -37,6 +37,13 @@ static struct fixture *init_fixture(void)
> >  	return fix;
> >  }
> > 
> > +static void read_output(struct fixture *fix)
> > +{
> > +	ssize_t len = pread(fix->fd, fix->output, fix->sz - 1, 0);
> > +
> > +	fix->output[len < 0 ? 0 : len] = 0;
> > +}
> > +
> 
> This isn't a bug, but since read_output() also terminates the buffer, would
> it make sense to switch the existing read()/pread() call sites in this file
> over to it as well? The new helper is safer than the bare reads used in the
> other subtests.

Makes sense.

> > +/*
> > + * Name filter tests below run veristat on veristat_foo.bpf.o and
> > + * veristat_bar.bpf.o, both defining programs 'foo', 'bar' and 'buz'.
> > + * Every entry describes a single (filters, file, prog) combination and
> > + * tells whether that program is expected in the veristat output:
> > + * 'true' if it is, 'false' if it is not and -1 if veristat is expected
> > + * to reject the filter.
> > + */
> > +#define FILTER_OBJS "veristat_foo.bpf.o veristat_bar.bpf.o"
> > +
> > +static const struct name_filter_case {
> > +	const char *filters;
> > +	const char *file;
> > +	const char *prog;
> > +	int included;
> > +} name_filter_cases[] = {
> > +	/* no filters, every program is processed */
> > +	{ "",			"foo", "foo", true  },
> > +	{ "",			"foo", "bar", true  },
> > +	{ "",			"foo", "buz", true  },
> > +	{ "",			"bar", "foo", true  },
> > +	{ "",			"bar", "bar", true  },
> > +	{ "",			"bar", "buz", true  },
> > +	/* deny filters */
> > +	{ "-f '!*foo*'",	"foo", "bar", false },
> > +	{ "-f '!*foo*'",	"bar", "foo", false },
> 
> A subsystem pattern flags this as potentially concerning: the table rows for
> '-f !*foo*' both assert 'false' (row should be absent), but there is no row
> that asserts 'true' (row should be present) for the same filter. Every other
> filter with 'false' rows also has a 'true' row for the same filter string.

Makes sense to add.

> For example:
>   '!*foo*/bar' has (foo,buz)/(bar,bar) true
>   '!*foo*/' has (bar,bar) true
>   '!/bar' has (foo,foo) true
>   '*foo*/bar' has (foo,bar) true
> 
> Adding one positive row for '-f !*foo*', such as { "-f '!*foo*'", "bar",
> "bar", true }, would make this filter's coverage consistent with the rest
> of the table. A regression that made the any-glob deny form reject everything
> would still satisfy both existing rows plus ASSERT_OK(err), because absence
> of a row is all that is checked.
> 
> The expectations themselves are correct (they match veristat's implementation
> and the changelog table of the preceding fix commit), so this is a
> test-completeness observation rather than a functional bug.
> 
> [ ... ]
> 
> > diff --git a/tools/testing/selftests/bpf/progs/veristat_bar.c b/tools/testing/selftests/bpf/progs/veristat_bar.c
> > new file mode 100644
> > index 000000000000..83d2a2a1dfc9
> > --- /dev/null
> > +++ b/tools/testing/selftests/bpf/progs/veristat_bar.c
> > @@ -0,0 +1,3 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2026 Meta Platforms, Inc. and affiliates. */
> > +#include "veristat_foo.c"
> 
> [ ... ]
> 
> 
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
> 
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/31521742364