netdev - Re: [bpf-next PATCH v2 3/5] selftests/bpf: Replace EXPECT_EQ with ASSERT_EQ and refactor verify

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 3 Nov 2020 10:38:12 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Martin Lau <kafai@...com>,
        john fastabend <john.fastabend@...il.com>,
        Kernel Team <kernel-team@...com>,
        Networking <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Lawrence Brakmo <brakmo@...com>, alexanderduyck@...com
Subject: Re: [bpf-next PATCH v2 3/5] selftests/bpf: Replace EXPECT_EQ with
 ASSERT_EQ and refactor verify_results

On Sat, Oct 31, 2020 at 11:52 AM Alexander Duyck
<alexander.duyck@...il.com> wrote:
>
> From: Alexander Duyck <alexanderduyck@...com>
>
> There is already logic in test_progs.h for asserting that a value is
> expected to be another value. So instead of reinventing it we should just
> make use of ASSERT_EQ in tcpbpf_user.c. This will allow for better
> debugging and integrates much more closely with the test_progs framework.
>
> In addition we can refactor the code a bit to merge together the two
> verify functions and tie them together into a single function. Doing this
> helps to clean the code up a bit and makes it more readable as all the
> verification is now done in one function.
>
> Lastly we can relocate the verification to the end of the run_test since it
> is logically part of the test itself. With this we can drop the need for a
> return value from run_test since verification becomes the last step of the
> call and then immediately following is the tear down of the test setup.
>
> Signed-off-by: Alexander Duyck <alexanderduyck@...com>
> ---
>  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |  114 ++++++++------------
>  1 file changed, 44 insertions(+), 70 deletions(-)
>

[...]

> +       rv = bpf_map_lookup_elem(map_fd, &key, &result);
> +       if (CHECK(rv, "bpf_map_lookup_elem(map_fd)", "err:%d errno:%d",
> +                 rv, errno))
> +               return;
> +
> +       /* check global map */
> +       CHECK(expected_events != result.event_map, "event_map",
> +             "unexpected event_map: actual %#" PRIx32" != expected %#" PRIx32 "\n",
> +             result.event_map, expected_events);

nit: libbpf and selftests don't use PRI modifiers approach. Just cast
to a consistent long, int, unsigned, whichever matches the needs and
use appropriate explicit % specifier.

> +
> +       ASSERT_EQ(result.bytes_received, 501, "bytes_received");
> +       ASSERT_EQ(result.bytes_acked, 1002, "bytes_acked");
> +       ASSERT_EQ(result.data_segs_in, 1, "data_segs_in");
> +       ASSERT_EQ(result.data_segs_out, 1, "data_segs_out");
> +       ASSERT_EQ(result.bad_cb_test_rv, 0x80, "bad_cb_test_rv");
> +       ASSERT_EQ(result.good_cb_test_rv, 0, "good_cb_test_rv");
> +       ASSERT_EQ(result.num_listen, 1, "num_listen");
> +

[...]