[PATCH v5 07/15] config: Look up log color configuration in configuration file

[Milan Zamazal]

Hi Barnabás,

thank you for review.

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

> Hi
>
>
> 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal <mzamazal at redhat.com> írta:
>
>> The configuration snippet:
>> 
>>   configuration:
>>     log:
>>       color: BOOL
>> 
>> In order to use the global configuration access helper for a boolean
>> value, we must make a template from it.  To make the template visible,
>> we must implement it in the header file, otherwise undefined reference
>> error would be reported when linking.
>> 
>> We don't implement envOption helper for boolean here, because we would
>> have to deal with a specific value interpretation in this particular
>> case.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  .../libcamera/internal/global_configuration.h | 27 ++++++++++++++++++-
>>  src/libcamera/base/global_configuration.cpp   | 20 ++++----------
>>  src/libcamera/base/log.cpp                    |  2 ++
>>  3 files changed, 33 insertions(+), 16 deletions(-)
>> 
>> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
>> index b1529f392..d34410744 100644
>> --- a/include/libcamera/internal/global_configuration.h
>> +++ b/include/libcamera/internal/global_configuration.h
>> @@ -12,6 +12,8 @@
>>  #include <string>
>>  #include <vector>
>> 
>> +#include <libcamera/base/utils.h>
>> +
>>  #include "libcamera/internal/yaml_parser.h"
>> 
>>  namespace libcamera {
>> @@ -27,7 +29,30 @@ public:
>> 
>>  	static unsigned int version();
>>  	static Configuration configuration();
>> -	static std::optional<std::string> option(const std::string &confPath);
>> +
>> +	template<typename T,
>> +		 std::enable_if_t<
>> +			 std::is_same_v<bool, T> ||
>> +			 std::is_same_v<double, T> ||
>> +			 std::is_same_v<int8_t, T> ||
>> +			 std::is_same_v<uint8_t, T> ||
>> +			 std::is_same_v<int16_t, T> ||
>> +			 std::is_same_v<uint16_t, T> ||
>> +			 std::is_same_v<int32_t, T> ||
>> +			 std::is_same_v<uint32_t, T> ||
>> +			 std::is_same_v<std::string, T> ||
>> +			 std::is_same_v<Size, T>> * = nullptr>
>> +	static std::optional<T> option(const std::string &confPath)
>> +	{
>> +		YamlObject *c = &const_cast<YamlObject &>(configuration());
>
> Why are the `const_cast`s needed?

They are not, I'll remove them.

>> +		for (auto part : utils::details::StringSplitter(confPath, "."))
>
> utils::split(...)

OK.

>> +			if (c->contains(part))
>
> You can get rid of this check, e.g.
>
>   for (...) {
>     c = &(*c)[part];
>     if (!*c)
>       return {};
>   }
>
> It does not look particularly aestethic, though...

I don't have a preference but your version is probably more common, so
I'll use it.

>> +				c = &const_cast<YamlObject &>((*c)[part]);
>> +			else
>> +				return std::optional<T>();
>> +		return c->get<T>();
>> +	}
>> +
>>  	static std::optional<std::string> envOption(const char *const envVariable,
>>  						    const std::string &confPath);
>> 
>> diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp
>> index 1243a5f4e..82d1339c3 100644
>> --- a/src/libcamera/base/global_configuration.cpp
>> +++ b/src/libcamera/base/global_configuration.cpp
>> @@ -14,7 +14,6 @@
>> 
>>  #include <libcamera/base/file.h>
>>  #include <libcamera/base/log.h>
>> -#include <libcamera/base/utils.h>
>> 
>>  #include "libcamera/internal/yaml_parser.h"
>> 
>> @@ -150,23 +149,14 @@ GlobalConfiguration::Configuration GlobalConfiguration::get()
>>  }
>> 
>>  /**
>> + * \fn static std::optional<T> GlobalConfiguration::option(const char *const confPath)
>>   * \brief Return value of the configuration option identified by \a confPath
>> + * \tparam T The type of the retrieved configuration value
>>   * \param[in] confPath Sequence of the YAML section names (excluding
>>   * `configuration') leading to the requested option separated by dots
>> - * \return A value if an item corresponding to \a confPath exists in the
>> - * configuration file, no value otherwise
>> + * \return A value of type \a T if an item corresponding to \a confPath exists
>> + * in the configuration file and matches type \a T, no value otherwise
>>   */
>> -std::optional<std::string> GlobalConfiguration::option(
>> -	const std::string &confPath)
>> -{
>> -	YamlObject *c = &const_cast<YamlObject &>(configuration());
>> -	for (auto part : utils::details::StringSplitter(confPath, "."))
>> -		if (c->contains(part))
>> -			c = &const_cast<YamlObject &>((*c)[part]);
>> -		else
>> -			return std::optional<std::string>();
>> -	return c->get<std::string>();
>> -}
>> 
>>  /**
>>   * \brief Return value of the configuration option from a file or environment
>> @@ -192,7 +182,7 @@ std::optional<std::string> GlobalConfiguration::envOption(
>>  	const char *envValue = utils::secure_getenv(envVariable);
>>  	if (envValue)
>>  		return std::optional{ std::string{ envValue } };
>> -	return option(confPath);
>> +	return option<std::string>(confPath);
>>  }
>> 
>>  /**
>> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
>> index 07352f6c1..d522e895e 100644
>> --- a/src/libcamera/base/log.cpp
>> +++ b/src/libcamera/base/log.cpp
>> @@ -590,6 +590,8 @@ void Logger::logSetLevel(const char *category, const char *level)
>>  Logger::Logger()
>>  {
>>  	bool color = !utils::secure_getenv("LIBCAMERA_LOG_NO_COLOR");
>> +	if (color)
>> +		color = GlobalConfiguration::option<bool>("log.color").value_or(true);
>>  	logSetStream(&std::cerr, color);
>> 
>>  	parseLogFile();
>> --
>> 2.44.1
>> 
>> 
>
>
> Regards,
> Barnabás Pőcze