On Tue, Jun 4, 2024 at 1:37 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Mon, 2024-06-03 at 16:17 -0700, Andrii Nakryiko wrote:
> > Implement iterator-based type ID and string offset BTF field iterator.
> > This is used extensively in BTF-handling code and BPF linker code for
> > various sanity checks, rewriting IDs/offsets, etc. Currently this is
> > implemented as visitor pattern calling custom callbacks, which makes the
> > logic (especially in simple cases) unnecessarily obscure and harder to
> > follow.
> >
> > Having equivalent functionality using iterator pattern makes for simpler
> > to understand and maintain code. As we add more code for BTF processing
> > logic in libbpf, it's best to switch to iterator pattern before adding
> > more callback-based code.
> >
> > The idea for iterator-based implementation is to record offsets of
> > necessary fields within fixed btf_type parts (which should be iterated
> > just once), and, for kinds that have multiple members (based on vlen
> > field), record where in each member necessary fields are located.
> >
> > Generic iteration code then just keeps track of last offset that was
> > returned and handles N members correctly. Return type is just u32
> > pointer, where NULL is returned when all relevant fields were already
> > iterated.
> >
> > Signed-off-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> > ---
>
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
>
> [...]
>
> > +__u32 *btf_field_iter_next(struct btf_field_iter *it)
> > +{
> > + if (!it->p)
> > + return NULL;
> > +
> > + if (it->m_idx < 0) {
> > + if (it->off_idx < it->desc.t_cnt)
> > + return it->p + it->desc.t_offs[it->off_idx++];
> > + /* move to per-member iteration */
> > + it->m_idx = 0;
> > + it->p += sizeof(struct btf_type);
> > + it->off_idx = 0;
> > + }
> > +
> > + /* if type doesn't have members, stop */
> > + if (it->desc.m_sz == 0) {
> > + it->p = NULL;
> > + return NULL;
> > + }
> > +
> > + if (it->off_idx >= it->desc.m_cnt) {
> > + /* exhausted this member's fields, go to the next member */
> > + it->m_idx++;
> > + it->p += it->desc.m_sz;
> > + it->off_idx = 0;
> > + }
> > +
> > + if (it->m_idx < it->vlen)
> > + return it->p + it->desc.m_offs[it->off_idx++];
>
> Nit: it is a bit confusing that for two 'if' statements above
> m_idx is guarded by vlen and off_idx is guarded by m_cnt :)
I'm open to suggestions. m_idx stands for "current member index",
m_cnt is for "per-member offset count", while "off_idx" is generic
"offset index" which indexes either a singular set of offsets or
per-member set of offsets. Easy ;)
>
> > +
> > + it->p = NULL;
> > + return NULL;
> > +}
> > +
>
> [...]
