[PATCH v5 09/15] config: Look up IPA configurables in configuration file

[Milan Zamazal]

Hi Barnabás,

Barnabás Pőcze <pobrn at protonmail.com> writes:

> Hi
>
>
> 2024. október 1., kedd 12:28 keltezéssel, Milan Zamazal <mzamazal at redhat.com> írta:
>
>> The configuration snippet:
>> 
>>   configuration:
>>     ipa:
>>       config_paths: CONFIG:PATHS:...
>>       module_paths: MODULE:PATHS:...
>>       force_isolation: BOOL
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  src/libcamera/ipa_manager.cpp | 11 ++++++++---
>>  src/libcamera/ipa_proxy.cpp   | 16 +++++++++++-----
>>  2 files changed, 19 insertions(+), 8 deletions(-)
>> 
>> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
>> index 33ae74e8f..566dc1402 100644
>> --- a/src/libcamera/ipa_manager.cpp
>> +++ b/src/libcamera/ipa_manager.cpp
>> @@ -114,8 +114,11 @@ IPAManager::IPAManager()
>>  	unsigned int ipaCount = 0;
>> 
>>  	/* User-specified paths take precedence. */
>> -	const char *modulePaths = utils::secure_getenv("LIBCAMERA_IPA_MODULE_PATH");
>> -	if (modulePaths) {
>> +	const auto confModulePaths =
>> +		GlobalConfiguration::envOption(
>> +			"LIBCAMERA_IPA_MODULE_PATH", "ipa.module_paths");
>> +	if (confModulePaths.has_value()) {
>> +		const char *modulePaths = confModulePaths.value().c_str();
>
> I think there is no need for this separate variable, i.e.
>
>   auto modulePaths = ...;
>   if (modulePaths) {
>     for (... : utils::split(*modulePaths, ":"))

OK.

>>  		for (const auto &dir : utils::split(modulePaths, ":")) {
>>  			if (dir.empty())
>>  				continue;
>> @@ -289,7 +292,9 @@ bool IPAManager::isSignatureValid([[maybe_unused]] IPAModule *ipa) const
>>  {
>>  #if HAVE_IPA_PUBKEY
>>  	char *force = utils::secure_getenv("LIBCAMERA_IPA_FORCE_ISOLATION");
>> -	if (force && force[0] != '\0') {
>> +	if ((force && force[0] != '\0') ||
>> +	    (!force && GlobalConfiguration::option<bool>("ipa.force_isolation")
>> +			       .value_or(false))) {
>>  		LOG(IPAManager, Debug)
>>  			<< "Isolation of IPA module " << ipa->path()
>>  			<< " forced through environment variable";
>> diff --git a/src/libcamera/ipa_proxy.cpp b/src/libcamera/ipa_proxy.cpp
>> index 85004737c..787d58019 100644
>> --- a/src/libcamera/ipa_proxy.cpp
>> +++ b/src/libcamera/ipa_proxy.cpp
>> @@ -14,6 +14,7 @@
>>  #include <libcamera/base/log.h>
>>  #include <libcamera/base/utils.h>
>> 
>> +#include "libcamera/internal/global_configuration.h"
>>  #include "libcamera/internal/ipa_module.h"
>> 
>>  /**
>> @@ -108,8 +109,11 @@ std::string IPAProxy::configurationFile(const std::string &name,
>>  	std::string ipaName = ipam_->info().name;
>> 
>>  	/* Check the environment variable first. */
>> -	const char *confPaths = utils::secure_getenv("LIBCAMERA_IPA_CONFIG_PATH");
>> -	if (confPaths) {
>> +	auto confConfPaths =
>> +		GlobalConfiguration::envOption(
>> +			"LIBCAMERA_IPA_CONFIG_PATH", "ipa.config_paths");
>> +	if (confConfPaths.has_value()) {
>> +		const char *confPaths = confConfPaths.value().c_str();
>
> Same here.

OK.

>>  		for (const auto &dir : utils::split(confPaths, ":")) {
>>  			if (dir.empty())
>>  				continue;
>> @@ -183,9 +187,11 @@ std::string IPAProxy::resolvePath(const std::string &file) const
>>  	std::string proxyFile = "/" + file;
>> 
>>  	/* Check env variable first. */
>> -	const char *execPaths = utils::secure_getenv("LIBCAMERA_IPA_PROXY_PATH");
>> -	if (execPaths) {
>> -		for (const auto &dir : utils::split(execPaths, ":")) {
>> +	const auto execPaths =
>> +		GlobalConfiguration::envOption(
>> +			"LIBCAMERA_IPA_PROXY_PATH", "ipa.proxy_paths");
>> +	if (execPaths.has_value()) {
>> +		for (const auto &dir : utils::split(execPaths.value().c_str(), ":")) {
>
> Just pass `execPaths.value()` or `*execPaths`.

OK.

>>  			if (dir.empty())
>>  				continue;
>> 
>> --
>> 2.44.1
>> 
>
> I think it would also be desirable if `ipa.X_paths` could be defined as a proper
> array in the configuration file.

Yes, I'll try to arrange it this way.

> Regards,
> Barnabás Pőcze