On Mon, Apr 12, 2021 at 9:29 AM Jiri Olsa <jo...@kernel.org> wrote:
>
> Adding the test to re-attach (detach/attach again) tracing
> fentry programs, plus check that already linked program can't
> be attached again.
>
> Also switching to ASSERT* macros and adding missing ';' in
> ASSERT_ERR_PTR macro.
>
> Signed-off-by: Jiri Olsa <jo...@kernel.org>
> ---
> .../selftests/bpf/prog_tests/fentry_test.c | 51 +++++++++++++------
> tools/testing/selftests/bpf/test_progs.h | 2 +-
> 2 files changed, 37 insertions(+), 16 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/fentry_test.c
> b/tools/testing/selftests/bpf/prog_tests/fentry_test.c
> index 04ebbf1cb390..f440c74f5367 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fentry_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fentry_test.c
> @@ -3,35 +3,56 @@
> #include <test_progs.h>
> #include "fentry_test.skel.h"
>
> -void test_fentry_test(void)
> +static int fentry_test(struct fentry_test *fentry_skel)
> {
> - struct fentry_test *fentry_skel = NULL;
> int err, prog_fd, i;
> __u32 duration = 0, retval;
> + struct bpf_link *link;
> __u64 *result;
>
> - fentry_skel = fentry_test__open_and_load();
> - if (CHECK(!fentry_skel, "fentry_skel_load", "fentry skeleton
> failed\n"))
> - goto cleanup;
> -
> err = fentry_test__attach(fentry_skel);
> - if (CHECK(err, "fentry_attach", "fentry attach failed: %d\n", err))
> - goto cleanup;
> + if (!ASSERT_OK(err, "fentry_attach"))
> + return err;
> +
> + /* Check that already linked program can't be attached again. */
> + link = bpf_program__attach(fentry_skel->progs.test1);
> + if (!ASSERT_ERR_PTR(link, "fentry_attach_link"))
> + return -1;
>
> prog_fd = bpf_program__fd(fentry_skel->progs.test1);
> err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
> NULL, NULL, &retval, &duration);
> - CHECK(err || retval, "test_run",
> - "err %d errno %d retval %d duration %d\n",
> - err, errno, retval, duration);
> + ASSERT_OK(err || retval, "test_run");
this is quite misleading, even if will result in a correct check. Toke
did this in his patch set:
ASSERT_OK(err, ...);
ASSERT_EQ(retval, 0, ...);
It is a better and more straightforward way to validate the checks
instead of relying on (err || retval) -> bool (true) -> int (1) -> !=
0 chain.
>
> result = (__u64 *)fentry_skel->bss;
> - for (i = 0; i < 6; i++) {
> - if (CHECK(result[i] != 1, "result",
> - "fentry_test%d failed err %lld\n", i + 1,
> result[i]))
> - goto cleanup;
> + for (i = 0; i < sizeof(*fentry_skel->bss) / sizeof(__u64); i++) {
> + if (!ASSERT_EQ(result[i], 1, "fentry_result"))
> + return -1;
> }
>
> + fentry_test__detach(fentry_skel);
> +
> + /* zero results for re-attach test */
> + memset(fentry_skel->bss, 0, sizeof(*fentry_skel->bss));
> + return 0;
> +}
> +
> +void test_fentry_test(void)
> +{
> + struct fentry_test *fentry_skel = NULL;
> + int err;
> +
> + fentry_skel = fentry_test__open_and_load();
> + if (!ASSERT_OK_PTR(fentry_skel, "fentry_skel_load"))
> + goto cleanup;
> +
> + err = fentry_test(fentry_skel);
> + if (!ASSERT_OK(err, "fentry_first_attach"))
> + goto cleanup;
> +
> + err = fentry_test(fentry_skel);
> + ASSERT_OK(err, "fentry_second_attach");
> +
> cleanup:
> fentry_test__destroy(fentry_skel);
> }
> diff --git a/tools/testing/selftests/bpf/test_progs.h
> b/tools/testing/selftests/bpf/test_progs.h
> index e87c8546230e..ee7e3b45182a 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -210,7 +210,7 @@ extern int test__join_cgroup(const char *path);
> #define ASSERT_ERR_PTR(ptr, name) ({ \
> static int duration = 0; \
> const void *___res = (ptr); \
> - bool ___ok = IS_ERR(___res) \
> + bool ___ok = IS_ERR(___res); \
heh, it probably deserves a separate patch with Fixes tag...
> CHECK(!___ok, (name), "unexpected pointer: %p\n", ___res); \
> ___ok; \
> })
> --
> 2.30.2
>
