[libcamera-devel] [PATCH LIBCAMERA] libcamera: utils: merge isLibcamerainstalled with libcameraPath

[Kieran Bingham]

Hi Kaaira,

On 19/03/2020 09:54, Kaaira Gupta wrote:
> The two callers of functions libcameraPath() and isLibcameraInstalled()
> end up using the same process and finally use the path with
> libcamera.so. Hence write a function libcameraSourcePath() which
> combines their functions, thereby making call sites simpler.
> 
> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
> ---
>  src/libcamera/include/utils.h |  2 +-
>  src/libcamera/ipa_manager.cpp |  5 +++--
>  src/libcamera/ipa_proxy.cpp   |  6 +++---
>  src/libcamera/utils.cpp       | 20 +++++++++++---------
>  4 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
> index bc96e79..2cff15e 100644
> --- a/src/libcamera/include/utils.h
> +++ b/src/libcamera/include/utils.h
> @@ -145,7 +145,7 @@ details::StringSplitter split(const std::string &str, const std::string &delim);
>  
>  bool isLibcameraInstalled();
>  
> -std::string libcameraPath();
> +std::string libcameraSourcePath();
>  
>  } /* namespace utils */
>  
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 0bd280c..cf9b6e7 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -119,8 +119,9 @@ IPAManager::IPAManager()
>  	 * path for the IPA from that point. We need to recurse one level of
>  	 * sub-directories to match the build tree.
>  	 */
> -	if (!utils::isLibcameraInstalled()) {
> -		std::string ipaBuildPath = utils::dirname(utils::libcameraPath()) + "/../ipa";
> +	std::string libPath = utils::libcameraSourcePath();
> +	if (!libPath.empty()) {
> +		std::string ipaBuildPath = libPath + "/../ipa";


I wonder if we should make utils::libcameraSourcePath return the root of
the libcamera sources so that any users take a relative path from the root?

That might make things clearer or more consistent as then the
ipaBuildPath would become:

  std::string ipaBuildPath = root + "/src/ipa";

What do you think?

if we do change to return the 'root', it would be a 'Build' directory
root not a 'Source', so I think we might have to reconsider the name of
the function too.

>  		constexpr int maxDepth = 1;
>  
>  		LOG(IPAManager, Info)
> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
> index 2f866cc..e9b7568 100644
> --- a/src/libcamera/ipa_proxy.cpp
> +++ b/src/libcamera/ipa_proxy.cpp
> @@ -97,9 +97,9 @@ std::string IPAProxy::resolvePath(const std::string &file) const
>  	 * This requires identifying the path of the libcamera.so, and
>  	 * referencing a relative path for the proxy workers from that point.
>  	 */
> -	if (!utils::isLibcameraInstalled()) {
> -		std::string ipaProxyDir = utils::dirname(utils::libcameraPath())
> -					  + "/proxy/worker";
> +	std::string libPath = utils::libcameraSourcePath();
> +	if (!libPath.empty()) {
> +		std::string ipaProxyDir = libPath + "/proxy/worker";

Of course, this would then become a bit longer:

std::string ipaProxyDir = libPath + "src/libcamera/proxy/worker";

But I think it might make the intent clearer for both locations?


>  
>  		LOG(IPAProxy, Info)
>  			<< "libcamera is not installed. Loading proxy workers from'"
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> index 7e118fa..8212d3e 100644
> --- a/src/libcamera/utils.cpp
> +++ b/src/libcamera/utils.cpp
> @@ -347,16 +347,18 @@ bool isLibcameraInstalled()
>   *
>   * \return A string stating the path
>   */
> -std::string libcameraPath()
> +std::string libcameraSourcePath()
>  {
> -	Dl_info info;
> -
> -	/* Look up our own symbol. */
> -	int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info);
> -	if (ret == 0)
> -		return nullptr;
> -
> -	return info.dli_fname;
> +	if (!utils::isLibcameraInstalled()) {

Because we are 'in' the utils:: namespace here, I don't think you need
to explicitly state 'utils::' to call isLibcameraInstalled.

The compiler will find the function without it.

> +		Dl_info info;
> +		/* Look up our own symbol. */
> +		int ret = dladdr(reinterpret_cast<void *>(libcameraSourcePath), &info);
> +		if (ret == 0)
> +			return nullptr;
> +
> +		return utils::dirname(info.dli_fname);

If you drop the 'else' as stated below, you can invert this if, to

 if (ret)
	return utils::dirname(info.dli_fname);

and let the error case fall through to the return std::string(); below
which is better than returning a nullptr anyway, as the callers check
this returned string for .empty() (so returning a nullptr is bad).

> +	} else
> +		return std::string();

You can drop the 'else', and put the return std::string(); as the last
statement of the function.


>  }
>  
>  } /* namespace utils */
> 

-- 
Regards
--
Kieran