[libcamera-devel] [PATCH] libcamera: ipa_manager: Rework error messages when enumerating IPAs

[Paul Elder]

Hi Laurent,

Thank you for the patch.

On Sun, Sep 15, 2019 at 11:23:18PM +0300, Laurent Pinchart wrote:
> When enumerating IPAs, the system IPA directory and all the directories
> listed in the LIBCAMERA_IPA_MODULE_PATH environment variable are listed
> in turn. Failing to list any of those directories results in an error
> message being printed for every failure. This is particularly common
> when developing libcamera, as IPAs may not have been installed locally.
> 
> To avoid unnecessarily worrying error messages, rework the enumeration
> procedure to only print a message when no IPA can be found at all.
> Individual missing directories are not considered as an issue anymore.
> The message is also downgraded from an error to a warning as the
> situation may still be normal.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/ipa_manager.cpp | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
> index 0a908eae6261..f3e08375820c 100644
> --- a/src/libcamera/ipa_manager.cpp
> +++ b/src/libcamera/ipa_manager.cpp
> @@ -94,11 +94,19 @@ LOG_DEFINE_CATEGORY(IPAManager)
>  
>  IPAManager::IPAManager()
>  {
> -	addDir(IPA_MODULE_DIR);
> +	unsigned int ipaCount = 0;
> +	int ret;
> +
> +	ret = addDir(IPA_MODULE_DIR);
> +	if (ret > 0)
> +		ipaCount += ret;
>  	const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
> -	if (!modulePaths)
> +	if (!modulePaths) {
> +		LOG(IPAManager, Warning)
> +			<< "No IPA found in '" IPA_MODULE_DIR "'";

We need to check ipaCount here, since we could have loaded some IPAs
from the global dir, and just not have the environment variable
specified.

>  		return;
> +	}
>  
>  	while (1) {
>  		const char *delim = strchrnul(modulePaths, ':');
> @@ -106,7 +114,9 @@ IPAManager::IPAManager()
>  
>  		if (count) {
>  			std::string path(modulePaths, count);
> -			addDir(path.c_str());
> +			ret = addDir(path.c_str());
> +			if (ret > 0)
> +				ipaCount += ret;
>  		}
>  
>  		if (*delim == '\0')
> @@ -114,6 +124,11 @@ IPAManager::IPAManager()
>  
>  		modulePaths += count + 1;
>  	}
> +
> +	if (!ipaCount)
> +		LOG(IPAManager, Warning)
> +			<< "No IPA found in '" IPA_MODULE_DIR "' and '"
> +			<< modulePaths << "'";

I presume you're fine with modulePaths being printed as-is, as a
colon-separated list of directories.

>  }
>  
>  IPAManager::~IPAManager()
> @@ -155,9 +170,6 @@ int IPAManager::addDir(const char *libDir)
>  	dir = opendir(libDir);
>  	if (!dir) {
>  		int ret = -errno;
> -		LOG(IPAManager, Error)
> -			<< "Invalid path " << libDir << " for IPA modules: "
> -			<< strerror(-ret);
>  		return ret;

Could this be shortened to
return -errno; ?

>  	}

With these changes,
Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>


Paul