Date:   Mon, 31 Aug 2020 08:40:01 -0700
From:   sdf@...gle.com
To:     "Toke Høiland-Jørgensen" <toke@...hat.com>
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, davem@...emloft.net,
        ast@...nel.org, daniel@...earbox.net,
        YiFei Zhu <zhuyifei1999@...il.com>
Subject: Re: [PATCH bpf-next v3 4/8] libbpf: implement bpf_prog_find_metadata

On 08/28, Toke H�iland-J�rgensen wrote:
> Stanislav Fomichev <sdf@...gle.com> writes:

> > This is a low-level function (hence in bpf.c) to find out the metadata
> > map id for the provided program fd.
> > It will be used in the next commits from bpftool.
> >
> > Cc: Toke H�iland-J�rgensen <toke@...hat.com>
> > Cc: YiFei Zhu <zhuyifei1999@...il.com>
> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > ---
> >  tools/lib/bpf/bpf.c      | 74 ++++++++++++++++++++++++++++++++++++++++
> >  tools/lib/bpf/bpf.h      |  1 +
> >  tools/lib/bpf/libbpf.map |  1 +
> >  3 files changed, 76 insertions(+)
> >
> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index 5f6c5676cc45..01c0ede1625d 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -885,3 +885,77 @@ int bpf_prog_bind_map(int prog_fd, int map_fd,
> >
> >  	return sys_bpf(BPF_PROG_BIND_MAP, &attr, sizeof(attr));
> >  }
> > +
> > +int bpf_prog_find_metadata(int prog_fd)
> > +{
> > +	struct bpf_prog_info prog_info = {};
> > +	struct bpf_map_info map_info;
> > +	__u32 prog_info_len;
> > +	__u32 map_info_len;
> > +	int saved_errno;
> > +	__u32 *map_ids;
> > +	int nr_maps;
> > +	int map_fd;
> > +	int ret;
> > +	int i;
> > +
> > +	prog_info_len = sizeof(prog_info);
> > +
> > +	ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!prog_info.nr_map_ids)
> > +		return -1;
> > +
> > +	map_ids = calloc(prog_info.nr_map_ids, sizeof(__u32));
> > +	if (!map_ids)
> > +		return -1;
> > +
> > +	nr_maps = prog_info.nr_map_ids;
> > +	memset(&prog_info, 0, sizeof(prog_info));
> > +	prog_info.nr_map_ids = nr_maps;
> > +	prog_info.map_ids = ptr_to_u64(map_ids);
> > +	prog_info_len = sizeof(prog_info);
> > +
> > +	ret = bpf_obj_get_info_by_fd(prog_fd, &prog_info, &prog_info_len);
> > +	if (ret)
> > +		goto free_map_ids;
> > +
> > +	ret = -1;
> > +	for (i = 0; i < prog_info.nr_map_ids; i++) {
> > +		map_fd = bpf_map_get_fd_by_id(map_ids[i]);
> > +		if (map_fd < 0) {
> > +			ret = -1;
> > +			goto free_map_ids;
> > +		}
> > +
> > +		memset(&map_info, 0, sizeof(map_info));
> > +		map_info_len = sizeof(map_info);
> > +		ret = bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len);
> > +		saved_errno = errno;
> > +		close(map_fd);
> > +		errno = saved_errno;
> > +		if (ret)
> > +			goto free_map_ids;

> If you get to this point on the last entry in the loop, ret will be 0,
> and any of the continue statements below will end the loop, causing the
> whole function to return 0. While this is not technically a valid ID, it
> still seems odd that the function returns -1 on all error conditions
> except this one.

> Also, it would be good to be able to unambiguously distinguish between
> "this program has no metadata associated" and "something went wrong
> while querying the kernel for metadata (e.g., permission error)". So
> something that amounts to a -ENOENT return; I guess turning all return
> values into negative error codes would do that (and also do away with
> the need for the saved_errno dance above), but id does clash a bit with
> the convention in the rest of the file (where all the other functions
> just return -1 and set errno)...
Good point. I think I can change the function signature to:

	int bpf_prog_find_metadata(int prog_fd, int *map_id)

And explicitly return map_id via argument. Then the ret can be used as
-1/0 error and I can set errno appropriately where it makes sense.
This will better match the convention we have in this file.

Will respin v4 later this week to give people opportunity to look at the
other patches in the series. Thanks!

Powered by blists - more mailing lists