On Fri, Feb 17, 2023 at 04:14:12PM -0800, Andrii Nakryiko wrote:
> 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)
Done.
> > + * 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
Sure.
> > +{
> > + 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?
Sounds good.
> > + 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
> >
