Re: [patch 3/4] unwinder: Provide dwfl_core_filename_report

[Mark Wielaard <mjw at redhat dot com>]

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