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

[Kieran Bingham]

Hi Laurent,

On 06/02/2020 19:36, Laurent Pinchart wrote:
> 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/

Uhhh ? I don't think so...

Means to an end...
not
Mean to an end.


> 
>> 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.

Hrm ... Identifying and finding are quite similar, perhaps it's just a
distinction of classifying the file, and iterating to be able to
classify it...

> 
>>   * \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.

files or libraries ?


I.e. should we have:

	for (const std::string &file : files) {
		IPAModule *ipaModule = new IPAModule(file);

or

	for (const std::string &library : libraries) {
		IPAModule *ipaModule = new IPAModule(library);


> 
>> + * \param[in] subdirs maximum number of sub-directories to parse
> 
> Same here.
> 
> s/subdirs/maxDepth/ ?

I certainly prefer maxDepth. 'subdirs' is a left over from when it was a
bool.

> 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.

Aha - then it should pass 1 instead :-)

I started with it as a bool, but realised that effectively it's the
depth, and if we want to go deeper, then we can set it to 2... or 3...

That could happen if an IPA creates subdirectories for some structure or
such, or we need to start higher up to search.

And I wanted to ensure there was a limit to prevent it going too far if
that ever happened.


>>   *
>> - * 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,/ ?

Yup.

> 
>> + * subdirectories no further than the quantity specified by \a subdirs
> 
> s/quantity/levels/ ?
> s/$/./

Hrm ... I'm not sure about 'levels'. depth (given maxDepth)?

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

 * Discovered shared objects are added to the files vector.

(unless we s/files/libraries/ of course)

>>   *
>> - * \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./


I think you should codify the rules for periods into checkstyle.py :-)
Or alternatively write them down in the documentation somewhere.

(I wouldn't ordinarily have used a period for a single line comment)

However, in this instance with s/subdirs/maxDepth/ - the code is self
documenting, and I will remove that comment anyway.



>> +			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().


agreed it's a max depth.

>> + *
>> + * 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
--
Kieran