On Thu, Jan 27, 2022 at 5:26 PM Delyan Kratunov <delyank@xxxxxx> wrote:
>
> Signed-off-by: Delyan Kratunov <delyank@xxxxxx>
> ---
> .../selftests/bpf/prog_tests/atomics.c | 86 +++---
> .../testing/selftests/bpf/prog_tests/bpf_nf.c | 10 +-
> .../selftests/bpf/prog_tests/fentry_fexit.c | 33 ++-
> .../selftests/bpf/prog_tests/fentry_test.c | 9 +-
> .../selftests/bpf/prog_tests/fexit_bpf2bpf.c | 33 ++-
> .../selftests/bpf/prog_tests/fexit_stress.c | 26 +-
> .../selftests/bpf/prog_tests/fexit_test.c | 9 +-
> .../prog_tests/flow_dissector_load_bytes.c | 27 +-
> .../selftests/bpf/prog_tests/for_each.c | 32 ++-
> .../bpf/prog_tests/get_func_args_test.c | 14 +-
> .../bpf/prog_tests/get_func_ip_test.c | 12 +-
> .../selftests/bpf/prog_tests/global_data.c | 25 +-
> .../bpf/prog_tests/global_func_args.c | 13 +-
> .../selftests/bpf/prog_tests/kfunc_call.c | 46 ++--
> .../selftests/bpf/prog_tests/ksyms_module.c | 23 +-
> .../selftests/bpf/prog_tests/l4lb_all.c | 35 ++-
> .../selftests/bpf/prog_tests/map_lock.c | 15 +-
> .../selftests/bpf/prog_tests/map_ptr.c | 18 +-
> .../selftests/bpf/prog_tests/modify_return.c | 38 +--
> .../selftests/bpf/prog_tests/pkt_access.c | 27 +-
> .../selftests/bpf/prog_tests/pkt_md_access.c | 15 +-
> .../bpf/prog_tests/queue_stack_map.c | 43 +--
> .../bpf/prog_tests/raw_tp_writable_test_run.c | 16 +-
> .../selftests/bpf/prog_tests/signal_pending.c | 24 +-
> .../selftests/bpf/prog_tests/spinlock.c | 15 +-
> .../selftests/bpf/prog_tests/tailcalls.c | 245 ++++++++++--------
> .../bpf/prog_tests/test_skb_pkt_end.c | 15 +-
> .../testing/selftests/bpf/prog_tests/timer.c | 9 +-
> .../selftests/bpf/prog_tests/timer_mim.c | 9 +-
> .../selftests/bpf/prog_tests/trace_ext.c | 28 +-
> tools/testing/selftests/bpf/prog_tests/xdp.c | 35 ++-
> .../bpf/prog_tests/xdp_adjust_frags.c | 34 ++-
> .../bpf/prog_tests/xdp_adjust_tail.c | 114 +++++---
> .../selftests/bpf/prog_tests/xdp_bpf2bpf.c | 16 +-
> .../selftests/bpf/prog_tests/xdp_noinline.c | 45 ++--
> .../selftests/bpf/prog_tests/xdp_perf.c | 19 +-
> tools/testing/selftests/bpf/test_lru_map.c | 11 +-
> tools/testing/selftests/bpf/test_progs.h | 2 +
> tools/testing/selftests/bpf/test_verifier.c | 16 +-
> 39 files changed, 727 insertions(+), 515 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/atomics.c b/tools/testing/selftests/bpf/prog_tests/atomics.c
> index 86b7d5d84eec..12ecb3ae5c28 100644
> --- a/tools/testing/selftests/bpf/prog_tests/atomics.c
> +++ b/tools/testing/selftests/bpf/prog_tests/atomics.c
> @@ -7,18 +7,20 @@
> static void test_add(struct atomics_lskel *skel)
> {
> int err, prog_fd;
> - __u32 duration = 0, retval;
> int link_fd;
> + LIBBPF_OPTS(bpf_test_run_opts, topts,
> + .repeat = 1,
> + );
for simple case like this you can just keep it single line:
LIBBPF_OPTS(bpf_test_run_opts, topts, .repeat = 1)
But, it seems like the kernel does `if (!repeat) repeat = 1;` logic
for program types that support repeat feature, so I'd suggest just
drop .repeat = 1 and keep it simple.
>
> link_fd = atomics_lskel__add__attach(skel);
> if (!ASSERT_GT(link_fd, 0, "attach(add)"))
> return;
>
> prog_fd = skel->progs.add.prog_fd;
> - err = bpf_prog_test_run(prog_fd, 1, NULL, 0,
> - NULL, NULL, &retval, &duration);
> - if (CHECK(err || retval, "test_run add",
> - "err %d errno %d retval %d duration %d\n", err, errno, retval, duration))
> + err = bpf_prog_test_run_opts(prog_fd, &topts);
> + if (CHECK_OPTS(err || topts.retval, "test_run add",
let's not add new CHECK*() variants, CHECK() and its derivative are
deprecated, we are using ASSERT_xxx() macros for all new code.
In this case, I think it's best to replace CHECK() with just:
ASSERT_OK(err, "run_err");
ASSERT_OK(topts.retval, "run_ret_val");
I don't think logging duration is useful for most, if not all, tests,
so I wouldn't bother.
> + "err %d errno %d retval %d duration %d\n", err, errno,
> + topts.retval, topts.duration))
> goto cleanup;
>
> ASSERT_EQ(skel->data->add64_value, 3, "add64_value");
[...]
> diff --git a/tools/testing/selftests/bpf/prog_tests/fentry_fexit.c b/tools/testing/selftests/bpf/prog_tests/fentry_fexit.c
> index 4374ac8a8a91..cb0bcd9bb950 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fentry_fexit.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fentry_fexit.c
> @@ -9,38 +9,43 @@ void test_fentry_fexit(void)
> struct fentry_test_lskel *fentry_skel = NULL;
> struct fexit_test_lskel *fexit_skel = NULL;
> __u64 *fentry_res, *fexit_res;
> - __u32 duration = 0, retval;
> int err, prog_fd, i;
> + LIBBPF_OPTS(bpf_test_run_opts, topts,
> + .repeat = 1,
> + );
>
> fentry_skel = fentry_test_lskel__open_and_load();
> - if (CHECK(!fentry_skel, "fentry_skel_load", "fentry skeleton failed\n"))
> + if (CHECK_OPTS(!fentry_skel, "fentry_skel_load",
> + "fentry skeleton failed\n"))
You didn't have to touch this code, you could have just kept duration
= 0 (which CHECK() internally assumes, unfortunately).
Alternatively, just switch to ASSERT_OK_PTR(fentry_skel,
"fentry_skel_load"); and be done with it. As I mentioned, we are in a
gradual process of removing CHECK()s, I was actually surprised how far
we've progressed already:
$ rg CHECK | wc -l
2024
$ rg ASSERT_ | wc -l
1782
At some point we'll do the final push and remove CHECK() altogether,
but it doesn't have to be part of this patch set (unless you are
interested in doing some more mechanical conversions, of course, it's
greatly appreciated, but I didn't yet have heart to ask anyone to do
those 2000 conversions...).
> goto close_prog;
> fexit_skel = fexit_test_lskel__open_and_load();
> - if (CHECK(!fexit_skel, "fexit_skel_load", "fexit skeleton failed\n"))
> + if (CHECK_OPTS(!fexit_skel, "fexit_skel_load",
> + "fexit skeleton failed\n"))
> goto close_prog;
>
[...]
