On Tue, 2012-11-13 at 21:31 +0100, Jan Kratochvil wrote: > dependent on: > [patch 2/4] unwinder: Provide EBLHOOKVAR
It doesn't seem to use EBLHOOKVAR. > Currently dwfl_core_file_report requires Elf *core. But application cannot > open Elf * on its own, it is elfutils internal function. > [...] > It is also already suspicious why libdwfl/argp-std.c is using elfutils > internal function, application should be able to implement parsing commandline > parameters on its own. So the issue is that __libdw_open_file does a bit more than elf_begin? In particular it handles compressed images. I do agree that it would be nice to be able to handle that transparently without needing access to an internal function, so a quick review below. But I do wonder if you cannot just directly open the Elf *core with elf_begin in your use case? > libdw/ > 2012-11-13 Jan Kratochvil <[email protected]> > > * libdw.map (ELFUTILS_0.156): New block. > > libdwfl/ > 2012-11-13 Jan Kratochvil <[email protected]> > > * argp-std.c (parse_opt): Remove label nofile. Substitute code by > a dwfl_core_filename_report call. Initialize error from dwfl_errno > now. Check dwfl->modulelist instead of former RESULT. > * core-file.c: Include fcntl.h. > (dwfl_core_filename_report): New function. > * libdwfl.h (dwfl_core_filename_report): New declaration. > > diff --git a/libdw/libdw.map b/libdw/libdw.map > index 1f71d03..5a055e8 100644 > --- a/libdw/libdw.map > +++ b/libdw/libdw.map > @@ -254,3 +254,8 @@ ELFUTILS_0.149 { > > dwfl_dwarf_line; > } ELFUTILS_0.148; > + > +ELFUTILS_0.156 { > + global: > + dwfl_core_filename_report; > +} ELFUTILS_0.149; > diff --git a/libdwfl/argp-std.c b/libdwfl/argp-std.c > index f0c530f..8aeb80a 100644 > --- a/libdwfl/argp-std.c > +++ b/libdwfl/argp-std.c > @@ -205,7 +205,6 @@ parse_opt (int key, char *arg, struct argp_state *state) > { > FILE *f = fopen (arg, "r"); > if (f == NULL) > - nofile: > { > int code = errno; > argp_failure (state, EXIT_FAILURE, code, > @@ -291,31 +290,18 @@ parse_opt (int key, char *arg, struct argp_state *state) > > if (opt->core) > { > - int fd = open64 (opt->core, O_RDONLY); > - if (fd < 0) > - goto nofile; > - > - Elf *core; > - Dwfl_Error error = __libdw_open_file (&fd, &core, true, false); > - if (error != DWFL_E_NOERROR) > + if (dwfl_core_filename_report (dwfl, NULL, opt->core) == NULL) Use INTUSE for internal usage of public functions inside the library itself. > { > + Dwfl_Error error = dwfl_errno (); Likewise. > argp_failure (state, EXIT_FAILURE, 0, > _("cannot read ELF core file: %s"), > INTUSE(dwfl_errmsg) (error)); > return error == DWFL_E_ERRNO ? errno : EIO; > } > > - int result = INTUSE(dwfl_core_file_report) (dwfl, core); > - if (result < 0) > - { > - elf_end (core); > - close (fd); > - return fail (dwfl, result, opt->core); > - } > - > /* From now we leak FD and CORE. */ > > - if (result == 0) > + if (dwfl->modulelist == NULL) > { > argp_failure (state, EXIT_FAILURE, 0, > _("No modules recognized in core file")); > diff --git a/libdwfl/core-file.c b/libdwfl/core-file.c > index 1545ca8..f217847 100644 > --- a/libdwfl/core-file.c > +++ b/libdwfl/core-file.c > @@ -37,6 +37,7 @@ > #include <endian.h> > #include <byteswap.h> > #include "system.h" > +#include <fcntl.h> > > > /* This is a prototype of what a new libelf interface might be. > @@ -461,3 +462,39 @@ dwfl_core_file_report (Dwfl *dwfl, Elf *elf) > return sniffed == 0 || listed > sniffed ? listed : sniffed; > } > INTDEF (dwfl_core_file_report) > + > +Elf * > +dwfl_core_filename_report (Dwfl *dwfl, int *fdp, const char *filename) > +{ > + int fd_storage = -1; > + if (fdp == NULL) > + fdp = &fd_storage; > + > + if (*fdp < 0) > + { > + *fdp = open64 (filename, O_RDONLY); > + if (*fdp < 0) > + { > + __libdwfl_seterrno (DWFL_E_ERRNO); > + return NULL; > + } > + } > + > + Elf *core; > + Dwfl_Error error = __libdw_open_file (fdp, &core, true, false); > + if (error != DWFL_E_NOERROR) > + { > + __libdwfl_seterrno (error); > + return NULL; > + } > + > + int result = INTUSE(dwfl_core_file_report) (dwfl, core); > + if (result < 0) > + { > + elf_end (core); > + close (*fdp); > + return NULL; > + } > + return core; > +} > +INTDEF (dwfl_core_filename_report) > diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h > index a2ab824..c2b85f4 100644 > --- a/libdwfl/libdwfl.h > +++ b/libdwfl/libdwfl.h > @@ -355,6 +355,14 @@ extern int dwfl_linux_kernel_report_offline (Dwfl *dwfl, > const char *release, > Returns the number of modules reported, or -1 for errors. */ > extern int dwfl_core_file_report (Dwfl *dwfl, Elf *elf); > > +/* Call dwfl_core_file_report but providing FILENAME instead. Function > returns > + opened core file Elf * and its *FDP file descriptor which must be held > valid > + until dwfl_end is called for DWFL. FDP can be passed as NULL (but the > + caller cannot close the file descriptor then). Function returns NULL for > + errors, check dwfl_errmsg in such case. */ > +extern Elf *dwfl_core_filename_report (Dwfl *dwfl, int *fdp, > + const char *filename); I don't really like passing the file descriptor as a pointer and leave the caller with the responsibility to close it afterwards. Can't we find a way to take ownership of the file descriptor and close it ourselves when we are done like with dwfl_report_offline (). That probably means recording it in the Dwfl and releasing in dwfl_end (). Cheers, Mark _______________________________________________ elfutils-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/elfutils-devel
