Re: [PATCH bpf-next v5 2/2] selftests: bpf: test BPF_PROG_QUERY for progs attached to sockmap

[Yonghong Song]

On 11/3/21 6:07 PM, Di Zhu wrote:
> Add test for querying progs attached to sockmap. we use an existing
> libbpf query interface to query prog cnt before and after progs
> attaching to sockmap and check whether the queried prog id is right.
>
> Signed-off-by: Di Zhu <zhudi2@xxxxxxxxxx>
> ---
>   .../selftests/bpf/prog_tests/sockmap_basic.c  | 75 +++++++++++++++++++
>   .../bpf/progs/test_sockmap_progs_query.c      | 24 ++++++
>   2 files changed, 99 insertions(+)
>   create mode 100644 tools/testing/selftests/bpf/progs/test_sockmap_progs_query.c
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> index 1352ec104149..de8f91d91240 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> @@ -8,6 +8,7 @@
>   #include "test_sockmap_update.skel.h"
>   #include "test_sockmap_invalid_update.skel.h"
>   #include "test_sockmap_skb_verdict_attach.skel.h"
> +#include "test_sockmap_progs_query.skel.h"
>   #include "bpf_iter_sockmap.skel.h"
>   
>   #define TCP_REPAIR		19	/* TCP sock is under repair right now */
> @@ -315,6 +316,74 @@ static void test_sockmap_skb_verdict_attach(enum bpf_attach_type first,
>   	test_sockmap_skb_verdict_attach__destroy(skel);
>   }
>   
> +static __u32 query_prog_id(int prog_fd)
> +{
> +	struct bpf_prog_info info = {};
> +	__u32 info_len = sizeof(info);
> +	int err;
> +
> +	err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len);
> +	if (CHECK_FAIL(err || info_len != sizeof(info))) {
> +		perror("bpf_obj_get_info_by_fd");

Please use ASSERT_* macros. These macros are defined in test_progs.h.
We may have some old files still using CHECK* which are not converted
to ASSERT* yet. But for new contributions, we would like to use
ASSERT* from start. You can check other prog_tests/*.c files for
examples.

For the above example, you can do
	if (!ASSERT_OK(err, "bpf_obj_get_info_by_fd") ||
	    !ASSERT_EQ(info_len, sizeof(info), "bpf_obj_get_info_by_fd"))
		return 0;

> +		return 0;
> +	}
> +
> +	return info.id;
[...]
> +	err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> +			     &attach_flags, prog_ids, &prog_cnt);
> +	if (CHECK(err, "bpf_prog_query", "failed\n"))
> +		goto out;

In this case, you can use
	if (!ASSERT_OK(err, "bpf_prog_query"))
		goto out;

Please also change below other CHECK usages.

> +
> +	if (CHECK(attach_flags != 0, "bpf_prog_query",
> +		  "wrong attach_flags on query: %u", attach_flags))
> +		goto out;
> +
> +	if (CHECK(prog_cnt != 0, "bpf_prog_query",
> +		  "wrong program count on query: %u", prog_cnt))
> +		goto out;
> +
> +	err = bpf_prog_attach(verdict_fd, map_fd, attach_type, 0);
> +	if (CHECK(err, "bpf_prog_attach", "failed\n"))
> +		goto out;
> +
> +	prog_cnt = 1;
> +	err = bpf_prog_query(map_fd, attach_type, 0 /* query flags */,
> +			     &attach_flags, prog_ids, &prog_cnt);
> +
> +	CHECK(err, "bpf_prog_query", "failed\n");
> +	CHECK(attach_flags != 0, "bpf_prog_query attach_flags",
> +	      "wrong attach_flags on query: %u", attach_flags);
> +	CHECK(prog_cnt != 1, "bpf_prog_query prog_cnt",
> +	      "wrong program count on query: %u", prog_cnt);
> +	CHECK(prog_ids[0] != query_prog_id(verdict_fd), "bpf_prog_query",
> +	      "wrong prog_ids on query: %u", prog_ids[0]);
[...]