Re: [PATCHv2 bpf-next 09/24] libbpf: Add elf_find_pattern_func_offset function

Andrii Nakryiko
2023-06-23T20:40:53Z

On Tue, Jun 20, 2023 at 1:37 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding elf_find_pattern_func_offset function that looks up
> offsets for symbols specified by pattern argument.
>
> The 'pattern' argument allows wildcards (*?' supported).
>
> Offsets are returned in allocated array together with its
> size and needs to be released by the caller.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/lib/bpf/libbpf.c | 78 +++++++++++++++++++++++++++++++++
> tools/lib/bpf/libbpf_internal.h | 3 ++
> 2 files changed, 81 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 1c310b718961..3e5c88caf5d5 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -11165,6 +11165,84 @@ int elf_find_multi_func_offset(const char *binary_path, int cnt,
> return ret;
> }
>
> +static int
> +__elf_find_pattern_func_offset(Elf *elf, const char *binary_path, const char *pattern,
> + const char ***pnames, unsigned long **poffsets, size_t *pcnt)
> +{
> + int sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB };
> + struct elf_symbol_offset *func_offs = NULL;
> + unsigned long *offsets = NULL;
> + const char **names = NULL;
> + size_t func_offs_cnt = 0;
> + size_t func_offs_cap = 0;
> + int err = 0, i;
> +
> + for (i = 0; i < ARRAY_SIZE(sh_types); i++) {
> + struct elf_symbol_iter iter;
> + struct elf_symbol *sym;
> +
> + if (elf_symbol_iter_new(&iter, elf, binary_path, sh_types[i]))
> + continue;

same as in the previous patch, error handling?

> +
> + while ((sym = elf_symbol_iter_next(&iter))) {
> + if (!glob_match(sym->name, pattern))
> + continue;
> +
> + err = libbpf_ensure_mem((void **) &func_offs, &func_offs_cap,
> + sizeof(*func_offs), func_offs_cnt + 1);
> + if (err)
> + goto out;
> +
> + func_offs[func_offs_cnt].offset = sym->offset;
> + func_offs[func_offs_cnt].name = strdup(sym->name);

check for NULL?

and I'm actually unsure why you need to reuse elf_symbol_offset struct
here? You just need names and offsets in two separate array, so just
do that?..

> + func_offs_cnt++;
> + }
> +
> + /* If we found anything in the first symbol section,
> + * do not search others to avoid duplicates.
> + */
> + if (func_offs_cnt)
> + break;
> + }
> +
> + offsets = calloc(func_offs_cnt, sizeof(*offsets));
> + names = calloc(func_offs_cnt, sizeof(*names));
> + if (!offsets || !names) {
> + free(offsets);
> + free(names);
> + err = -ENOMEM;
> + goto out;
> + }
> +
> + for (i = 0; i < func_offs_cnt; i++) {
> + offsets[i] = func_offs[i].offset;
> + names[i] = func_offs[i].name;

see above, why not fill these out right away during elf symbols iteration?

> + }
> +
> + *pnames = names;
> + *poffsets = offsets;
> + *pcnt = func_offs_cnt;
> +out:
> + free(func_offs);
> + return err;
> +}
> +
> +int elf_find_pattern_func_offset(const char *binary_path, const char *pattern,
> + const char ***pnames, unsigned long **poffsets,
> + size_t *pcnt)
> +{
> + struct elf_fd elf_fd = {};
> + long ret = -ENOENT;
> +
> + ret = open_elf(binary_path, &elf_fd);
> + if (ret)
> + return ret;
> +
> + ret = __elf_find_pattern_func_offset(elf_fd.elf, binary_path, pattern, pnames, poffsets, pcnt);

I don't really like these underscored functions,
elf_find_pattern_func_offset() already has to do goto out for clean
ups, this close_elf() thing is just another resource to clean up
there, I don't see much reason to separate open_elf/close_elf in this
case

> + close_elf(&elf_fd);
> + return ret;
> +}
> +
> /* Find offset of function name in ELF object specified by path. "name" matches
> * symbol name or name@@LIB for library functions.
> */
> diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> index 13d5c12fbd0b..22b0834e7fe1 100644
> --- a/tools/lib/bpf/libbpf_internal.h
> +++ b/tools/lib/bpf/libbpf_internal.h
> @@ -579,4 +579,7 @@ int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size, int attempts);
>
> int elf_find_multi_func_offset(const char *binary_path, int cnt,
> const char **syms, unsigned long **poffsets);
> +int elf_find_pattern_func_offset(const char *binary_path, const char *pattern,
> + const char ***pnames, unsigned long **poffsets,
> + size_t *pcnt);
> #endif /* __LIBBPF_LIBBPF_INTERNAL_H */
> --
> 2.41.0
>