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

[Barnabás Pőcze]

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?


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

utils::split(...)


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


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