| Message ID | 160417034457.2823.10600750891200038944.stgit@localhost.localdomain |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | selftests/bpf: Migrate test_tcpbpf_user to be a part of test_progs framework | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for bpf-next |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | fail | Errors and warnings before: 4 this patch: 4 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | fail | Link |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
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)"); > }
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)"); > > }
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)"); > > > }
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 --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: