This is the mail archive of the
elfutils-devel@sourceware.org
mailing list for the elfutils project.
Re: [patch 3/4] unwinder: Provide dwfl_core_filename_report
- From: Mark Wielaard <mjw at redhat dot com>
- To: elfutils-devel at lists dot fedorahosted dot org
- Date: Wed, 29 May 2013 11:07:02 +0200
- Subject: Re: [patch 3/4] unwinder: Provide dwfl_core_filename_report
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 <jan.kratochvil@redhat.com>
>
> * libdw.map (ELFUTILS_0.156): New block.
>
> libdwfl/
> 2012-11-13 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * 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