On Mon, Aug 31, 2020 at 08:40:01AM -0700, sdf@xxxxxxxxxx wrote:
> On 08/28, Toke H�iland-J�rgensen wrote:
> > Stanislav Fomichev <sdf@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > Cc: YiFei Zhu <zhuyifei1999@xxxxxxxxx>
> > > Signed-off-by: Stanislav Fomichev <sdf@xxxxxxxxxx>
> > > ---
> > > 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.
I don't feel great about this libbpf api. bpftool already does
bpf_obj_get_info_by_fd() for progs and for maps.
This extra step and extra set of syscalls is redundant work.
I think it's better to be done as part of bpftool.
It doesn't quite fit as generic api.
