On Wed, Jul 28, 2021 at 8:14 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
>
> Rewrite to skel and ASSERT macros as well while we are at it.
>
> Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> ---
Thanks! Bonus points for skeleton and ASSERT_XXX! ;)
In addition to Yonghong's comments, a few more below. Missed assert()s
require a new revision, unfortunately.
> tools/testing/selftests/bpf/Makefile | 3 +-
> .../testing/selftests/bpf/prog_tests/netcnt.c | 93 +++++++++++
> tools/testing/selftests/bpf/test_netcnt.c | 148 ------------------
Usually there is .gitignore clean up as well.
> 3 files changed, 94 insertions(+), 150 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/netcnt.c
> delete mode 100644 tools/testing/selftests/bpf/test_netcnt.c
>
[...]
> +
> + skel->links.bpf_nextcnt =
> + bpf_program__attach_cgroup(skel->progs.bpf_nextcnt, cg_fd);
> + if (!ASSERT_OK_PTR(skel->links.bpf_nextcnt,
> + "attach_cgroup(bpf_nextcnt)"))
> + goto err;
> +
> + if (system("which ping6 &>/dev/null") == 0)
see 4cda0c82a34b ("selftests/bpf: Use ping6 only if available in
tc_redirect"), we should probably add some system() + ping -6/ping6
wrapper into network_helpers.c and use that in at least all
test_progs' tests.
> + assert(!system("ping6 ::1 -c 10000 -f -q > /dev/null"));
> + else
> + assert(!system("ping -6 ::1 -c 10000 -f -q > /dev/null"));
no assert() please
> +
> + map_fd = bpf_map__fd(skel->maps.netcnt);
> + if (!ASSERT_GE(map_fd, 0, "bpf_map__fd(netcnt)"))
> + goto err;
> +
> + percpu_map_fd = bpf_map__fd(skel->maps.percpu_netcnt);
> + if (!ASSERT_GE(percpu_map_fd, 0, "bpf_map__fd(percpu_netcnt)"))
> + goto err;
> +
> + if (!ASSERT_OK(bpf_map_get_next_key(map_fd, NULL, &key),
> + "bpf_map_get_next_key"))
it's ok to use all 100 characters if that helps keeps simple function
invocations on the single line, so don't hesitate to do that
> + goto err;
> +
> + if (!ASSERT_OK(bpf_map_lookup_elem(map_fd, &key, &netcnt),
> + "bpf_map_lookup_elem(netcnt)"))
> + goto err;
> +
> + if (!ASSERT_OK(bpf_map_lookup_elem(percpu_map_fd, &key,
> + &percpu_netcnt[0]),
> + "bpf_map_lookup_elem(percpu_netcnt)"))
> + goto err;
> +
[...]
