On Tue, Jan 14, 2020 at 05:10:03PM -0800, Andrii Nakryiko wrote:
> On Tue, Jan 14, 2020 at 2:44 PM Martin KaFai Lau <kafai@xxxxxx> wrote:
> >
> > When testing a map has btf or not, maps_have_btf() tests it by actually
> > getting a btf_fd from sys_bpf(BPF_BTF_GET_FD_BY_ID). However, it
> > forgot to btf__free() it.
> >
> > In maps_have_btf() stage, there is no need to test it by really
> > calling sys_bpf(BPF_BTF_GET_FD_BY_ID). Testing non zero
> > info.btf_id is good enough.
> >
> > Also, the err_close case is unnecessary, and also causes double
> > close() because the calling func do_dump() will close() all fds again.
> >
> > Fixes: 99f9863a0c45 ("bpftool: Match maps by name")
> > Cc: Paul Chaignon <paul.chaignon@xxxxxxxxxx>
> > Signed-off-by: Martin KaFai Lau <kafai@xxxxxx>
> > ---
>
> this is clearly a simplification, but isn't do_dump still buggy? see below
>
> > tools/bpf/bpftool/map.c | 16 ++--------------
> > 1 file changed, 2 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/map.c b/tools/bpf/bpftool/map.c
> > index c01f76fa6876..e00e9e19d6b7 100644
> > --- a/tools/bpf/bpftool/map.c
> > +++ b/tools/bpf/bpftool/map.c
> > @@ -915,32 +915,20 @@ static int maps_have_btf(int *fds, int nb_fds)
> > {
> > struct bpf_map_info info = {};
> > __u32 len = sizeof(info);
> > - struct btf *btf = NULL;
> > int err, i;
> >
> > for (i = 0; i < nb_fds; i++) {
> > err = bpf_obj_get_info_by_fd(fds[i], &info, &len);
> > if (err) {
> > p_err("can't get map info: %s", strerror(errno));
> > - goto err_close;
> > - }
> > -
> > - err = btf__get_from_id(info.btf_id, &btf);
> > - if (err) {
> > - p_err("failed to get btf");
> > - goto err_close;
> > + return -1;
> > }
> >
> > - if (!btf)
> > + if (!info.btf_id)
> > return 0;
>
> if info.btf_id is non-zero, shouldn't we immediately return 1 and be
> done with it?
No. maps_have_btf() returns 1 only if all the maps have btf.
>
> I'm also worried about do_dump logic. What's the behavior when some
> maps do have BTF and some don't? Should we use btf_writer for all,
> some or none maps for that case?
For plain_text, btf output is either for all or for none.
It is the intention of the "Fixes" patch if I read it correctly,
and it is kept as is in this bug fix.
It will become clear by doing a plain text dump on maps with and
without btf. They are very different.
Can the output format for with and without BTF somehow merged for
plain text? May be if it is still common to have no-BTF map
going forward but how this may look like will need another
discussion.
> I'd expect we'd use BTF info for
> those maps that have BTF and fall back to raw output for those that
> don't, but I'm not sure that how code behaves right now.
The json_output is doing what you described, print BTF info
whenever available.
>
> Maybe Paul can clarify...
>
>
> > }
> >
> > return 1;
> > -
> > -err_close:
> > - for (; i < nb_fds; i++)
> > - close(fds[i]);
> > - return -1;
> > }
> >
> > static int
> > --
> > 2.17.1
> >
