diff mbox series

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

Message ID 160417034457.2823.10600750891200038944.stgit@localhost.localdomain (mailing list archive)
State Accepted
Delegated to: BPF
Headers show
Series selftests/bpf: Migrate test_tcpbpf_user to be a part of test_progs framework | expand

Commit Message

Alexander Duyck Oct. 31, 2020, 6:52 p.m. UTC
From: Alexander Duyck <alexanderduyck@fb.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@fb.com>
---
 .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |  114 ++++++++------------
 1 file changed, 44 insertions(+), 70 deletions(-)

Comments

Martin KaFai Lau Nov. 3, 2020, 12:42 a.m. UTC | #1
On Sat, Oct 31, 2020 at 11:52:24AM -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@fb.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@fb.com>
Acked-by: Martin KaFai Lau <kafai@fb.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)");
>  }
Alexander Duyck Nov. 3, 2020, 12:56 a.m. UTC | #2
On Mon, Nov 2, 2020 at 4:42 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Sat, Oct 31, 2020 at 11:52:24AM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexanderduyck@fb.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@fb.com>
> Acked-by: Martin KaFai Lau <kafai@fb.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?

> >       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)");
> >  }
Martin KaFai Lau Nov. 3, 2020, 1:40 a.m. UTC | #3
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@fb.com> wrote:
> >
> > On Sat, Oct 31, 2020 at 11:52:24AM -0700, Alexander Duyck wrote:
> > > From: Alexander Duyck <alexanderduyck@fb.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@fb.com>
> > Acked-by: Martin KaFai Lau <kafai@fb.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)");
> > >  }
Andrii Nakryiko Nov. 3, 2020, 6:38 p.m. UTC | #4
On Sat, Oct 31, 2020 at 11:52 AM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> From: Alexander Duyck <alexanderduyck@fb.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@fb.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");
> +

[...]
diff mbox series

Patch

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 };
 	__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;
 	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)");
 }
 
-static int run_test(void)
+static void run_test(int map_fd, int sock_map_fd)
 {
 	int listen_fd = -1, cli_fd = -1, accept_fd = -1;
 	char buf[1000];
@@ -135,18 +127,17 @@  static int run_test(void)
 	if (listen_fd != -1)
 		close(listen_fd);
 
-	return err;
+	if (!err)
+		verify_result(map_fd, sock_map_fd);
 }
 
 void test_tcpbpf_user(void)
 {
 	const char *file = "test_tcpbpf_kern.o";
 	int prog_fd, map_fd, sock_map_fd;
-	struct tcpbpf_globals g = {0};
 	int error = EXIT_FAILURE;
 	struct bpf_object *obj;
 	int cg_fd = -1;
-	__u32 key = 0;
 	int rv;
 
 	cg_fd = test__join_cgroup(CG_NAME);
@@ -173,24 +164,7 @@  void test_tcpbpf_user(void)
 	if (sock_map_fd < 0)
 		goto err;
 
-	if (run_test())
-		goto err;
-
-	rv = bpf_map_lookup_elem(map_fd, &key, &g);
-	if (rv != 0) {
-		fprintf(stderr, "FAILED: bpf_map_lookup_elem returns %d\n", rv);
-		goto err;
-	}
-
-	if (verify_result(&g)) {
-		fprintf(stderr, "FAILED: Wrong stats\n");
-		goto err;
-	}
-
-	if (verify_sockopt_result(sock_map_fd)) {
-		fprintf(stderr, "FAILED: Wrong sockopt stats\n");
-		goto err;
-	}
+	run_test(map_fd, sock_map_fd);
 
 	error = 0;
 err: