On Mon, Nov 2, 2020 at 4:42 PM Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> On Sat, Oct 31, 2020 at 11:52:24AM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexanderduyck@xxxxxx>
> >
> > 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@xxxxxx>
> Acked-by: Martin KaFai Lau <kafai@xxxxxx>
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?
> > 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)");
> > }
