Subject: Re: [PATCH 01/11] bpf: Add btfid tool to resolve BTF IDs in ELF object

From: Jiri Olsa <[email-masked]>
Date: Mon, 22 Jun 2020 10:59:26 +0200

On Fri, Jun 19, 2020 at 11:12:29AM -0700, Andrii Nakryiko wrote:

SNIP

> >
> > ok
> >
> > >
> > > > +                       root = &obj->funcs;
> > > > +                       nr = &nr_funcs;
> > > > +               } else if (BTF_INFO_KIND(type->info) == BTF_KIND_STRUCT && nr_structs) {
> > >
> > > same as above: btf_is_struct
> > >
> > > But I think you also need to support unions?
> > >
> > > Also what about typedefs? A lot of types are typedefs to struct/func_proto/etc.
> >
> > I added only types which are needed at the moment, but maybe
> > we can add the basic types now, so we don't need to bother later,
> > when we forget how this all work ;-)
> 
> yeah, exactly. Once this works, no one will want to go and revisit it,
> so I'd rather make it generic from the get go, especially that it's
> really easy in this case, right?

right, I'll add the basic types

SNIP

> > >
> > > nit: should these elf_end/close properly on error?
> >
> > I wrote in the comment above that I intentionaly do not cleanup
> > on error path.. I wanted to save some time, but actualy I think
> > that would not be so expensive, I can add it
> 
> as in save CPU time in error case? Who cares about that? If saving
> developer time, well... `goto cleanup` is common and simple pattern ;)

the sooner you see the error the better ;-) ok

jirka