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

[Nicolas Dufresne]

Le mardi 11 février 2020 à 17:58 +0200, Laurent Pinchart a écrit :
> 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 ? :-)

That is good idea since it will be very common from C and specially
GStreamer contributors. Basically any code that is copied from
somewhere else will have NULL in it.

> 
> > > 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)