Re: [PATCH bpf-next 1/5] libbpf: add BTF field iterator

[Eduard Zingerman <eddyz87@gmail.com>]

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@kernel.org>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

> +__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 :)

> +
> +	it->p = NULL;
> +	return NULL;
> +}
> +

[...]