On Fri, Mar 26, 2021 at 5:24 AM Yauheni Kaliuta
<yauheni.kaliuta@xxxxxxxxxx> wrote:
>
> Switch the test to use BPF skeleton to save some boilerplate and
> make it easy to access bpf program bss segment.
>
> The latter will be used to pass PAGE_SIZE from userspace since there
> is no convenient way for bpf program to get it from inside of the
> kernel.
>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@xxxxxxxxxx>
> ---
It's a nice cleanup. See some suggestion to further clean it up. But
even with this:
Acked-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> .../selftests/bpf/prog_tests/sockopt_sk.c | 72 ++++++-------------
> 1 file changed, 23 insertions(+), 49 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> index d5b44b135c00..114c1a622ffa 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> @@ -3,6 +3,7 @@
> #include "cgroup_helpers.h"
>
> #include <linux/tcp.h>
> +#include "sockopt_sk.skel.h"
>
> #ifndef SOL_TCP
> #define SOL_TCP IPPROTO_TCP
> @@ -191,60 +192,33 @@ static int getsetsockopt(void)
> return -1;
> }
>
> -static int prog_attach(struct bpf_object *obj, int cgroup_fd, const char *title)
> -{
> - enum bpf_attach_type attach_type;
> - enum bpf_prog_type prog_type;
> - struct bpf_program *prog;
> - int err;
> -
> - err = libbpf_prog_type_by_name(title, &prog_type, &attach_type);
> - if (err) {
> - log_err("Failed to deduct types for %s BPF program", title);
> - return -1;
> - }
> -
> - prog = bpf_object__find_program_by_title(obj, title);
> - if (!prog) {
> - log_err("Failed to find %s BPF program", title);
> - return -1;
> - }
> -
> - err = bpf_prog_attach(bpf_program__fd(prog), cgroup_fd,
> - attach_type, 0);
> - if (err) {
> - log_err("Failed to attach %s BPF program", title);
> - return -1;
> - }
> -
> - return 0;
> -}
> -
> static void run_test(int cgroup_fd)
> {
> - struct bpf_prog_load_attr attr = {
> - .file = "./sockopt_sk.o",
> - };
> - struct bpf_object *obj;
> - int ignored;
> - int err;
> -
> - err = bpf_prog_load_xattr(&attr, &obj, &ignored);
> - if (CHECK_FAIL(err))
> - return;
> -
> - err = prog_attach(obj, cgroup_fd, "cgroup/getsockopt");
> - if (CHECK_FAIL(err))
> - goto close_bpf_object;
> -
> - err = prog_attach(obj, cgroup_fd, "cgroup/setsockopt");
> - if (CHECK_FAIL(err))
> - goto close_bpf_object;
> + struct sockopt_sk *skel;
> + int duration = 0;
> +
> + skel = sockopt_sk__open_and_load();
> + if (CHECK(!skel, "skel_load", "sockopt_sk skeleton failed\n"))
> + goto cleanup;
if (!ASSERT_OK_PTR(skel, "skel_load))
is still less boilerplate :)
> +
> + skel->links._setsockopt =
> + bpf_program__attach_cgroup(skel->progs._setsockopt, cgroup_fd);
> + if (CHECK(IS_ERR(skel->links._setsockopt),
> + "setsockopt_attach", "err: %ld\n",
> + PTR_ERR(skel->links._setsockopt)))
you could save some more boilerplate if you did:
if (!ASSERT_OK_PTR(skel->links._getsockopt), "getsockopt_link")
goto cleanup;
> + goto cleanup;
> +
> + skel->links._getsockopt =
> + bpf_program__attach_cgroup(skel->progs._getsockopt, cgroup_fd);
> + if (CHECK(IS_ERR(skel->links._getsockopt),
> + "getsockopt_attach", "err: %ld\n",
> + PTR_ERR(skel->links._getsockopt)))
> + goto cleanup;
>
> CHECK_FAIL(getsetsockopt());
please switch this to ASSERT_OK() as well.
Once you don't use CHECK/CHECK_FAIL, you also won't need `int duration`, btw.
>
> -close_bpf_object:
> - bpf_object__close(obj);
> +cleanup:
> + sockopt_sk__destroy(skel);
> }
>
> void test_sockopt_sk(void)
> --
> 2.29.2
>
