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

[Laurent Pinchart]

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.

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;
> +	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