Re: [PATCH bpf-next 1/2] libbpf: Move core "types_are_compat" logic into relo_core.c

["Daniel Müller" <deso@posteo.net>]

On Wed, Jun 22, 2022 at 09:15:01PM -0700, Andrii Nakryiko wrote:
> On Wed, Jun 22, 2022 at 10:35 AM Daniel Müller <deso@posteo.net> wrote:
> >
> > This change merges the two existing implementations of the
> > bpf_core_types_are_compat() function into relo_core.c, inheriting the
> > recursion tracking from the kernel and the usage of
> > btf_kind_core_compat() from libbpf. The kernel is left untouched and
> > will be adjusted subsequently.
> >
> > Signed-off-by: Daniel Müller <deso@posteo.net>
> > ---
> 
> I don't feel very strongly about this, but given we are consolidating
> kernel and libbpf code, I think it makes sense to do it in one patch.

Sure.


> >  tools/lib/bpf/libbpf.c    | 72 +-----------------------------------
> >  tools/lib/bpf/relo_core.c | 78 +++++++++++++++++++++++++++++++++++++++
> >  tools/lib/bpf/relo_core.h |  2 +
> >  3 files changed, 81 insertions(+), 71 deletions(-)
> >
> 
> [...]
> 
> > -       default:
> > -               pr_warn("unexpected kind %s relocated, local [%d], target [%d]\n",
> > -                       btf_kind_str(local_type), local_id, targ_id);
> > -               return 0;
> > -       }
> > +       return bpf_core_types_are_compat_recur(local_btf, local_id, targ_btf, targ_id, INT_MAX);
> 
> INT_MAX seems like an overkill, let's just hard-code 32 just like we
> have for a local recursion limit here?

Okay.

> >  }
> >
> 
> [...]
> 
> >  /*
> >   * Turn bpf_core_relo into a low- and high-level spec representation,
> >   * validating correctness along the way, as well as calculating resulting
> > diff --git a/tools/lib/bpf/relo_core.h b/tools/lib/bpf/relo_core.h
> > index 7df0da0..b8998f 100644
> > --- a/tools/lib/bpf/relo_core.h
> > +++ b/tools/lib/bpf/relo_core.h
> > @@ -68,6 +68,8 @@ struct bpf_core_relo_res {
> >         __u32 new_type_id;
> >  };
> >
> > +int bpf_core_types_are_compat_recur(const struct btf *local_btf, __u32 local_id,
> > +                                   const struct btf *targ_btf, __u32 targ_id, int level);
> 
> Just leave it called __bpf_core_types_are_compat like in kernel, it is
> clearly an "internal" version of bpf_core_types_are_compat(), so it's
> more proper naming convention.

Sounds good.

[...]

Thanks,
Daniel