On Fri, Mar 11, 2022 at 3:28 PM Delyan Kratunov <delyank@xxxxxx> wrote:
>
> 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@xxxxxxxxx> 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.
Oh, if it's libbpf code then it shouldn't be necessary, after all. I'm
pretty sure we assume in many places that we can do pointer arithmetic
on void *. Did you try and it didn't compile? Or you just did it
preemptively?
>
> >
> > >
> > > > + 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
>
