[libcamera-devel] [PATCH LIBCAMERA v3] libcamera: utils: adapt libcameraPath to match use cases

[Kieran Bingham]

Hi Laurent, Kaaira,

On 19/03/2020 13:35, Laurent Pinchart wrote:
> Hi Kaaira,
> 
> Thank you for the patch.
> 
> On Thu, Mar 19, 2020 at 06:12:52PM +0530, 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 libcameraBuildPath() which
>> combines their functions and returns the root of the build sources
>> when the library has not been installed, but is running,thereby making
> 
> s/running,/running, /
> 
> We don't charge extra for white space :-)

Ooops I missed that one.

> 
>> call sites simpler.
>>
>> When the library is imstalled, libcameraBuildPath() will return an empty
> 
> s/imstalled/installed/
> 
>> string.
>>
>> Make changes in the call sites accordingly.
>>
>> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
>> ---
>> Changes since v2:
>> 	- rename subject line and add necessary details in commit
>> 	  message.
>> 	- Solve whitespace issues.
>> 	- specify the final 'slash' for directory path in return
>> 	  statement itself.
>>
>> Changes since v1:
>>         - rename function to libcameraBuildPath() instead, and return
>>           the root of libcamera source, and not the source directory
>>           root.
>>         - Return empty string instead of nullptr when ret==0
>>
>>  src/libcamera/include/utils.h |  2 +-
>>  src/libcamera/ipa_manager.cpp |  5 +++--
>>  src/libcamera/ipa_proxy.cpp   |  6 +++---
>>  src/libcamera/utils.cpp       | 16 +++++++++-------
>>  4 files changed, 16 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
>> index bc96e79..5dea8d2 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();
> 
> This function isn't used externally anymore, I would drop it from the
> header.

but it /could/ be used. It's not a static, and other locations might
want to be able to detect this in the future.

That was sort of the reason of keeping the two functions separate rather
than merging them.

Still it could easily be added back in if needed in the future too ...

>>  
>> -std::string libcameraPath();
>> +std::string libcameraBuildPath();
>>  
>>  } /* namespace utils */
>>  
>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>> index 0bd280c..bcaae35 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 root = utils::libcameraBuildPath();
>> +	if (!root.empty()) {
>> +		std::string ipaBuildPath = root + "src/ipa";
> 
> Very neat :-)
> 
>>  		constexpr int maxDepth = 1;
>>  
>>  		LOG(IPAManager, Info)
>> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
>> index 2f866cc..5fd88a4 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 root = utils::libcameraBuildPath();
>> +	if (!root.empty()) {
>> +		std::string ipaProxyDir = root + "src/libcamera/proxy/worker";
>>  
>>  		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..42c5c76 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 libcameraBuildPath()
>>  {
>> -	Dl_info info;
>> +	if (!isLibcameraInstalled()) {
>> +		Dl_info info;
>>  
>> -	/* Look up our own symbol. */
>> -	int ret = dladdr(reinterpret_cast<void *>(libcameraPath), &info);
>> -	if (ret == 0)
>> -		return nullptr;
>> +		/* Look up our own symbol. */
>> +		int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info);
>> +		if (ret)
>> +			return dirname(info.dli_fname) + "/../../";
>> +	}
>>  
>> -	return info.dli_fname;
>> +	return std::string();
> 
> Maybe it's my personal preference, but I find code easier to read when
> dealing away with errors immediately:
> 
> 	if (isLibcameraInstalled())
> 		return std::string();
> 
> 	Dl_info info;
> 
> 	/* Look up our own symbol. */
> 	int ret = dladdr(reinterpret_cast<void *>(libcameraBuildPath), &info);
> 	if (!ret)
> 		return std::string();
> 
> 	return dirname(info.dli_fname) + "/../../";
> 
> This way you can reduce indentation for most of the code that implements
> the real logic.
> 
> If you're fine with this, I'm sure Kieran can change it while applying.

Hrm, I usually look to reduce the number of return paths ... but I'm not
fussed either way. It's only because I had to work on MISRA projects in
the past.

Kaaira - up to you really I think. What do you prefer? If you like
Laurent's version - please send a v4 with the other fixes and I can test
and apply it.

--
Kieran


> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
>>  }
>>  
>>  } /* namespace utils */
>