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

[Kieran Bingham]

Hi Laurent,

On 18/09/2019 14:14, Laurent Pinchart wrote:
> Hi Paul,
> 
> On Tue, Sep 17, 2019 at 02:07:03AM -0400, Paul Elder wrote:
>> 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.
> 
> Oops, indeed. I'll fix this.
> 
>>>  		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.
> 
> I think it's good enough for now, yes.
> 
>>>  }
>>>  
>>>  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; ?
> 
> Unfortunately not, as LOG() could result in errno being modified. That's
> why we have these constructs through the code where we store -errno in
> ret.

You mean the LOG() which you have removed right ;-)

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

-- 
Regards
--
Kieran