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(a)redhat.com>
* libdw.map (ELFUTILS_0.156): New block.
libdwfl/
2012-11-13 Jan Kratochvil <jan.kratochvil(a)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