| 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: <20201103013727.l24s7cveuxmpjuvb@kafai-mbp.dhcp.thefacebook.com>
Date: Mon, 2 Nov 2020 17:40:44 -0800
From: Martin KaFai Lau <kafai@...com>
To: Alexander Duyck <alexander.duyck@...il.com>
CC: bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>,
John Fastabend <john.fastabend@...il.com>,
Kernel Team <kernel-team@...com>,
Netdev <netdev@...r.kernel.org>,
Eric Dumazet <edumazet@...gle.com>,
Lawrence Brakmo <brakmo@...com>,
Andrii Nakryiko <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 Mon, Nov 02, 2020 at 04:56:37PM -0800, Alexander Duyck wrote:
> On Mon, Nov 2, 2020 at 4:42 PM Martin KaFai Lau <kafai@...com> wrote:
> >
> > 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>
>
> Thanks for the review.
>
> > > ---
> > > .../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.
>
> I had copied/pasted it from the original code that was defining this.
> If a v3 is needed I can drop the initialization.
>
> > > __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.
>
> I assume you mean it is redundant since it was initialized to 0 when
> we declared key in the first place?
Correct.
My eariler comment in this patch can be ignored. I just noticed that this
will go away in the last patch.
I was nit-picking a little here because people will copy-and-paste codes
from selftests. just don't want to give a wrong impression that
those are necessary for calling bpf_map_lookup_elem().
>
> > > 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