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

[zhudi (E)]

> 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;

I refer to the implementation of the old codes, I will modify it
Thanks,


> > +		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]);
> [...]