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

[Laurent Pinchart]

Hi Kieran,

Another comment.

On Thu, Feb 06, 2020 at 08:07:38PM +0200, 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.
> 
> s/a const/A const/
> 
> Maybe s/elf/ELF/ ?
> 
> > + * or NULL if it is not found.
> 
> 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
> 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.
> 
> 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.
> 
> > +{
> > +	const ElfW(Dyn) *dyn = _DYNAMIC;

bionic and musl don't seem to provide _DYNAMIC :-S They both have elf.h
and link.h, with a definition of the macros and structures used below,
but _DYNAMIC seems to be missing. I may be missing something, it should
be tested.

> > +	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) {
> 
> ?
> 
> 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