[v5,09/15] config: Look up IPA configurables in configuration file
diff mbox series

Message ID 20241001102810.479285-10-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Add global configuration file
Related show

Commit Message

Milan Zamazal Oct. 1, 2024, 10:28 a.m. UTC 
The configuration snippet:

  configuration:
    ipa:
      config_paths: CONFIG:PATHS:...
      module_paths: MODULE:PATHS:...
      force_isolation: BOOL

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/libcamera/ipa_manager.cpp | 11 ++++++++---
 src/libcamera/ipa_proxy.cpp   | 16 +++++++++++-----
 2 files changed, 19 insertions(+), 8 deletions(-)

Comments

Barnabás Pőcze Dec. 4, 2024, 5:45 p.m. UTC | #1 
Hi


2024. október 1., kedd 12:28 keltezéssel, Milan Zamazal <mzamazal@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@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
Milan Zamazal Dec. 5, 2024, 11:57 a.m. UTC | #2 
Hi Barnabás,

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

> Hi
>
>
> 2024. október 1., kedd 12:28 keltezéssel, Milan Zamazal <mzamazal@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@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

Patch
diff mbox series 

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();
 		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();
 		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(), ":")) {
 			if (dir.empty())
 				continue;