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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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?
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)");
> > >  }



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux