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

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Thu, Feb 20, 2020 at 04:57:01PM +0000, Kieran Bingham wrote:
> Provide an optional means to recurse into subdirectories to search for IPA
> libraries. This allows IPAs contained within their own build directory
> to be resolved when loading from a non-installed build.

s/resolved/found/ ?

> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/include/ipa_manager.h |  6 ++-
>  src/libcamera/ipa_manager.cpp       | 69 ++++++++++++++++++++++-------
>  2 files changed, 58 insertions(+), 17 deletions(-)
> 
> diff --git a/src/libcamera/include/ipa_manager.h b/src/libcamera/include/ipa_manager.h
> index 94d35d9062e4..5dca2c2a7d8d 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> &files,
> +		     unsigned int maxDepth);
> +	int addDir(const char *libDir, unsigned int maxDepth = 0);
> +	int addPath(const char *path, unsigned int maxDepth = 0);
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index d87f2221b00b..c30b4555290f 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -140,16 +140,20 @@ IPAManager *IPAManager::instance()
>  }
>  
>  /**
> - * \brief Load IPA modules from a directory
> - * \param[in] libDir directory to search for IPA modules
> + * \brief Identify shared library objects within a directory
> + * \param[in] libDir The directory to search for IPA modules

s/IPA modules/shared objects/ ?

> + * \param[in] files A vector of paths to shared object library files

this is an out argument. As such I would move it as the last argument of
the function to have input arguments first, followed by output
arguments.

> + * \param[in] maxDepth The maximum depth of sub-directories to parse
>   *
> - * 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
> + * subdirectories no further than the depth specified by \a maxDepth.

s/subdirectories/sub-directories/ (for consistency, I don't care much
which one we pick). And this can go on the previous line I think.

>   *
> - * \return Number of modules loaded by this call, or a negative error code
> - * otherwise
> + * Discovered shared objects are added to the files vector.

"\a files" ?

> + *
> + * \return 0 on success or a negative error code otherwise

"or a negative error code if \a libdir can't be opened" ?

>   */
> -int IPAManager::addDir(const char *libDir)
> +int IPAManager::parseDir(const char *libDir, std::vector<std::string> &files,
> +			 unsigned int maxDepth)
>  {
>  	struct dirent *ent;
>  	DIR *dir;
> @@ -158,30 +162,64 @@ 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 && maxDepth) {
> +			if (strcmp(ent->d_name, ".") == 0 ||
> +			    strcmp(ent->d_name, "..") == 0)
> +				continue;
> +
> +			std::string subdir = std::string(libDir) + "/" + ent->d_name;
> +
> +			/* Recursion is limited to maxDepth. */
> +			parseDir(subdir.c_str(), files, maxDepth - 1);
> +
> +			continue;
> +		}
> +
>  		int offset = strlen(ent->d_name) - 3;
>  		if (offset < 0)
>  			continue;
>  		if (strcmp(&ent->d_name[offset], ".so"))
>  			continue;
>  
> -		paths.push_back(std::string(libDir) + "/" + ent->d_name);
> +		files.push_back(std::string(libDir) + "/" + ent->d_name);
>  	}
>  	closedir(dir);
>  
> +	return 0;
> +}
> +
> +/**
> + * \brief Load IPA modules from a directory
> + * \param[in] libDir The directory to search for IPA modules
> + * \param[in] maxDepth The maximum depth of sub-directories to parse

s/parse/search/ ?

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

Maybe add in the same paragraph

Sub-directories are searched up to a depth of \a maxDepth. A \a maxdepth
value of 0 only searches the directory specified in \a libdir.

> + *
> + * \return Number of modules loaded by this call, or a negative error code
> + * otherwise

"otherwise" compared to what ? :-) I think we can drop the error code
and turn the function return type into unsigned int, as no caller of
addDir() cares about errors. It will actually simplify the callers that
won't have to check for errors anymore, for instance

 	/* Finally try to load IPAs from the installed system path. */
-	if (ret > 0)
-		ipaCount += ret;
+	ipaCount += addDir(IPA_MODULE_DIR);

> + */
> +int IPAManager::addDir(const char *libDir, unsigned int maxDepth)
> +{
> +	std::vector<std::string> files;
> +
> +	int ret = parseDir(libDir, files, maxDepth);
> +	if (ret < 0)
> +		return ret;
> +
>  	/* Ensure a stable ordering of modules. */
> -	std::sort(paths.begin(), paths.end());
> +	std::sort(files.begin(), files.end());
>  
>  	unsigned int count = 0;
> -	for (const std::string &path : paths) {
> -		IPAModule *ipaModule = new IPAModule(path);
> +	for (const std::string &file : files) {
> +		IPAModule *ipaModule = new IPAModule(file);
>  		if (!ipaModule->isValid()) {
>  			delete ipaModule;
>  			continue;
>  		}
>  
> -		LOG(IPAManager, Debug) << "Loaded IPA module '" << path << "'";
> +		LOG(IPAManager, Debug) << "Loaded IPA module '" << file << "'";
>  
>  		modules_.push_back(ipaModule);
>  		count++;
> @@ -193,6 +231,7 @@ int IPAManager::addDir(const char *libDir)
>  /**
>   * \brief Load IPA modules from a search path
>   * \param[in] path The colon-separated list of directories to load IPA modules from
> + * \param[in] maxDepth The maximum number of sub-directories to parse

Still not the maximum number of directories, but the depth :-)

 * \param[in] maxDepth The maximum depth of sub-directories to search

like above should be fine.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>   *
>   * This method tries to create an IPAModule instance for every shared object
>   * found in the directories listed in \a path.
> @@ -200,7 +239,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 *path)
> +int IPAManager::addPath(const char *path, unsigned int maxDepth)
>  {
>  	int ipaCount = 0;
>  
> @@ -208,7 +247,7 @@ int IPAManager::addPath(const char *path)
>  		if (dir.empty())
>  			continue;
>  
> -		int ret = addDir(dir.c_str());
> +		int ret = addDir(dir.c_str(), maxDepth);
>  		if (ret > 0)
>  			ipaCount += ret;
>  	}

-- 
Regards,

Laurent Pinchart