[libcamera-devel] [PATCH 1/5] libcamera: utils: Provide helper to get dynamic library runpath

[Laurent Pinchart]

Hi Kieran,

On Tue, Feb 11, 2020 at 02:05:53PM +0000, Kieran Bingham wrote:
> On 06/02/2020 18:07, Laurent Pinchart wrote:
> > On Wed, Feb 05, 2020 at 01:04:16PM +0000, Kieran Bingham wrote:
> >> Provide a helper that will identify the DT_RUNPATH string from the
> >> ELF dynamic library string table entries.
> >>
> >> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >> ---
> >>  src/libcamera/include/utils.h |  1 +
> >>  src/libcamera/utils.cpp       | 26 ++++++++++++++++++++++++++
> >>  2 files changed, 27 insertions(+)
> >>
> >> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> >> index e467eb21c518..069b327bb436 100644
> >> --- a/src/libcamera/include/utils.h
> >> +++ b/src/libcamera/include/utils.h
> >> @@ -33,6 +33,7 @@ namespace utils {
> >>  const char *basename(const char *path);
> >>  
> >>  char *secure_getenv(const char *name);
> >> +const char *get_runpath();
> >>  
> >>  template<class InputIt1, class InputIt2>
> >>  unsigned int set_overlap(InputIt1 first1, InputIt1 last1,
> >> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> >> index 4beffdab5eb6..e48cf2b6a48e 100644
> >> --- a/src/libcamera/utils.cpp
> >> +++ b/src/libcamera/utils.cpp
> >> @@ -7,7 +7,9 @@
> >>  
> >>  #include "utils.h"
> >>  
> >> +#include <elf.h>
> >>  #include <iomanip>
> >> +#include <link.h>
> >>  #include <sstream>
> >>  #include <stdlib.h>
> >>  #include <string.h>
> >> @@ -70,6 +72,30 @@ char *secure_getenv(const char *name)
> >>  #endif
> >>  }
> >>  
> >> +/**
> >> + * \brief Obtain the runpath string from the dynamic symbol table
> > 
> > It's the "dynamic tags" (.dynamic), not "dynamic symbol table"
> > (.dynsym).
> > 
> >> + * \returns a const char pointer to the runpath entry in the elf string table
> > 
> > s/returns/return/
> > 
> > Actually after checking the doxygen documentation \returns is an alias
> > for \return. Still, I think we should standardize on \return.
> 
> Looks like I copied \returns from the secure_getenv above.
> 
> I'll send a patch to fix that, and the other one in the source code.
> 
>  <sent>
> 
> > s/a const/A const/
> > 
> > Maybe s/elf/ELF/ ?
> > 
> >> + * or NULL if it is not found.
> 
> s/NULL/nullptr here of course too
> 
> Perhaps NULL => nullptr should be in checkstyle.py.

That's a good idea. It should be easy to add a regex that will match on
NULL, but ideally we should have a better parser that would handle the
context. "NULL" may be acceptable, as well as NULL in a comment for
instance.

Does clang-format support python plugins ? :-)

> > s/\.$//
> > 
> >> + */
> >> +const char *get_runpath()
> > 
> > We only use this function in a single location, I'm tempted to move it
> > to ipa_manager.cpp. If we want to create ELF-related helpers, then
> 
> Indeed, as it's only a single use and not expected to be used elsewhere
> it could be a local static.
> 
> At that point, the implementation can be squashed into the patch that
> needs it too.
> 
> > moving this, along with the ELF code from ipa_module.cpp to an elf.cpp
> > file could be useful, but would be overkill for now.
> 
> If we end up with any more ELF support, I'd like to see it all collected
> together indeed but for now it's fine.
> 
> > How about calling it elf_get_runpath(), elf_runpath(), or elfRunPath() ?
> > The latter is more compliant with our coding style. The reason why most
> > other functions in this file use snake case instead of camel case is
> > because they mimic the glibc or libstdc++ APIs.
> 
> I was mimicking the style of this file.
> 
> I'll move to ipa_manager and use elfRunPath
> 
> >> +{
> >> +	const ElfW(Dyn) *dyn = _DYNAMIC;
> >> +	const ElfW(Dyn) *runpath = NULL;
> > 
> > %s/NULL/nullptr/
> > 
> >> +	const char *strtab = NULL;
> >> +
> >> +	for (; dyn->d_tag != DT_NULL; ++dyn) {
> > 
> > Maybe
> > 
> > 	for (const ELFW(Dyn) *dyn = _DYNAMIC; dyn->d_tag != DT_NULL; ++dyn) {
> 
> Sure but s/ELFW/ElfW/ ...

Absolutely :-)

> > ?
> > 
> > Do I understand correctly that _DYNAMIC is the dynamic tags array for
> > the current ELF binary, and thus always corresponds to libcamera.so, not
> > the code calling get_runpath() ? That's what we need, but I think it
> > should be documented.
> > 
> >> +		if (dyn->d_tag == DT_STRTAB)
> >> +			strtab = reinterpret_cast<const char *>(dyn->d_un.d_val);
> >> +		else if (dyn->d_tag == DT_RUNPATH)
> >> +			runpath = dyn;
> >> +	}
> >> +
> >> +	if (strtab && runpath)
> >> +		return strtab + runpath->d_un.d_val;
> >> +
> >> +	return NULL;
> > 
> > nullptr here too.
> > 
> >> +}
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > 
> >> +
> >>  /**
> >>   * \fn libcamera::utils::set_overlap(InputIt1 first1, InputIt1 last1,
> >>   *				     InputIt2 first2, InputIt2 last2)

-- 
Regards,

Laurent Pinchart