Re: [PATCH bpf-next v2 3/5] libbpf: add subskeleton scaffolding

[Delyan Kratunov <delyank@fb.com>]

Thanks Andrii!

On Fri, 2022-03-11 at 15:08 -0800, Andrii Nakryiko wrote:
> On Fri, Mar 11, 2022 at 2:52 PM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> 
> > 

[...]

> > > +
> > > +       err = populate_skeleton_maps(s->obj, s->maps, s->map_cnt);
> > > +       if (err) {
> > > +               pr_warn("failed to populate subskeleton maps: %d\n", err);
> > > +               return libbpf_err(err);
> > >         }
> > > 
> > > -       for (i = 0; i < s->prog_cnt; i++) {
> > > -               struct bpf_program **prog = s->progs[i].prog;
> > > -               const char *name = s->progs[i].name;
> > > +       err = populate_skeleton_progs(s->obj, s->progs, s->prog_cnt);
> > > +       if (err) {
> > > +               pr_warn("failed to populate subskeleton maps: %d\n", err);
> > > +               return libbpf_err(err);
> > > +       }
> > > 
> > > -               *prog = bpf_object__find_program_by_name(obj, name);
> > > -               if (!*prog) {
> > > -                       pr_warn("failed to find skeleton program '%s'\n", name);
> > > -                       return libbpf_err(-ESRCH);
> > > +       for (var_idx = 0; var_idx < s->var_cnt; var_idx++) {
> > > +               var_skel = &s->vars[var_idx];
> > > +               map = *var_skel->map;
> > > +               map_type_id = bpf_map__btf_value_type_id(map);
> > > +               map_type = btf__type_by_id(btf, map_type_id);
> > > +
> > 
> > should we double-check that map_type is DATASEC?

Sure, can do.

> > 
> > > +               len = btf_vlen(map_type);
> > > +               var = btf_var_secinfos(map_type);
> > > +               for (i = 0; i < len; i++, var++) {
> > > +                       var_type = btf__type_by_id(btf, var->type);
> > > +                       if (!var_type) {
> > 
> > unless BTF itself is corrupted, this shouldn't ever happen. So
> > checking for DATASEC should be enough and this if (!var_type) is
> > redundant
> > 
> > > +                               pr_warn("Could not find var type for item %1$d in section %2$s",
> > > +                                       i, bpf_map__name(map));
> > > +                               return libbpf_err(-EINVAL);
> > > +                       }
> > > +                       var_name = btf__name_by_offset(btf, var_type->name_off);
> > > +                       if (strcmp(var_name, var_skel->name) == 0) {
> > > +                               *var_skel->addr = (char *) map->mmaped + var->offset;
> > 
> > is (char *) cast necessary? C allows pointer adjustment on void *, so
> > shouldn't be
> 
> oh, wait, it's so that C++ compiler doesn't complain, never mind

This is libbpf code, not subskel code, so it shouldn't get compiled as C++. It's
really because of -Wpointer-arith and -Werror.

> 
> > 
> > > +                               break;
> > > +                       }
> > >                 }
> > >         }
> > > -
> > >         return 0;
> > >  }
> > > 
> > 
> > [...]
> > 
> > >  struct gen_loader_opts {
> > >         size_t sz; /* size of this struct, for forward/backward compatiblity */
> > >         const char *data;
> > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > > index df1b947792c8..d744fbb8612e 100644
> > > --- a/tools/lib/bpf/libbpf.map
> > > +++ b/tools/lib/bpf/libbpf.map
> > > @@ -442,6 +442,8 @@ LIBBPF_0.7.0 {
> > > 
> > >  LIBBPF_0.8.0 {
> > >         global:
> > > +               bpf_object__open_subskeleton;
> > > +               bpf_object__destroy_subskeleton;
> > 
> > nit: should be in alphabetic order
> > 
> > >                 libbpf_register_prog_handler;
> > >                 libbpf_unregister_prog_handler;
> > >  } LIBBPF_0.7.0;
> > > --
> > > 2.34.1