On Tue, Jun 18, 2024 at 9:25 AM Alan Maguire <alan.maguire@xxxxxxxxxx> wrote:
>
> Use less verbose names in BTF relocation code and fix off-by-one error
> and typo in btf_relocate.c. Simplify loop over matching distilled
> types, moving from assigning a _next value in loop body to moving
> match check conditions into the guard.
>
> Suggested-by: Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx>
> Signed-off-by: Alan Maguire <alan.maguire@xxxxxxxxxx>
> ---
> tools/lib/bpf/btf_relocate.c | 74 ++++++++++++++++--------------------
> 1 file changed, 33 insertions(+), 41 deletions(-)
>
Few more nits, but generally looks great, thanks!
> diff --git a/tools/lib/bpf/btf_relocate.c b/tools/lib/bpf/btf_relocate.c
> index eabb8755f662..64cd8bdc0105 100644
> --- a/tools/lib/bpf/btf_relocate.c
> +++ b/tools/lib/bpf/btf_relocate.c
> @@ -160,7 +160,7 @@ static int btf_mark_embedded_composite_type_ids(struct btf_relocate *r, __u32 i)
> */
> static int btf_relocate_map_distilled_base(struct btf_relocate *r)
> {
> - struct btf_name_info *dist_base_info_sorted, *dist_base_info_sorted_end;
> + struct btf_name_info *info, *info_end;
> struct btf_type *base_t, *dist_t;
> __u8 *base_name_cnt = NULL;
> int err = 0;
> @@ -169,26 +169,25 @@ static int btf_relocate_map_distilled_base(struct btf_relocate *r)
> /* generate a sort index array of name/type ids sorted by name for
> * distilled base BTF to speed name-based lookups.
> */
> - dist_base_info_sorted = calloc(r->nr_dist_base_types, sizeof(*dist_base_info_sorted));
> - if (!dist_base_info_sorted) {
> + info = calloc(r->nr_dist_base_types, sizeof(*info));
> + if (!info) {
> err = -ENOMEM;
> goto done;
> }
> - dist_base_info_sorted_end = dist_base_info_sorted + r->nr_dist_base_types;
> + info_end = info + r->nr_dist_base_types;
> for (id = 0; id < r->nr_dist_base_types; id++) {
> dist_t = btf_type_by_id(r->dist_base_btf, id);
> - dist_base_info_sorted[id].name = btf__name_by_offset(r->dist_base_btf,
> - dist_t->name_off);
> - dist_base_info_sorted[id].id = id;
> - dist_base_info_sorted[id].size = dist_t->size;
> - dist_base_info_sorted[id].needs_size = true;
> + info[id].name = btf__name_by_offset(r->dist_base_btf,
> + dist_t->name_off);
please make it a single line, now that it's much shorter
> + info[id].id = id;
> + info[id].size = dist_t->size;
> + info[id].needs_size = true;
> }
[...]
> /* iterate over all matching distilled base types */
> - for (dist_name_info = search_btf_name_size(&base_name_info, dist_base_info_sorted,
> - r->nr_dist_base_types);
> - dist_name_info != NULL; dist_name_info = dist_name_info_next) {
> - /* Are there more distilled matches to process after
> - * this one?
> - */
> - dist_name_info_next = dist_name_info + 1;
> - if (dist_name_info_next >= dist_base_info_sorted_end ||
> - cmp_btf_name_size(&base_name_info, dist_name_info_next))
> - dist_name_info_next = NULL;
> -
> - if (!dist_name_info->id || dist_name_info->id > r->nr_dist_base_types) {
> + for (dist_info = search_btf_name_size(&base_info, info,
> + r->nr_dist_base_types);
does it fit under 100? please prioritize keeping single-line code as
much as possible
> + dist_info && dist_info < info_end &&
I missed the need for `dist_info < info_end` in my original
suggestion, but yes, this looks much better, thanks (and yeah, I don't
think one extra cmp_btf_name_size() call matters much).
> + !cmp_btf_name_size(&base_info, dist_info);
nit: given this is strcmp()-like function, I'd prefer explicit `== 0`
instead of boolean-like !
> + dist_info++) {
> + if (!dist_info->id || dist_info->id >= r->nr_dist_base_types) {
> pr_warn("base BTF id [%d] maps to invalid distilled base BTF id [%d]\n",
> - id, dist_name_info->id);
> + id, dist_info->id);
> err = -EINVAL;
> goto done;
> }
> - dist_t = btf_type_by_id(r->dist_base_btf, dist_name_info->id);
> + dist_t = btf_type_by_id(r->dist_base_btf, dist_info->id);
> dist_kind = btf_kind(dist_t);
>
[...]
