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

[Kieran Bingham]

Hi Laurent,

On 06/02/2020 18:07, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> 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.

> 
> 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/ ...

> ?
> 
> 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
--
Kieran