On Tue, Dec 6, 2022 at 8:43 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Mon, Dec 05, 2022 at 05:11:58PM -0800, Andrii Nakryiko wrote:
> > +
> > +
> > +typedef const void *(*skel_elf_bytes_fn)(size_t *sz);
> > +
> > +extern void verification_tester__run_subtests(struct verification_tester *tester,
> > + const char *skel_name,
> > + skel_elf_bytes_fn elf_bytes_factory);
> > +
> > +extern void tester_fini(struct verification_tester *tester);
> > +
> > +#define RUN_VERIFICATION_TESTS(skel) ({ \
> > + struct verification_tester tester = {}; \
> > + \
> > + verification_tester__run_subtests(&tester, #skel, skel##__elf_bytes); \
> > + tester_fini(&tester); \
> > +})
>
> Looking great, but couldn't resist to bikeshed a bit here.
> It looks like generic testing facility. Maybe called RUN_TESTS ?
Sure, I will rename it to RUN_TESTS.
>
> > +
> > +#endif /* __TEST_PROGS_H */
> > diff --git a/tools/testing/selftests/bpf/verifier_tester.c b/tools/testing/selftests/bpf/verifier_tester.c
>
> verifier_tester name also doesn't quite fit imo.
> These tests not necessarily trying to test just the verifier.
> They test BTF, kfuncs and everything that kernel has to check during the loading.
>
verifier_tester is bad name, I was intending it as
verification_testing, because it's testing BPF program loading
(verification). But you are right, we most probably will extend it to
allow doing attach/prog_test_run for successful cases (I just didn't
have time to implement that yet).
> In other words they test this:
> > + err = bpf_object__load(tobj);
> > + if (spec.expect_failure) {
> > + if (!ASSERT_ERR(err, "unexpected_load_success")) {
> > + emit_verifier_log(tester->log_buf, false /*force*/);
> > + goto tobj_cleanup;
> > + }
> > + } else {
> > + if (!ASSERT_OK(err, "unexpected_load_failure")) {
> > + emit_verifier_log(tester->log_buf, true /*force*/);
> > + goto tobj_cleanup;
> > + }
> > + }
>
> maybe call it
> +struct test_loader {
> + char *log_buf;
> + size_t log_buf_sz;
> +
> + struct bpf_object *obj;
> +};
> ?
> and the file test_loader.c ?
> Nicely shorter than verification_tester__ prefix...
sure, test_loader sounds fine to me, will rename
