From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30748 invoked by alias); 29 Jan 2019 15:11:43 -0000 Mailing-List: contact elfutils-devel-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Post: List-Help: List-Subscribe: Sender: elfutils-devel-owner@sourceware.org Received: (qmail 30738 invoked by uid 89); 29 Jan 2019 15:11:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.2 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.2 spammy=searching X-Spam-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_PASS autolearn=ham version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on sourceware.org X-Spam-Level: X-HELO: gnu.wildebeest.org Received: from wildebeest.demon.nl (HELO gnu.wildebeest.org) (212.238.236.112) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 29 Jan 2019 15:11:41 +0000 Received: from librem.wildebeest.org (deer0x15.wildebeest.org [172.31.17.151]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 5D88C302BBFE; Tue, 29 Jan 2019 16:11:39 +0100 (CET) Received: by librem.wildebeest.org (Postfix, from userid 1000) id EB98113FB04; Tue, 29 Jan 2019 16:11:38 +0100 (CET) Date: Tue, 29 Jan 2019 15:11:00 -0000 From: Mark Wielaard To: Luke Diamand Cc: "elfutils-devel@sourceware.org" , "Dmitry V. Levin" Subject: Re: [PATCHv2 1/2] libdwfl: specify optional sysroot to search for shared libraries Message-ID: <20190129151138.GG9378@wildebeest.org> References: <20190122101610.5301-1-ldiamand@roku.com> <20190122101610.5301-2-ldiamand@roku.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190122101610.5301-2-ldiamand@roku.com> User-Agent: Mutt/1.10.1 (2018-07-13) X-Spam-Flag: NO X-IsSubscribed: yes X-SW-Source: 2019-q1/txt/msg00097.txt.bz2 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 > --- > 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 > #include > #include > +#include > > /* 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