| 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
| ||
|
Message-ID: <20201103004205.qbyabntlc4yl5vwn@kafai-mbp.dhcp.thefacebook.com>
Date: Mon, 2 Nov 2020 16:42:44 -0800
From: Martin KaFai Lau <kafai@...com>
To: Alexander Duyck <alexander.duyck@...il.com>
CC: <bpf@...r.kernel.org>, <ast@...nel.org>, <daniel@...earbox.net>,
<john.fastabend@...il.com>, <kernel-team@...com>,
<netdev@...r.kernel.org>, <edumazet@...gle.com>, <brakmo@...com>,
<andrii.nakryiko@...il.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:24AM -0700, Alexander Duyck 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>
Acked-by: Martin KaFai Lau <kafai@...com>
> ---
> .../testing/selftests/bpf/prog_tests/tcpbpf_user.c | 114 ++++++++------------
> 1 file changed, 44 insertions(+), 70 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> index 17d4299435df..d96f4084d2f5 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> @@ -10,66 +10,58 @@
>
> static __u32 duration;
>
> -#define EXPECT_EQ(expected, actual, fmt) \
> - do { \
> - if ((expected) != (actual)) { \
> - fprintf(stderr, " Value of: " #actual "\n" \
> - " Actual: %" fmt "\n" \
> - " Expected: %" fmt "\n", \
> - (actual), (expected)); \
> - ret--; \
> - } \
> - } while (0)
> -
> -int verify_result(const struct tcpbpf_globals *result)
> -{
> - __u32 expected_events;
> - int ret = 0;
> -
> - expected_events = ((1 << BPF_SOCK_OPS_TIMEOUT_INIT) |
> - (1 << BPF_SOCK_OPS_RWND_INIT) |
> - (1 << BPF_SOCK_OPS_TCP_CONNECT_CB) |
> - (1 << BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB) |
> - (1 << BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB) |
> - (1 << BPF_SOCK_OPS_NEEDS_ECN) |
> - (1 << BPF_SOCK_OPS_STATE_CB) |
> - (1 << BPF_SOCK_OPS_TCP_LISTEN_CB));
> -
> - EXPECT_EQ(expected_events, result->event_map, "#" PRIx32);
> - EXPECT_EQ(501ULL, result->bytes_received, "llu");
> - EXPECT_EQ(1002ULL, result->bytes_acked, "llu");
> - EXPECT_EQ(1, result->data_segs_in, PRIu32);
> - EXPECT_EQ(1, result->data_segs_out, PRIu32);
> - EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32);
> - EXPECT_EQ(0, result->good_cb_test_rv, PRIu32);
> - EXPECT_EQ(1, result->num_listen, PRIu32);
> -
> - /* 3 comes from one listening socket + both ends of the connection */
> - EXPECT_EQ(3, result->num_close_events, PRIu32);
> -
> - return ret;
> -}
> -
> -int verify_sockopt_result(int sock_map_fd)
> +static void verify_result(int map_fd, int sock_map_fd)
> {
> + __u32 expected_events = ((1 << BPF_SOCK_OPS_TIMEOUT_INIT) |
> + (1 << BPF_SOCK_OPS_RWND_INIT) |
> + (1 << BPF_SOCK_OPS_TCP_CONNECT_CB) |
> + (1 << BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB) |
> + (1 << BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB) |
> + (1 << BPF_SOCK_OPS_NEEDS_ECN) |
> + (1 << BPF_SOCK_OPS_STATE_CB) |
> + (1 << BPF_SOCK_OPS_TCP_LISTEN_CB));
> + struct tcpbpf_globals result = { 0 };
nit. init is not needed.
> __u32 key = 0;
> - int ret = 0;
> int res;
> int rv;
>
> + 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);
> +
> + 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");
> +
> + /* 3 comes from one listening socket + both ends of the connection */
> + ASSERT_EQ(result.num_close_events, 3, "num_close_events");
> +
> /* check setsockopt for SAVE_SYN */
> + key = 0;
nit. not needed.
> rv = bpf_map_lookup_elem(sock_map_fd, &key, &res);
> - EXPECT_EQ(0, rv, "d");
> - EXPECT_EQ(0, res, "d");
> - key = 1;
> + CHECK(rv, "bpf_map_lookup_elem(sock_map_fd)", "err:%d errno:%d",
> + rv, errno);
> + ASSERT_EQ(res, 0, "bpf_setsockopt(TCP_SAVE_SYN)");
> +
> /* check getsockopt for SAVED_SYN */
> + key = 1;
> rv = bpf_map_lookup_elem(sock_map_fd, &key, &res);
> - EXPECT_EQ(0, rv, "d");
> - EXPECT_EQ(1, res, "d");
> - return ret;
> + CHECK(rv, "bpf_map_lookup_elem(sock_map_fd)", "err:%d errno:%d",
> + rv, errno);
> + ASSERT_EQ(res, 1, "bpf_getsockopt(TCP_SAVED_SYN)");
> }
Powered by blists - more mailing lists