[v10,04/13] config: Add configuration retrieval helpers
diff mbox series

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

Commit Message

Milan Zamazal June 16, 2025, 8:47 a.m. UTC
Let's add some helpers to make accessing simple configuration values
simpler.

GlobalConfiguration::option must check for the key presence before
retrieving the value using operator[] because if the key is not present
then YamlObject::empty may be returned as a regular value.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 .../libcamera/internal/global_configuration.h |  5 ++
 src/libcamera/base/global_configuration.cpp   | 52 +++++++++++++++++++
 2 files changed, 57 insertions(+)

Comments

Laurent Pinchart June 16, 2025, 11:30 p.m. UTC | #1
Hi Milan,

Thank you for the patch.

On Mon, Jun 16, 2025 at 10:47:22AM +0200, Milan Zamazal wrote:
> Let's add some helpers to make accessing simple configuration values
> simpler.
> 
> GlobalConfiguration::option must check for the key presence before
> retrieving the value using operator[] because if the key is not present
> then YamlObject::empty may be returned as a regular value.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  .../libcamera/internal/global_configuration.h |  5 ++
>  src/libcamera/base/global_configuration.cpp   | 52 +++++++++++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
> index 8d0410ac2..5d9531158 100644
> --- a/include/libcamera/internal/global_configuration.h
> +++ b/include/libcamera/internal/global_configuration.h
> @@ -8,6 +8,8 @@
>  #pragma once
>  
>  #include <filesystem>
> +#include <optional>
> +#include <string>
>  
>  #include "libcamera/internal/yaml_parser.h"
>  
> @@ -22,6 +24,9 @@ public:
>  
>  	unsigned int version() const;
>  	Configuration configuration() const;
> +	std::optional<std::string> option(const std::string &confPath) const;
> +	std::optional<std::string> envOption(const char *const envVariable,
> +					     const std::string &confPath) const;
>  
>  private:
>  	bool loadFile(const std::filesystem::path &fileName);
> diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp
> index 1ddf5bc33..1ef63750c 100644
> --- a/src/libcamera/base/global_configuration.cpp
> +++ b/src/libcamera/base/global_configuration.cpp
> @@ -8,6 +8,8 @@
>  #include "libcamera/internal/global_configuration.h"
>  
>  #include <filesystem>
> +#include <memory>
> +#include <string>
>  #include <string_view>
>  #include <sys/types.h>
>  
> @@ -40,6 +42,11 @@ LOG_DEFINE_CATEGORY(Configuration)
>   * is not found then in system-wide configuration directories. If multiple
>   * configuration files exist then only the first one found is used and no
>   * configuration merging is performed.
> + *
> + * The configuration can be accessed using the provided helpers. Namely
> + * GlobalConfiguration::option() or GlobalConfiguration::envOption() to access
> + * individual options or GlobalConfiguration::configuration() to access the
> + * whole configuration.
>   */
>  
>  bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName)
> @@ -112,6 +119,51 @@ GlobalConfiguration::GlobalConfiguration()
>   * the underlying type.
>   */
>  
> +/**
> + * \brief Return value of the configuration option identified by \a confPath
> + * \param[in] confPath Sequence of the YAML section names (excluding
> + * `configuration') leading to the requested option separated by dots

Periods are allowed in key names, but I suppose we can forbid them in
libcamera. Is there a specific reason you picked the period as a
separator over, for instance, '/' ? I suppose somehow matching the
jquery syntax is a good idea.

> + * \return A value if an item corresponding to \a confPath exists in the
> + * configuration file, no value otherwise
> + */
> +std::optional<std::string> GlobalConfiguration::option(const std::string &confPath) const

This seems possibly useful for YamlObject, not just GlobalConfiguration.
Would it make sense to implement it there ?

I'll comment more on this patch after reviewing the callers.

> +{
> +	const YamlObject *c = &configuration();
> +	for (auto part : utils::split(confPath, ".")) {
> +		c = &(*c)[part];
> +		if (!*c)
> +			return {};
> +	}
> +	return c->get<std::string>();
> +}
> +
> +/**
> + * \brief Return value of the configuration option from a file or environment
> + * \param[in] envVariable Environment variable to get the value from
> + * \param[in] confPath The same as in GlobalConfiguration::option
> + *
> + * This helper looks first at the given environment variable and if it is
> + * defined then it returns its value (even if it is empty). Otherwise it looks
> + * for \a confPath the same way as in GlobalConfiguration::option. Only string
> + * values are supported.
> + *
> + * \note Support for using environment variables to configure libcamera behavior
> + * is provided here mostly for backward compatibility reasons. Introducing new
> + * configuration environment variables is discouraged.
> + *
> + * \return A value retrieved from the given environment option or configuration
> + * file or no value if not found
> + */
> +std::optional<std::string> GlobalConfiguration::envOption(
> +	const char *const envVariable,
> +	const std::string &confPath) const
> +{
> +	const char *envValue = utils::secure_getenv(envVariable);
> +	if (envValue)
> +		return std::optional{ std::string{ envValue } };
> +	return option(confPath);
> +}
> +
>  /**
>   * \brief Return configuration version
>   *
Milan Zamazal June 20, 2025, 9:29 p.m. UTC | #2
Hi Laurent,

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patch.
>
> On Mon, Jun 16, 2025 at 10:47:22AM +0200, Milan Zamazal wrote:
>> Let's add some helpers to make accessing simple configuration values
>> simpler.
>> 
>> GlobalConfiguration::option must check for the key presence before
>> retrieving the value using operator[] because if the key is not present
>> then YamlObject::empty may be returned as a regular value.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  .../libcamera/internal/global_configuration.h |  5 ++
>>  src/libcamera/base/global_configuration.cpp   | 52 +++++++++++++++++++
>>  2 files changed, 57 insertions(+)
>> 
>> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
>> index 8d0410ac2..5d9531158 100644
>> --- a/include/libcamera/internal/global_configuration.h
>> +++ b/include/libcamera/internal/global_configuration.h
>> @@ -8,6 +8,8 @@
>>  #pragma once
>>  
>>  #include <filesystem>
>> +#include <optional>
>> +#include <string>
>>  
>>  #include "libcamera/internal/yaml_parser.h"
>>  
>> @@ -22,6 +24,9 @@ public:
>>  
>>  	unsigned int version() const;
>>  	Configuration configuration() const;
>> +	std::optional<std::string> option(const std::string &confPath) const;
>> +	std::optional<std::string> envOption(const char *const envVariable,
>> +					     const std::string &confPath) const;
>>  
>>  private:
>>  	bool loadFile(const std::filesystem::path &fileName);
>> diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp
>> index 1ddf5bc33..1ef63750c 100644
>> --- a/src/libcamera/base/global_configuration.cpp
>> +++ b/src/libcamera/base/global_configuration.cpp
>> @@ -8,6 +8,8 @@
>>  #include "libcamera/internal/global_configuration.h"
>>  
>>  #include <filesystem>
>> +#include <memory>
>> +#include <string>
>>  #include <string_view>
>>  #include <sys/types.h>
>>  
>> @@ -40,6 +42,11 @@ LOG_DEFINE_CATEGORY(Configuration)
>>   * is not found then in system-wide configuration directories. If multiple
>>   * configuration files exist then only the first one found is used and no
>>   * configuration merging is performed.
>> + *
>> + * The configuration can be accessed using the provided helpers. Namely
>> + * GlobalConfiguration::option() or GlobalConfiguration::envOption() to access
>> + * individual options or GlobalConfiguration::configuration() to access the
>> + * whole configuration.
>>   */
>>  
>>  bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName)
>> @@ -112,6 +119,51 @@ GlobalConfiguration::GlobalConfiguration()
>>   * the underlying type.
>>   */
>>  
>> +/**
>> + * \brief Return value of the configuration option identified by \a confPath
>> + * \param[in] confPath Sequence of the YAML section names (excluding
>> + * `configuration') leading to the requested option separated by dots
>
> Periods are allowed in key names, but I suppose we can forbid them in
> libcamera. Is there a specific reason you picked the period as a
> separator over, for instance, '/' ? 

No, I can change it.

> I suppose somehow matching the jquery syntax is a good idea.
>
>> + * \return A value if an item corresponding to \a confPath exists in the
>> + * configuration file, no value otherwise
>> + */
>> +std::optional<std::string> GlobalConfiguration::option(const std::string &confPath) const
>
> This seems possibly useful for YamlObject, not just GlobalConfiguration.
> Would it make sense to implement it there ?

Maybe, I can try.

> I'll comment more on this patch after reviewing the callers.
>
>> +{
>> +	const YamlObject *c = &configuration();
>> +	for (auto part : utils::split(confPath, ".")) {
>> +		c = &(*c)[part];
>> +		if (!*c)
>> +			return {};
>> +	}
>> +	return c->get<std::string>();
>> +}
>> +
>> +/**
>> + * \brief Return value of the configuration option from a file or environment
>> + * \param[in] envVariable Environment variable to get the value from
>> + * \param[in] confPath The same as in GlobalConfiguration::option
>> + *
>> + * This helper looks first at the given environment variable and if it is
>> + * defined then it returns its value (even if it is empty). Otherwise it looks
>> + * for \a confPath the same way as in GlobalConfiguration::option. Only string
>> + * values are supported.
>> + *
>> + * \note Support for using environment variables to configure libcamera behavior
>> + * is provided here mostly for backward compatibility reasons. Introducing new
>> + * configuration environment variables is discouraged.
>> + *
>> + * \return A value retrieved from the given environment option or configuration
>> + * file or no value if not found
>> + */
>> +std::optional<std::string> GlobalConfiguration::envOption(
>> +	const char *const envVariable,
>> +	const std::string &confPath) const
>> +{
>> +	const char *envValue = utils::secure_getenv(envVariable);
>> +	if (envValue)
>> +		return std::optional{ std::string{ envValue } };
>> +	return option(confPath);
>> +}
>> +
>>  /**
>>   * \brief Return configuration version
>>   *
Milan Zamazal June 24, 2025, 8:43 a.m. UTC | #3
Milan Zamazal <mzamazal@redhat.com> writes:

> Hi Laurent,
>
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>
>> Hi Milan,
>>
>> Thank you for the patch.
>>
>> On Mon, Jun 16, 2025 at 10:47:22AM +0200, Milan Zamazal wrote:
>>> Let's add some helpers to make accessing simple configuration values
>>> simpler.
>>> 
>>> GlobalConfiguration::option must check for the key presence before
>>> retrieving the value using operator[] because if the key is not present
>>> then YamlObject::empty may be returned as a regular value.
>>> 
>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>>> ---
>>>  .../libcamera/internal/global_configuration.h |  5 ++
>>>  src/libcamera/base/global_configuration.cpp   | 52 +++++++++++++++++++
>>>  2 files changed, 57 insertions(+)
>>> 
>>> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
>>> index 8d0410ac2..5d9531158 100644
>>> --- a/include/libcamera/internal/global_configuration.h
>>> +++ b/include/libcamera/internal/global_configuration.h
>>> @@ -8,6 +8,8 @@
>>>  #pragma once
>>>  
>>>  #include <filesystem>
>>> +#include <optional>
>>> +#include <string>
>>>  
>>>  #include "libcamera/internal/yaml_parser.h"
>>>  
>>> @@ -22,6 +24,9 @@ public:
>>>  
>>>  	unsigned int version() const;
>>>  	Configuration configuration() const;
>>> +	std::optional<std::string> option(const std::string &confPath) const;
>>> +	std::optional<std::string> envOption(const char *const envVariable,
>>> +					     const std::string &confPath) const;
>>>  
>>>  private:
>>>  	bool loadFile(const std::filesystem::path &fileName);
>>> diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp
>>> index 1ddf5bc33..1ef63750c 100644
>>> --- a/src/libcamera/base/global_configuration.cpp
>>> +++ b/src/libcamera/base/global_configuration.cpp
>>> @@ -8,6 +8,8 @@
>>>  #include "libcamera/internal/global_configuration.h"
>>>  
>>>  #include <filesystem>
>>> +#include <memory>
>>> +#include <string>
>>>  #include <string_view>
>>>  #include <sys/types.h>
>>>  
>>> @@ -40,6 +42,11 @@ LOG_DEFINE_CATEGORY(Configuration)
>>>   * is not found then in system-wide configuration directories. If multiple
>>>   * configuration files exist then only the first one found is used and no
>>>   * configuration merging is performed.
>>> + *
>>> + * The configuration can be accessed using the provided helpers. Namely
>>> + * GlobalConfiguration::option() or GlobalConfiguration::envOption() to access
>>> + * individual options or GlobalConfiguration::configuration() to access the
>>> + * whole configuration.
>>>   */
>>>  
>>>  bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName)
>>> @@ -112,6 +119,51 @@ GlobalConfiguration::GlobalConfiguration()
>>>   * the underlying type.
>>>   */
>>>  
>>> +/**
>>> + * \brief Return value of the configuration option identified by \a confPath
>>> + * \param[in] confPath Sequence of the YAML section names (excluding
>>> + * `configuration') leading to the requested option separated by dots
>>
>> Periods are allowed in key names, but I suppose we can forbid them in
>> libcamera. Is there a specific reason you picked the period as a
>> separator over, for instance, '/' ? 
>
> No, I can change it.
>
>> I suppose somehow matching the jquery syntax is a good idea.
>>
>>> + * \return A value if an item corresponding to \a confPath exists in the
>>> + * configuration file, no value otherwise
>>> + */
>>> +std::optional<std::string> GlobalConfiguration::option(const std::string &confPath) const
>>
>> This seems possibly useful for YamlObject, not just GlobalConfiguration.
>> Would it make sense to implement it there ?
>
> Maybe, I can try.

Tried but it doesn't look that useful there, so I decided not to put it
there.

>> I'll comment more on this patch after reviewing the callers.
>>
>>> +{
>>> +	const YamlObject *c = &configuration();
>>> +	for (auto part : utils::split(confPath, ".")) {
>>> +		c = &(*c)[part];
>>> +		if (!*c)
>>> +			return {};
>>> +	}
>>> +	return c->get<std::string>();
>>> +}
>>> +
>>> +/**
>>> + * \brief Return value of the configuration option from a file or environment
>>> + * \param[in] envVariable Environment variable to get the value from
>>> + * \param[in] confPath The same as in GlobalConfiguration::option
>>> + *
>>> + * This helper looks first at the given environment variable and if it is
>>> + * defined then it returns its value (even if it is empty). Otherwise it looks
>>> + * for \a confPath the same way as in GlobalConfiguration::option. Only string
>>> + * values are supported.
>>> + *
>>> + * \note Support for using environment variables to configure libcamera behavior
>>> + * is provided here mostly for backward compatibility reasons. Introducing new
>>> + * configuration environment variables is discouraged.
>>> + *
>>> + * \return A value retrieved from the given environment option or configuration
>>> + * file or no value if not found
>>> + */
>>> +std::optional<std::string> GlobalConfiguration::envOption(
>>> +	const char *const envVariable,
>>> +	const std::string &confPath) const
>>> +{
>>> +	const char *envValue = utils::secure_getenv(envVariable);
>>> +	if (envValue)
>>> +		return std::optional{ std::string{ envValue } };
>>> +	return option(confPath);
>>> +}
>>> +
>>>  /**
>>>   * \brief Return configuration version
>>>   *

Patch
diff mbox series

diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
index 8d0410ac2..5d9531158 100644
--- a/include/libcamera/internal/global_configuration.h
+++ b/include/libcamera/internal/global_configuration.h
@@ -8,6 +8,8 @@ 
 #pragma once
 
 #include <filesystem>
+#include <optional>
+#include <string>
 
 #include "libcamera/internal/yaml_parser.h"
 
@@ -22,6 +24,9 @@  public:
 
 	unsigned int version() const;
 	Configuration configuration() const;
+	std::optional<std::string> option(const std::string &confPath) const;
+	std::optional<std::string> envOption(const char *const envVariable,
+					     const std::string &confPath) const;
 
 private:
 	bool loadFile(const std::filesystem::path &fileName);
diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp
index 1ddf5bc33..1ef63750c 100644
--- a/src/libcamera/base/global_configuration.cpp
+++ b/src/libcamera/base/global_configuration.cpp
@@ -8,6 +8,8 @@ 
 #include "libcamera/internal/global_configuration.h"
 
 #include <filesystem>
+#include <memory>
+#include <string>
 #include <string_view>
 #include <sys/types.h>
 
@@ -40,6 +42,11 @@  LOG_DEFINE_CATEGORY(Configuration)
  * is not found then in system-wide configuration directories. If multiple
  * configuration files exist then only the first one found is used and no
  * configuration merging is performed.
+ *
+ * The configuration can be accessed using the provided helpers. Namely
+ * GlobalConfiguration::option() or GlobalConfiguration::envOption() to access
+ * individual options or GlobalConfiguration::configuration() to access the
+ * whole configuration.
  */
 
 bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName)
@@ -112,6 +119,51 @@  GlobalConfiguration::GlobalConfiguration()
  * the underlying type.
  */
 
+/**
+ * \brief Return value of the configuration option identified by \a confPath
+ * \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
+ */
+std::optional<std::string> GlobalConfiguration::option(const std::string &confPath) const
+{
+	const YamlObject *c = &configuration();
+	for (auto part : utils::split(confPath, ".")) {
+		c = &(*c)[part];
+		if (!*c)
+			return {};
+	}
+	return c->get<std::string>();
+}
+
+/**
+ * \brief Return value of the configuration option from a file or environment
+ * \param[in] envVariable Environment variable to get the value from
+ * \param[in] confPath The same as in GlobalConfiguration::option
+ *
+ * This helper looks first at the given environment variable and if it is
+ * defined then it returns its value (even if it is empty). Otherwise it looks
+ * for \a confPath the same way as in GlobalConfiguration::option. Only string
+ * values are supported.
+ *
+ * \note Support for using environment variables to configure libcamera behavior
+ * is provided here mostly for backward compatibility reasons. Introducing new
+ * configuration environment variables is discouraged.
+ *
+ * \return A value retrieved from the given environment option or configuration
+ * file or no value if not found
+ */
+std::optional<std::string> GlobalConfiguration::envOption(
+	const char *const envVariable,
+	const std::string &confPath) const
+{
+	const char *envValue = utils::secure_getenv(envVariable);
+	if (envValue)
+		return std::optional{ std::string{ envValue } };
+	return option(confPath);
+}
+
 /**
  * \brief Return configuration version
  *