From: Yonghong Song <yonghong.song@linux.dev>
To: Kui-Feng Lee <sinquersw@gmail.com>,
Yuran Pereira <yuran.pereira@hotmail.com>,
bpf@vger.kernel.org
Cc: shuah@kernel.org, ast@kernel.org, daniel@iogearbox.net,
song@kernel.org, john.fastabend@gmail.com, kpsingh@kernel.org,
sdf@google.com, haoluo@google.com, jolsa@kernel.org,
mykolal@fb.com, brauner@kernel.org, iii@linux.ibm.com,
kuifeng@meta.com, linux-kselftest@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH bpf-next v2 1/2] selftests/bpf: Convert CHECK macros to ASSERT_* macros in bpf_iter
Date: Thu, 26 Oct 2023 21:42:56 -0700 [thread overview]
Message-ID: <20d0c45e-8e1a-4bb9-93eb-8487d8d79651@linux.dev> (raw)
In-Reply-To: <3739e65c-b86d-4c11-9cf5-7b76080400c2@gmail.com>
On 10/25/23 9:33 PM, Kui-Feng Lee wrote:
>
>
> On 10/25/23 19:03, Yuran Pereira wrote:
>> As it was pointed out by Yonghong Song [1], in the bpf selftests the use
>> of the ASSERT_* series of macros is preferred over the CHECK macro.
>> This patch replaces all CHECK calls in bpf_iter with the appropriate
>> ASSERT_* macros.
>>
>> [1]
>> https://lore.kernel.org/lkml/0a142924-633c-44e6-9a92-2dc019656bf2@linux.dev
>>
>> Suggested-by: Yonghong Song <yonghong.song@linux.dev>
>> Signed-off-by: Yuran Pereira <yuran.pereira@hotmail.com>
>> ---
>> .../selftests/bpf/prog_tests/bpf_iter.c | 82 +++++++++----------
>> 1 file changed, 39 insertions(+), 43 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
>> b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
>> index 1f02168103dd..7db6972ed952 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_iter.c
>> @@ -34,8 +34,6 @@
>> #include "bpf_iter_ksym.skel.h"
>> #include "bpf_iter_sockmap.skel.h"
>> -static int duration;
>> -
>> static void test_btf_id_or_null(void)
>> {
>> struct bpf_iter_test_kern3 *skel;
>> @@ -64,7 +62,7 @@ static void do_dummy_read_opts(struct bpf_program
>> *prog, struct bpf_iter_attach_
>> /* not check contents, but ensure read() ends without error */
>> while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
>> ;
>> - CHECK(len < 0, "read", "read failed: %s\n", strerror(errno));
>> + ASSERT_GE(len, 0, "read");
>> close(iter_fd);
>> @@ -413,7 +411,7 @@ static int do_btf_read(struct bpf_iter_task_btf
>> *skel)
>> goto free_link;
>> }
>> - if (CHECK(err < 0, "read", "read failed: %s\n", strerror(errno)))
>> + if (!ASSERT_GE(err, 0, "read"))
>> goto free_link;
>> ASSERT_HAS_SUBSTR(taskbuf, "(struct task_struct)",
>> @@ -526,11 +524,11 @@ static int do_read_with_fd(int iter_fd, const
>> char *expected,
>> start = 0;
>> while ((len = read(iter_fd, buf + start, read_buf_len)) > 0) {
>> start += len;
>> - if (CHECK(start >= 16, "read", "read len %d\n", len))
>> + if (!ASSERT_LT(start, 16, "read"))
>> return -1;
>> read_buf_len = read_one_char ? 1 : 16 - start;
>> }
>> - if (CHECK(len < 0, "read", "read failed: %s\n", strerror(errno)))
>> + if (!ASSERT_GE(len, 0, "read"))
>> return -1;
>> if (!ASSERT_STREQ(buf, expected, "read"))
>> @@ -571,8 +569,7 @@ static int do_read(const char *path, const char
>> *expected)
>> int err, iter_fd;
>> iter_fd = open(path, O_RDONLY);
>> - if (CHECK(iter_fd < 0, "open", "open %s failed: %s\n",
>> - path, strerror(errno)))
>> + if (!ASSERT_GE(iter_fd, 0, "open"))
>> return -1;
>> err = do_read_with_fd(iter_fd, expected, false);
>> @@ -600,7 +597,7 @@ static void test_file_iter(void)
>> unlink(path);
>> err = bpf_link__pin(link, path);
>> - if (CHECK(err, "pin_iter", "pin_iter to %s failed: %d\n", path,
>> err))
>> + if (!ASSERT_OK(err, "pin_iter"))
>> goto free_link;
>> err = do_read(path, "abcd");
>> @@ -651,12 +648,10 @@ static void test_overflow(bool
>> test_e2big_overflow, bool ret1)
>> * overflow and needs restart.
>> */
>> map1_fd = bpf_map_create(BPF_MAP_TYPE_ARRAY, NULL, 4, 8, 1, NULL);
>> - if (CHECK(map1_fd < 0, "bpf_map_create",
>> - "map_creation failed: %s\n", strerror(errno)))
>> + if (!ASSERT_GE(map1_fd, 0, "bpf_map_create"))
>> goto out;
>> map2_fd = bpf_map_create(BPF_MAP_TYPE_ARRAY, NULL, 4, 8, 1, NULL);
>> - if (CHECK(map2_fd < 0, "bpf_map_create",
>> - "map_creation failed: %s\n", strerror(errno)))
>> + if (!ASSERT_GE(map2_fd, 0, "bpf_map_create"))
>> goto free_map1;
>> /* bpf_seq_printf kernel buffer is 8 pages, so one map
>> @@ -685,14 +680,12 @@ static void test_overflow(bool
>> test_e2big_overflow, bool ret1)
>> /* setup filtering map_id in bpf program */
>> map_info_len = sizeof(map_info);
>> err = bpf_map_get_info_by_fd(map1_fd, &map_info, &map_info_len);
>> - if (CHECK(err, "get_map_info", "get map info failed: %s\n",
>> - strerror(errno)))
>> + if (!ASSERT_OK(err, "get_map_info"))
>> goto free_map2;
>> skel->bss->map1_id = map_info.id;
>> err = bpf_map_get_info_by_fd(map2_fd, &map_info, &map_info_len);
>> - if (CHECK(err, "get_map_info", "get map info failed: %s\n",
>> - strerror(errno)))
>> + if (!ASSERT_OK(err, "get_map_info"))
>> goto free_map2;
>> skel->bss->map2_id = map_info.id;
>> @@ -714,16 +707,15 @@ static void test_overflow(bool
>> test_e2big_overflow, bool ret1)
>> while ((len = read(iter_fd, buf, expected_read_len)) > 0)
>> total_read_len += len;
>> - CHECK(len != -1 || errno != E2BIG, "read",
>> - "expected ret -1, errno E2BIG, but get ret %d, error
>> %s\n",
>> - len, strerror(errno));
>> + if (!ASSERT_EQ(len, -1, "read"))
>> + goto free_buf;
>> + ASSERT_EQ(errno, E2BIG, "read");
>
> I think you can just do
>
> ASSERT_EQ(len, -1, "read");
> ASSERT_EQ(errno, E2BG, "read");
>
> without a check here.
> WDYT?
Many recent selftests have ASSERT_* similar to what Kui-Feng is suggested.
So I think it is okay to do adjustment like it. The same for some other suggestions
below.
But since this patch intends to convert CHECK to ASSERT_*, so other possible
'optimizations' like above ASSERT_EQ can stay as is since they do not really
affect functionality.
In the next revision, please put three patches 0/2, 1/2 and 2/2 together as a single
patch set. Thanks.
>
>> goto free_buf;
>> } else if (!ret1) {
>> while ((len = read(iter_fd, buf, expected_read_len)) > 0)
>> total_read_len += len;
>> - if (CHECK(len < 0, "read", "read failed: %s\n",
>> - strerror(errno)))
>> + if (!ASSERT_GE(len, 0, "read"))
>> goto free_buf;
>> } else {
>> do {
>> @@ -732,8 +724,7 @@ static void test_overflow(bool
>> test_e2big_overflow, bool ret1)
>> total_read_len += len;
>> } while (len > 0 || len == -EAGAIN);
>> - if (CHECK(len < 0, "read", "read failed: %s\n",
>> - strerror(errno)))
>> + if (!ASSERT_GE(len, 0, "read"))
>> goto free_buf;
>> }
>> @@ -836,7 +827,7 @@ static void test_bpf_hash_map(void)
>> /* do some tests */
>> while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
>> ;
>> - if (CHECK(len < 0, "read", "read failed: %s\n", strerror(errno)))
>> + if (!ASSERT_GE(len, 0, "read"))
>> goto close_iter;
>> /* test results */
>> @@ -917,7 +908,7 @@ static void test_bpf_percpu_hash_map(void)
>> /* do some tests */
>> while ((len = read(iter_fd, buf, sizeof(buf))) > 0)
>> ;
>> - if (CHECK(len < 0, "read", "read failed: %s\n", strerror(errno)))
>> + if (!ASSERT_GE(len, 0, "read"))
>> goto close_iter;
>> /* test results */
>> @@ -983,17 +974,15 @@ static void test_bpf_array_map(void)
>> start = 0;
>> while ((len = read(iter_fd, buf + start, sizeof(buf) - start))
>> > 0)
>> start += len;
>> - if (CHECK(len < 0, "read", "read failed: %s\n", strerror(errno)))
>> + if (!ASSERT_GE(len, 0, "read"))
>> goto close_iter;
>> /* test results */
>> res_first_key = *(__u32 *)buf;
>> res_first_val = *(__u64 *)(buf + sizeof(__u32));
>> - if (CHECK(res_first_key != 0 || res_first_val != first_val,
>> - "bpf_seq_write",
>> - "seq_write failure: first key %u vs expected 0, "
>> - " first value %llu vs expected %llu\n",
>> - res_first_key, res_first_val, first_val))
>> + if (!ASSERT_EQ(res_first_key, 0, "bpf_seq_write"))
>> + goto close_iter;
>> + else if (!ASSERT_EQ(res_first_val, first_val, "bpf_seq_write"))
>> goto close_iter;
>
> Similiar here!
>
> if (!ASSERT_EQ(...) ||
> !ASSERT_EQ(...))
> goto close_iter;
>
> [...]
next prev parent reply other threads:[~2023-10-27 4:43 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20231026020319.1203600-1-yuran.pereira@hotmail.com>
2023-10-26 2:03 ` [PATCH bpf-next v2 1/2] selftests/bpf: Convert CHECK macros to ASSERT_* macros in bpf_iter Yuran Pereira
2023-10-26 4:33 ` Kui-Feng Lee
2023-10-27 4:42 ` Yonghong Song [this message]
2023-10-26 2:03 ` [PATCH bpf-next v2 2/2] selftests/bpf: Add malloc failure checks " Yuran Pereira
2023-10-26 4:40 ` Kui-Feng Lee
2023-10-27 4:37 ` Yonghong Song
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20d0c45e-8e1a-4bb9-93eb-8487d8d79651@linux.dev \
--to=yonghong.song@linux.dev \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=brauner@kernel.org \
--cc=daniel@iogearbox.net \
--cc=haoluo@google.com \
--cc=iii@linux.ibm.com \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=kuifeng@meta.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=mykolal@fb.com \
--cc=sdf@google.com \
--cc=shuah@kernel.org \
--cc=sinquersw@gmail.com \
--cc=song@kernel.org \
--cc=yuran.pereira@hotmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.