[libcamera-devel] [PATCH LIBCAMERA v3 3/6] libcamera: utils: add Libcamera installed & path

[Kieran Bingham]

Hi Kaaira,

Looks like Laurent beat me to the review while I was testing, so I'll
let you work through his comments.

Small additional comment here that the $SUBJECT also has a 'Libcamera'
reference which should be 'libcamera' to match the style used elsewhere.

and going below for the question to me...

On 18/03/2020 12:25, Laurent Pinchart wrote:
> Hi Kaaira,
> 
> Thank you for the patch.
> 
> On Wed, Mar 18, 2020 at 05:28:43PM +0530, Kaaira Gupta wrote:
>> Add a global functions 'isLibcameraInstalled' to check if libcamera is
>> installed or not, and another global function 'libcameraPath' to return
>> the path of libcamera.so in utils.
>>
>> Signed-off-by: Kaaira Gupta <kgupta at es.iitr.ac.in>
>> ---
>>  src/libcamera/include/utils.h |  4 +++
>>  src/libcamera/utils.cpp       | 48 +++++++++++++++++++++++++++++++++++
>>  2 files changed, 52 insertions(+)
>>
>> diff --git a/src/libcamera/include/utils.h b/src/libcamera/include/utils.h
>> index 9405977..bc96e79 100644
>> --- a/src/libcamera/include/utils.h
>> +++ b/src/libcamera/include/utils.h
>> @@ -143,6 +143,10 @@ private:
>>  
>>  details::StringSplitter split(const std::string &str, const std::string &delim);
>>  
>> +bool isLibcameraInstalled();
>> +
>> +std::string libcameraPath();
>> +
>>  } /* namespace utils */
>>  
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
>> index f566e88..aeaf163 100644
>> --- a/src/libcamera/utils.cpp
>> +++ b/src/libcamera/utils.cpp
>> @@ -12,12 +12,18 @@
>>  #include <stdlib.h>
>>  #include <string.h>
>>  #include <unistd.h>
>> +#include <dlfcn.h>
>> +#include <elf.h>
>> +#include <link.h>
> 
> Could you please keep headers alphabetically sorted ? The header order
> is explained in section "Order of Includes" of
> Documentation/coding-style.rst.
> 
>>  
>>  /**
>>   * \file utils.h
>>   * \brief Miscellaneous utility functions
>>   */
>>  
>> +/* musl doesn't declare _DYNAMIC in link.h, declare it manually. */
>> +extern ElfW(Dyn) _DYNAMIC[];
>> +
>>  namespace libcamera {
>>  
>>  namespace utils {
>> @@ -310,6 +316,48 @@ details::StringSplitter split(const std::string &str, const std::string &delim)
>>  	return details::StringSplitter(str, delim);
>>  }
>>  
>> +/**
>> + * \brief Checks if Libcamera is installed or not
> 
> s/Checks/Check/
> 
> (We use the imperative mood for \brief statements through the code)
> 
> and
> 
> s/Libcamera/libcamera/
> 
> "libcamera" is spelled in lower case, with the exception of symbols in
> the code where the camelCase rule forces usage of "Libcamera".
> 
>> + *
>> + * Utilises the build_rpath dynamic tag which is stripped out by meson at
> 
> s/Utilises/Utilise/ or "This function utilises ...".
> 
>> + * install time to determine at runtime if the library currently executing
>> + * has been installed or not.
>> + *
>> + * \return A bool stating if libcamera is installed or not
> 
> The usual documentation style would be
> 
>  * \return True if libcamera is installed, false otherwise
> 
>> + */
>> +bool isLibcameraInstalled()
>> +{
>> +	/*
>> +	 * DT_RUNPATH (DT_RPATH when the linker uses old dtags) is removed on
>> +	 * install.
>> +	 */
>> +	for (const ElfW(Dyn) *dyn = _DYNAMIC; dyn->d_tag != DT_NULL; ++dyn) {
>> +		if (dyn->d_tag == DT_RUNPATH || dyn->d_tag == DT_RPATH)
>> +			return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +/**
>> + * \brief Identifies libcamera path
> 
> Maybe "Identify the libcamera.so path" ?
> 
>> + *
>> + * Identifies the location by finding the path of the active libcamera.so itself.
> 
> Please wrap lines at the 80 columns limit when possible. We have a 120
> columns hard limit and a 80 columns soft limit.
> 
> "Path" is a bit ambiguous I think, as it's not clear if it includes
> "libcamera.so" at the end or not. How about clarifying this with
> 
>  * This function locates the running libcamera.so and returns its full
>  * path, including the file name.
> 
>> + *
>> + * \return A string stating the path.
> 
> To match the documentation style of the rest of libcamera, no period is
> needed at tne end.
> 
>> + */
>> +std::string libcameraPath()
>> +{
>> +	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;
>> +}
> 
> I wonder if we should merge the two functions into a
> libcameraBuildPath() that would return an empty string if libcamera is
> installed and the path (with the libcamera.so) otherwise, as that's what
> all callers end up doing. This can be done on top of this series if
> desired. Kieran, any opinion ?

It might indeed make sense, but I'm happy to do it on top, so we don't
over complicate this series.

--
Kieran


>> +
>>  } /* namespace utils */
>>  
>>  } /* namespace libcamera */
> 

-- 
Regards
--
Kieran