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

[Barnabás Pőcze]

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



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


>  		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`.


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


Regards,
Barnabás Pőcze