[libcamera-devel] [PATCH 3/5] libcamera: ipa_manager: Allow recursive parsing

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Wed, Feb 05, 2020 at 01:04:18PM +0000, Kieran Bingham wrote:
> Provide an optional means to recurse into subdirectories to search for IPA

s/means/mean/

> libraries. This allows IPAs contained within their own build directory
> to be resolved when loading from a non-installed build.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/include/ipa_manager.h |  6 ++--
>  src/libcamera/ipa_manager.cpp       | 54 ++++++++++++++++++++++++-----
>  2 files changed, 49 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h
> index 94d35d9062e4..8243ba5a1f51 100644
> --- a/src/libcamera/include/ipa_manager.h
> +++ b/src/libcamera/include/ipa_manager.h
> @@ -32,8 +32,10 @@ private:
>  	IPAManager();
>  	~IPAManager();
>  
> -	int addDir(const char *libDir);
> -	int addPath(const char *path);
> +	int parseDir(const char *libDir, std::vector<std::string> &paths,
> +		     unsigned int subdirs);
> +	int addDir(const char *libDir, unsigned int subdirs = 0);
> +	int addPath(const char *path, unsigned int subdirs = 0);
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 048465c37772..24fe709108fe 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -140,16 +140,17 @@ IPAManager *IPAManager::instance()
>  }
>  
>  /**
> - * \brief Load IPA modules from a directory
> + * \brief Identify shared library objects within a directory

As the function finds shared libraries, maybe s/Identify/Find/ ? I would
also rename the function to findSharedObjects() or something similar.

>   * \param[in] libDir directory to search for IPA modules

While at it could you s/directory/Directory/ ?

Missing documentation for the paths parameters, which I would rename to
files.

> + * \param[in] subdirs maximum number of sub-directories to parse

Same here.

s/subdirs/maxDepth/ ?

Do we need to specify a maximum depth ? The only place where you parse
directories recursively passes true as the subdirs value. It should pass
1 instead, but that seems a bit arbitrary.

>   *
> - * This method tries to create an IPAModule instance for every shared object
> - * found in \a libDir, and skips invalid IPA modules.
> + * Search a directory for .so files. allowing recursion down to

Did you mean s/files\./files,/ ?

> + * subdirectories no further than the quantity specified by \a subdirs

s/quantity/levels/ ?
s/$/./

You should add a sentence to state that the found shared objects are
added to the files vector.

>   *
> - * \return Number of modules loaded by this call, or a negative error code
> - * otherwise
> + * \return 0 on success or a negative error code otherwise
>   */
> -int IPAManager::addDir(const char *libDir)
> +int IPAManager::parseDir(const char *libDir, std::vector<std::string> &paths,
> +			 unsigned int subdirs)
>  {
>  	struct dirent *ent;
>  	DIR *dir;
> @@ -158,8 +159,20 @@ int IPAManager::addDir(const char *libDir)
>  	if (!dir)
>  		return -errno;
>  
> -	std::vector<std::string> paths;
>  	while ((ent = readdir(dir)) != nullptr) {
> +		if (ent->d_type == DT_DIR && subdirs) {
> +			if (strcmp(ent->d_name, ".") == 0 ||
> +			    strcmp(ent->d_name, "..") == 0)
> +				continue;
> +
> +			std::string subdir = std::string(libDir) + "/" + ent->d_name;
> +
> +			/* Only recurse to the limit specified by subdirs */

s/subdirs/subdirs./

> +			parseDir(subdir.c_str(), paths, subdirs - 1);
> +
> +			continue;
> +		}
> +
>  		int offset = strlen(ent->d_name) - 3;
>  		if (offset < 0)
>  			continue;
> @@ -170,6 +183,28 @@ int IPAManager::addDir(const char *libDir)
>  	}
>  	closedir(dir);
>  
> +	return 0;
> +}
> +
> +/**
> + * \brief Load IPA modules from a directory
> + * \param[in] libDir directory to search for IPA modules
> + * \param[in] subdirs maximum number of sub-directories to parse

s/maximum/Maximum/

It's the maximum depth level, not the maximum number of directories.
Same comments as for parseDir().

> + *
> + * This method tries to create an IPAModule instance for every shared object
> + * found in \a libDir, and skips invalid IPA modules.

Here too, shouldn't you add a sentence to explain subdirs ?

> + *
> + * \return Number of modules loaded by this call, or a negative error code
> + * otherwise
> + */
> +int IPAManager::addDir(const char *libDir, unsigned int subdirs)
> +{
> +	std::vector<std::string> paths;
> +
> +	int ret = parseDir(libDir, paths, subdirs);
> +	if (ret < 0)
> +		return ret;
> +
>  	/* Ensure a stable ordering of modules. */
>  	std::sort(paths.begin(), paths.end());
>  
> @@ -193,6 +228,7 @@ int IPAManager::addDir(const char *libDir)
>  /**
>   * \brief Load IPA modules from a colon separated PATH variable
>   * \param[in] path string to split to search for IPA modules
> + * \param[in] subdirs maximum number of sub-directories to parse

Ditto.

>   *
>   * This method tries to create an IPAModule instance for every shared object
>   * found in the directories described by \a paths.

Same.

> @@ -200,7 +236,7 @@ int IPAManager::addDir(const char *libDir)
>   * \return Number of modules loaded by this call, or a negative error code
>   * otherwise
>   */
> -int IPAManager::addPath(const char *paths)
> +int IPAManager::addPath(const char *paths, unsigned int subdirs)
>  {
>  	int ipaCount = 0;
>  
> @@ -210,7 +246,7 @@ int IPAManager::addPath(const char *paths)
>  
>  		if (count) {
>  			std::string path(paths, count);
> -			int ret = addDir(path.c_str());
> +			int ret = addDir(path.c_str(), subdirs);
>  			if (ret > 0)
>  				ipaCount += ret;
>  		}

-- 
Regards,

Laurent Pinchart