Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> [Wed, 2019-12-18 22:11 -0800]:
> On Wed, Dec 18, 2019 at 6:14 PM Andrey Ignatov <rdna@xxxxxx> wrote:
> >
> > Convert test_cgroup_attach to prog_tests.
> >
> > This change does a lot of things but in many cases it's pretty expensive
> > to separate them, so they go in one commit. Nevertheless the logic is
> > ketp as is and changes made are just moving things around, simplifying
> > them (w/o changing the meaning of the tests) and making prog_tests
> > compatible:
> >
> > * split the 3 tests in the file into 3 separate files in prog_tests/;
> >
> > * rename the test functions to test_<file_base_name>;
> >
> > * remove unused includes, constants, variables and functions from every
> > test;
> >
> > * replace `if`-s with or `if (CHECK())` where additional context should
> > be logged and with `if (CHECK_FAIL())` where line number is enough;
> >
> > * switch from `log_err()` to logging via `CHECK()`;
> >
> > * replace `assert`-s with `CHECK_FAIL()` to avoid crashing the whole
> > test_progs if one assertion fails;
> >
> > * replace cgroup_helpers with test__join_cgroup() in
> > cgroup_attach_override only, other tests need more fine-grained
> > control for cgroup creation/deletion so cgroup_helpers are still used
> > there;
> >
> > * simplify cgroup_attach_autodetach by switching to easiest possible
> > program since this test doesn't really need such a complicated program
> > as cgroup_attach_multi does;
> >
> > * remove test_cgroup_attach.c itself.
> >
> > Signed-off-by: Andrey Ignatov <rdna@xxxxxx>
> > ---
> > tools/testing/selftests/bpf/.gitignore | 1 -
> > tools/testing/selftests/bpf/Makefile | 3 +-
> > .../bpf/prog_tests/cgroup_attach_autodetach.c | 111 ++++
> > .../bpf/prog_tests/cgroup_attach_multi.c | 238 ++++++++
> > .../bpf/prog_tests/cgroup_attach_override.c | 148 +++++
> > .../selftests/bpf/test_cgroup_attach.c | 571 ------------------
> > 6 files changed, 498 insertions(+), 574 deletions(-)
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_autodetach.c
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_multi.c
> > create mode 100644 tools/testing/selftests/bpf/prog_tests/cgroup_attach_override.c
> > delete mode 100644 tools/testing/selftests/bpf/test_cgroup_attach.c
> >
>
> [...]
>
> > +
> > + if (CHECK_FAIL(setup_cgroup_environment()))
> > + goto err;
> > +
> > + cg1 = create_and_get_cgroup("/cg1");
> > + if (CHECK_FAIL(cg1 < 0))
> > + goto err;
> > + cg2 = create_and_get_cgroup("/cg1/cg2");
> > + if (CHECK_FAIL(cg2 < 0))
> > + goto err;
> > + cg3 = create_and_get_cgroup("/cg1/cg2/cg3");
> > + if (CHECK_FAIL(cg3 < 0))
> > + goto err;
> > + cg4 = create_and_get_cgroup("/cg1/cg2/cg3/cg4");
> > + if (CHECK_FAIL(cg4 < 0))
> > + goto err;
> > + cg5 = create_and_get_cgroup("/cg1/cg2/cg3/cg4/cg5");
> > + if (CHECK_FAIL(cg5 < 0))
> > + goto err;
> > +
> > + if (CHECK_FAIL(join_cgroup("/cg1/cg2/cg3/cg4/cg5")))
> > + goto err;
> > +
> > + if (CHECK(bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS,
> > + BPF_F_ALLOW_MULTI),
> > + "prog0_attach_to_cg1_multi", "errno=%d\n", errno))
> > + goto err;
> > +
> > + if (CHECK(!bpf_prog_attach(allow_prog[0], cg1, BPF_CGROUP_INET_EGRESS,
> > + BPF_F_ALLOW_MULTI),
> > + "fail_same_prog_attach_to_cg1", "unexpected success\n"))
> > + goto err;
> > +
> > + if (CHECK(bpf_prog_attach(allow_prog[1], cg1, BPF_CGROUP_INET_EGRESS,
> > + BPF_F_ALLOW_MULTI),
> > + "prog1_attach_to_cg1_multi", "errno=%d\n", errno))
> > + goto err;
> > +
> > + if (CHECK(bpf_prog_attach(allow_prog[2], cg2, BPF_CGROUP_INET_EGRESS,
> > + BPF_F_ALLOW_OVERRIDE),
> > + "prog2_attach_to_cg2_override", "errno=%d\n", errno))
> > + goto err;
> > +
> > + if (CHECK(bpf_prog_attach(allow_prog[3], cg3, BPF_CGROUP_INET_EGRESS,
> > + BPF_F_ALLOW_MULTI),
> > + "prog3_attach_to_cg3_multi", "errno=%d\n", errno))
> > + goto err;
> > +
> > + if (CHECK(bpf_prog_attach(allow_prog[4], cg4, BPF_CGROUP_INET_EGRESS,
> > + BPF_F_ALLOW_OVERRIDE),
> > + "prog4_attach_to_cg4_override", "errno=%d\n", errno))
> > + goto err;
> > +
> > + if (CHECK(bpf_prog_attach(allow_prog[5], cg5, BPF_CGROUP_INET_EGRESS, 0),
> > + "prog5_attach_to_cg5_none", "errno=%d\n", errno))
> > + goto err;
>
> nit: this looks like a good candidate for a loop...
These tests can benefit from a lot of cleanup steps and yeah, this can
be one of them. I'd prefer to deal with it separately though since it
shouldn't be a blocker for this patch set. That's why I preserved this
logic with just adding checks.
> > + CHECK_FAIL(system(PING_CMD));
> > + CHECK_FAIL(bpf_map_lookup_elem(map_fd, &key, &value));
> > + CHECK_FAIL(value != 1 + 2 + 8 + 32);
> > +
>
> [...]
>
> > -int main(void)
> > -{
> > - int (*tests[])(void) = {
> > - test_foo_bar,
> > - test_multiprog,
> > - test_autodetach,
> > - };
> > - int errors = 0;
> > - int i;
> > -
> > - for (i = 0; i < ARRAY_SIZE(tests); i++)
> > - if (tests[i]())
> > - errors++;
>
> Depending on what you think is better structure (I couldn't follow
> through entire test logic...), you could have done this as a single
> test with 3 subtests. Search for test__start_subtest(). If that saves
> you some duplication, I think it's worth converting (which will be
> 1-to-1 with original structure).
Yeah, I saw test__start_subtest() and checked if it would fit here. IMO
it wouldn't since these tests don't have much in common and can't be
converted to, say, parametrized tests that differ only in input that can
be easily described by a struct. They do cover parts of same feature
(cgroup attach) but focus on different parts of it. That's why separate
files. This actually helped to identify and remove a bunch of junk from
each test (like simplifying testing prog in autodetach).
> But either way thanks a lot for doing the convertion, brings as
> another step closer to more uniform and consolidated testing infra.
>
> > -
> > - if (errors)
> > - printf("test_cgroup_attach:FAIL\n");
> > - else
> > - printf("test_cgroup_attach:PASS\n");
> > -
> > - return errors ? EXIT_FAILURE : EXIT_SUCCESS;
> > -}
> > --
> > 2.17.1
> >
--
Andrey Ignatov
