[libcamera-devel] libcamera: ipa_manager: Sort IPA modules by name

2094 diff mbox series

Sort IPA modules by name when enumerating modules in a directory in
order to guarantee a stable ordering. This eases debugging by making
issues more reproducible.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/ipa_manager.cpp | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

Hi Laurent,

On Fri, Oct 04, 2019 at 12:01:13AM +0300, Laurent Pinchart wrote:
> Sort IPA modules by name when enumerating modules in a directory in
> order to guarantee a stable ordering. This eases debugging by making
> issues more reproducible.
>

This is welcome and I think you should simply add
+               LOG(IPAManager, Debug) << "Loaded IPA module '" << path << "'";

So that we drop my: "ipa: ipa_manager: Print the loaded IPA modules path"

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/ipa_manager.cpp | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 708233e8a9c7..27aa17920bab 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -7,6 +7,7 @@
>
>  #include "ipa_manager.h"
>
> +#include <algorithm>
>  #include <dirent.h>
>  #include <string.h>
>  #include <sys/types.h>
> @@ -112,7 +113,7 @@ int IPAManager::addDir(const char *libDir)
>  	if (!dir)
>  		return -errno;
>
> -	unsigned int count = 0;
> +	std::vector<std::string> paths;
>  	while ((ent = readdir(dir)) != nullptr) {
>  		int offset = strlen(ent->d_name) - 3;
>  		if (offset < 0)
> @@ -120,8 +121,16 @@ int IPAManager::addDir(const char *libDir)
>  		if (strcmp(&ent->d_name[offset], ".so"))
>  			continue;
>
> -		IPAModule *ipaModule = new IPAModule(std::string(libDir) +
> -						     "/" + ent->d_name);
> +		paths.push_back(std::string(libDir) + "/" + ent->d_name);
> +	}
> +	closedir(dir);
> +
> +	/* Ensure a stable ordering of modules. */
> +	std::sort(paths.begin(), paths.end());
> +
> +	unsigned int count = 0;
> +	for (const std::string &path : paths) {
> +		IPAModule *ipaModule = new IPAModule(path);
>  		if (!ipaModule->isValid()) {
>  			delete ipaModule;
>  			continue;
> @@ -131,7 +140,6 @@ int IPAManager::addDir(const char *libDir)
>  		count++;
>  	}
>
> -	closedir(dir);
>  	return count;
>  }
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

On Fri, Oct 04, 2019 at 05:03:46PM +0200, Jacopo Mondi wrote:
> On Fri, Oct 04, 2019 at 12:01:13AM +0300, Laurent Pinchart wrote:
> > Sort IPA modules by name when enumerating modules in a directory in
> > order to guarantee a stable ordering. This eases debugging by making
> > issues more reproducible.
> 
> This is welcome and I think you should simply add
> +               LOG(IPAManager, Debug) << "Loaded IPA module '" << path << "'";
> 
> So that we drop my: "ipa: ipa_manager: Print the loaded IPA modules path"

Niklas also keeps asking us to split patches, are you proposing doing
the opposite ? :-) I'll push your patch on top of this.

> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/ipa_manager.cpp | 16 ++++++++++++----
> >  1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> > index 708233e8a9c7..27aa17920bab 100644
> > --- a/src/libcamera/ipa_manager.cpp
> > +++ b/src/libcamera/ipa_manager.cpp
> > @@ -7,6 +7,7 @@
> >
> >  #include "ipa_manager.h"
> >
> > +#include <algorithm>
> >  #include <dirent.h>
> >  #include <string.h>
> >  #include <sys/types.h>
> > @@ -112,7 +113,7 @@ int IPAManager::addDir(const char *libDir)
> >  	if (!dir)
> >  		return -errno;
> >
> > -	unsigned int count = 0;
> > +	std::vector<std::string> paths;
> >  	while ((ent = readdir(dir)) != nullptr) {
> >  		int offset = strlen(ent->d_name) - 3;
> >  		if (offset < 0)
> > @@ -120,8 +121,16 @@ int IPAManager::addDir(const char *libDir)
> >  		if (strcmp(&ent->d_name[offset], ".so"))
> >  			continue;
> >
> > -		IPAModule *ipaModule = new IPAModule(std::string(libDir) +
> > -						     "/" + ent->d_name);
> > +		paths.push_back(std::string(libDir) + "/" + ent->d_name);
> > +	}
> > +	closedir(dir);
> > +
> > +	/* Ensure a stable ordering of modules. */
> > +	std::sort(paths.begin(), paths.end());
> > +
> > +	unsigned int count = 0;
> > +	for (const std::string &path : paths) {
> > +		IPAModule *ipaModule = new IPAModule(path);
> >  		if (!ipaModule->isValid()) {
> >  			delete ipaModule;
> >  			continue;
> > @@ -131,7 +140,6 @@ int IPAManager::addDir(const char *libDir)
> >  		count++;
> >  	}
> >
> > -	closedir(dir);
> >  	return count;
> >  }
> >