On Fri, Feb 17, 2023 at 11:19 AM Daniel Müller <deso@xxxxxxxxxx> wrote:
>
> This change splits the elf_find_func_offset() function in two:
> elf_find_func_offset(), which now accepts an already opened Elf object
> instead of a path to a file that is to be opened, as well as
> elf_find_func_offset_from_elf_file(), which opens a binary based on a
> path and then invokes elf_find_func_offset() on the Elf object. Having
> this split in responsibilities will allow us to call
> elf_find_func_offset() from other code paths on Elf objects that did not
> necessarily come from a file on disk.
>
> Signed-off-by: Daniel Müller <deso@xxxxxxxxxx>
> ---
> tools/lib/bpf/libbpf.c | 55 +++++++++++++++++++++++++++---------------
> 1 file changed, 35 insertions(+), 20 deletions(-)
>
Looks good, just few pedantic nits
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 05c4db3..a474f49 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -10531,32 +10531,19 @@ static Elf_Scn *elf_find_next_scn_by_type(Elf *elf, int sh_type, Elf_Scn *scn)
> return NULL;
> }
>
> -/* Find offset of function name in object specified by path. "name" matches
> - * symbol name or name@@LIB for library functions.
> +/* Find offset of function name in the provided ELF object. "binary_path" is
> + * the path to the ELF binary represented by "elf", and only used for error
> + * reporting matters. "name" matches symbol name or name@@LIB for library
> + * functions.
> */
> -static long elf_find_func_offset(const char *binary_path, const char *name)
> +static long elf_find_func_offset(Elf *elf, const char *binary_path, const char *name)
> {
> - int fd, i, sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB };
> + int i, sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB };
> bool is_shared_lib, is_name_qualified;
> - char errmsg[STRERR_BUFSIZE];
> long ret = -ENOENT;
> size_t name_len;
> GElf_Ehdr ehdr;
> - Elf *elf;
>
> - fd = open(binary_path, O_RDONLY | O_CLOEXEC);
> - if (fd < 0) {
> - ret = -errno;
> - pr_warn("failed to open %s: %s\n", binary_path,
> - libbpf_strerror_r(ret, errmsg, sizeof(errmsg)));
> - return ret;
> - }
> - elf = elf_begin(fd, ELF_C_READ_MMAP, NULL);
> - if (!elf) {
> - pr_warn("elf: could not read elf from %s: %s\n", binary_path, elf_errmsg(-1));
> - close(fd);
> - return -LIBBPF_ERRNO__FORMAT;
> - }
> if (!gelf_getehdr(elf, &ehdr)) {
> pr_warn("elf: failed to get ehdr from %s: %s\n", binary_path, elf_errmsg(-1));
> ret = -LIBBPF_ERRNO__FORMAT;
> @@ -10682,6 +10669,34 @@ static long elf_find_func_offset(const char *binary_path, const char *name)
> }
> }
> out:
> + return ret;
> +}
> +
> +/* Find offset of function name in ELF object specified by path. "name" matches
nit: seems like it's original spelling, but let's remove these double
spaces (same above in elf_find_func_offset comment)
> + * symbol name or name@@LIB for library functions.
> + */
> +static long elf_find_func_offset_from_elf_file(const char *binary_path, const char *name)
"from_file" would be enough, reads a bit tautological otherwise
> +{
> + char errmsg[STRERR_BUFSIZE];
> + long ret = -ENOENT;
> + Elf *elf;
> + int fd;
> +
> + fd = open(binary_path, O_RDONLY | O_CLOEXEC);
btw, this reminded me that in patch #1 we probably want to pass
O_CLOEXEC as well?
> + if (fd < 0) {
> + ret = -errno;
> + pr_warn("failed to open %s: %s\n", binary_path,
> + libbpf_strerror_r(ret, errmsg, sizeof(errmsg)));
> + return ret;
> + }
> + elf = elf_begin(fd, ELF_C_READ_MMAP, NULL);
> + if (!elf) {
> + pr_warn("elf: could not read elf from %s: %s\n", binary_path, elf_errmsg(-1));
> + close(fd);
> + return -LIBBPF_ERRNO__FORMAT;
> + }
> +
> + ret = elf_find_func_offset(elf, binary_path, name);
> elf_end(elf);
> close(fd);
> return ret;
> @@ -10805,7 +10820,7 @@ bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> if (func_name) {
> long sym_off;
>
> - sym_off = elf_find_func_offset(binary_path, func_name);
> + sym_off = elf_find_func_offset_from_elf_file(binary_path, func_name);
> if (sym_off < 0)
> return libbpf_err_ptr(sym_off);
> func_offset += sym_off;
> --
> 2.30.2
>
