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

[Laurent Pinchart]

Hi Kieran,

On Thu, Feb 13, 2020 at 05:55:46PM +0000, Kieran Bingham wrote:
> On 06/02/2020 19:36, Laurent Pinchart wrote:
> > 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.

Interesting, https://en.wiktionary.org/wiki/means

means

    plural of mean

means (plural means)

    An instrument or condition for attaining a purpose.

So means is both the plural of mean, but also a singular whose plural is
means. And people say that French is difficult :-)

> >> 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);

I'm fine with either.

> > 
> >> + * \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)?

Works for me.

> > 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 :-)

I tried to, but it involves parsing comments, and was more complex than
I thought it would be. Implementing a C++ parser in checkstyle.py is
likely overkill. It would be nice if we could extend clang-format with
additional rules written in python :-)

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

Laurent Pinchart