Re: [PATCH mptcp-next v12 2/5] Squash to "selftests: bpf: add MPTCP test base"

[Geliang Tang <geliang.tang@suse.com>]

Hi Matt,

On Thu, Mar 24, 2022 at 06:05:03PM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> Thank you for this new version!
> 
> On 24/03/2022 14:28, Geliang Tang wrote:
> > fix libbpf 0.8 build break:
> > 
> > prog_tests/mptcp.c:176:2: error: 'bpf_prog_load_xattr' is deprecated: libbpf v0.8+: use bpf_object__open() and bpf_object__load() instead [-Werror=deprecated-declarations]
> 
> These modifications seem good but while updating this test to sync with
> what has done upstream recently, maybe it would also be good to use
> macros that seem to be used in many places now, WDYT?
> Please see below:
> 
> (...)
> 
> > @@ -44,28 +40,46 @@ static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 is_mptcp)
> >  
> >  static int run_test(int cgroup_fd, int server_fd, bool is_mptcp)
> >  {
> > -	int client_fd, prog_fd, map_fd, err;
> > +	int client_fd, prog_fd, map_fd;
> > +	const char *file = "./mptcp.o";
> > +	struct bpf_program *prog;
> >  	struct bpf_object *obj;
> >  	struct bpf_map *map;
> > +	int err = 0;
> >  
> > -	struct bpf_prog_load_attr attr = {
> > -		.prog_type = BPF_PROG_TYPE_SOCK_OPS,
> > -		.file = "./mptcp.o",
> > -		.expected_attach_type = BPF_CGROUP_SOCK_OPS,
> > -	};
> > +	obj = bpf_object__open(file);
> 
> Does bpf_object__open() not take two parameters?

bpf_object__open_file() takes two parameters, this one takes one :)

> 
> > +	if (libbpf_get_error(obj))
> 
> I was surprised not to see a check 'obj != NULL', e.g.
> 
>   if (!ASSERT_OK_PTR(obj, "bpf_obj_open"))

This check had been done in libbpf_get_error().

> 
> like it is done in some other selftests but I see in other selftests
> they use libbpf_get_error() like here.
> 
> I guess that's fine but I don't find a proper doc explaining what to use
> here :)
> 
> I would still recommend to use ASSERT_OK_PTR(), at least to have a clear
> error message.
> 
> 
> > +		return -1;
> >  
> > -	err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
> > +	err = bpf_object__load(obj);
> >  	if (err) {
> 
> Should you use ASSERT*() and CHECK() macros to include the 'log_err()'
> below?
> 
>   if (!ASSERT_OK(err, "bpf_obj_load")) {
> 
> In 2020, these macro were not common but it looks like there are more
> and more used now, e.g. check commit 186d1a86003d ("selftests/bpf:
> Remove all the uses of deprecated bpf_prog_load_xattr()")

Updated in v13.

> 
> >  		log_err("Failed to load BPF object");
> > -		return -1;
> > +		err = -1;
> 
> I see some selftests returning "-EIO", maybe better here?
> (same below)

Yes, -EIO is much better, updated in v13.

> 
> > +		goto close_bpf_object;
> >  	}
> >  
> > +	prog = bpf_object__next_program(obj, NULL);
> 
> It seems more common to use bpf_object__find_program_by_name() just in
> case a new program is added later I guess. But maybe fine now?

Agree, updated in v13.

> 
> > +	if (!prog) {> +		log_err("Failed to get BPF program");
> 
> Maybe good to use:
> 
>   CHECK(!prog, "find_prog", "mptcp prog not found\n")
> 
> > +		err = -1;
> > +		goto close_bpf_object;
> > +	}
> > +
> > +	prog_fd = bpf_program__fd(prog);
> > +
> >  	map = bpf_object__next_map(obj, NULL);
> 
> Similar here: maybe better to use bpf_object__find_map_by_name()?

Updated in v13.

> 
> > +	if (!map) {> +		log_err("Failed to get BPF map");
> 
> Maybe good to use CHECK() here?

Yes.

> 
> > +		err = -1;
> > +		goto close_bpf_object;
> > +	}
> > +
> >  	map_fd = bpf_map__fd(map);
> >  
> >  	err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_SOCK_OPS, 0);
> >  	if (err) {
> >  		log_err("Failed to attach BPF program");
> 
> And use CHECK() here as well?

Yes.

> 
> > +		err = -1;
> 
> 'err' should already be != 0 here. Do you need to set it explicitly to -1?

No, updated in v13.

Thanks,
-Geliang

> 
> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>