Re: [PATCHv2 1/2] libdwfl: specify optional sysroot to search for shared libraries

[Mark Wielaard <mark at klomp dot org>]

On Tue, Jan 22, 2019 at 10:16:25AM +0000, Luke Diamand wrote:
> When searching the list of modules in a core file, if the core was
> generated on a different system to the current one, we need to look
> in a sysroot for the various shared objects.
> 
> For example, we might be looking at a core file from an ARM system
> using elfutils running on an x86 host.
> 
> This change adds a new function, dwfl_set_sysroot(), which then
> gets used when searching for libraries.

A few comments below.
Also a ChangeLog entry is missing. I added comments for them.

> Signed-off-by: Luke Diamand <ldiamand@roku.com>
> ---
>  libdw/libdw.map    |  7 ++++++-
>  libdwfl/dwfl_end.c |  1 +
>  libdwfl/libdwfl.h  |  5 +++++
>  libdwfl/libdwflP.h |  1 +
>  libdwfl/link_map.c | 26 ++++++++++++++++++++++++--
>  5 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/libdw/libdw.map b/libdw/libdw.map
> index 55482d58..43a9de2e 100644
> --- a/libdw/libdw.map
> +++ b/libdw/libdw.map
> @@ -360,4 +360,9 @@ ELFUTILS_0.173 {
>  ELFUTILS_0.175 {
>    global:
>      dwelf_elf_begin;
> -} ELFUTILS_0.173;
> \ No newline at end of file
> +} ELFUTILS_0.173;
> +
> +ELFUTILS_0.176 {
> +  global:
> +    dwfl_set_sysroot;
> +} ELFUTILS_0.175;

OK.

       * libdw.map (ELFUTILS_0.176): New section, add dwfl_set_rootsysroot.

> diff --git a/libdwfl/dwfl_end.c b/libdwfl/dwfl_end.c
> index 74ee9e07..345d9947 100644
> --- a/libdwfl/dwfl_end.c
> +++ b/libdwfl/dwfl_end.c
> @@ -45,6 +45,7 @@ dwfl_end (Dwfl *dwfl)
>    free (dwfl->lookup_addr);
>    free (dwfl->lookup_module);
>    free (dwfl->lookup_segndx);
> +  free (dwfl->sysroot);
>  
>    Dwfl_Module *next = dwfl->modulelist;
>    while (next != NULL)

OK.

	* libdwfl_end.c (dwfl_end): Free dwfl->sysroot.

> diff --git a/libdwfl/libdwfl.h b/libdwfl/libdwfl.h
> index a0c1d357..c11e2f24 100644
> --- a/libdwfl/libdwfl.h
> +++ b/libdwfl/libdwfl.h
> @@ -807,6 +807,11 @@ int dwfl_getthread_frames (Dwfl *dwfl, pid_t tid,
>  bool dwfl_frame_pc (Dwfl_Frame *state, Dwarf_Addr *pc, bool *isactivation)
>    __nonnull_attribute__ (1, 2);
>  
> +/* Set the sysroot to use when searching for shared libraries. If not
> + specified, search in the system root.  */
> +void dwfl_set_sysroot (Dwfl *dwfl, const char *sysroot)
> +  __nonnull_attribute__ (1);

This should document that a copy is made of sysroot which
is owned by the library, the passed in string is owned and
can be freed after the call.

It probably should return an int, so failure can be indicated
(you call realpath, which can fail).
What happens if sysroot was already set?
Does setting it to NULL clear it?

	* libdwfl.h (dwfl_set_sysroot): New function declaration.

> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h
> index 941a8b66..db16ab57 100644
> --- a/libdwfl/libdwflP.h
> +++ b/libdwfl/libdwflP.h
> @@ -138,6 +138,7 @@ struct Dwfl
>    int lookup_tail_ndx;
>  
>    struct Dwfl_User_Core *user_core;
> +  char *sysroot;	/* sysroot, or NULL to search standard system paths */
>  };

OK.

	* libdwflP.h (struct Dwfl): Add sysroot field.
>  
>  #define OFFLINE_REDZONE		0x10000
> diff --git a/libdwfl/link_map.c b/libdwfl/link_map.c
> index 29307c74..cf18c0a2 100644
> --- a/libdwfl/link_map.c
> +++ b/libdwfl/link_map.c
> @@ -34,6 +34,7 @@
>  #include <byteswap.h>
>  #include <endian.h>
>  #include <fcntl.h>
> +#include <stdlib.h>
>  
>  /* This element is always provided and always has a constant value.
>     This makes it an easy thing to scan for to discern the format.  */
> @@ -388,8 +389,21 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
>        if (name != NULL)
>  	{
>  	  /* This code is mostly inlined dwfl_report_elf.  */
> -	  // XXX hook for sysroot
> -	  int fd = open (name, O_RDONLY);
> +	  char *path_name;
> +	  int rc;
> +	  const char *sysroot = dwfl->sysroot;
> +
> +	  /* don't look in the sysroot if the path is already inside the sysroot */
> +	  bool name_in_sysroot = sysroot && (strncmp(name, sysroot, strlen(sysroot)) == 0);
> +
> +	  if (!name_in_sysroot && sysroot)

The && sysroot is now redundant.

> +	    rc = asprintf(&path_name, "%s/%s", sysroot, name);
> +	  else
> +	    rc = asprintf(&path_name, "%s", name);
> +	  if (unlikely(rc == -1))
> +	    return release_buffer(-1);

Minor optimization.
In theory you could just assign name to path_name here and then...

> +
> +	  int fd = open (path_name, O_RDONLY);
>  	  if (fd >= 0)
>  	    {
>  	      Elf *elf;
> @@ -471,6 +485,7 @@ report_r_debug (uint_fast8_t elfclass, uint_fast8_t elfdata,
>  		    close (fd);
>  		}
>  	    }
> +          free(path_name);

Guard this free with if (name_in_sysroot).
No worries if you think that is ugly, it does make for
easier error handling and resource cleanup.

Please use tab plus 2 spaces to indent.

	* link_map.c (report_r_debug): Check and use Dwfl sysroot.

>  	}
>  
>        if (mod != NULL)
> @@ -1037,3 +1052,10 @@ dwfl_link_map_report (Dwfl *dwfl, const void *auxv, size_t auxv_size,
>  			 &integrated_memory_callback, &mcb, r_debug_info);
>  }
>  INTDEF (dwfl_link_map_report)
> +
> +void
> +dwfl_set_sysroot (Dwfl *dwfl, const char *sysroot)
> +{
> +  dwfl->sysroot = realpath(sysroot, NULL);
> +}
> +INTDEF (dwfl_set_sysroot)

I like it better if this goes into its own file (in theory every public
function has its own implementation file). Also see the comments above
for the function declaration. You probably want to handle sysroot
already been set (error or replace?) and sysroot being NULL (error or
clear sysroot setting?).

Cheers,

Mark