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

3165 diff mbox series

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@es.iitr.ac.in>
---
 src/libcamera/include/utils.h |  4 +++
 src/libcamera/utils.cpp       | 48 +++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

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

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

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@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 */
>