[v16,03/12] config: Add configuration retrieval helpers
diff mbox series

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

Commit Message

Milan Zamazal July 29, 2025, 7:31 a.m. UTC
Let's add some helpers to make accessing simple configuration values
simpler.

GlobalConfiguration::option ensures that no value is returned rather
than a value of YamlObject::empty.

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

Comments

Paul Elder Sept. 8, 2025, 10:25 a.m. UTC | #1
Hi Milan,

Thanks for the patch.

Quoting Milan Zamazal (2025-07-29 16:31:51)
> Let's add some helpers to make accessing simple configuration values
> simpler.
> 
> GlobalConfiguration::option ensures that no value is returned rather
> than a value of YamlObject::empty.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  .../libcamera/internal/global_configuration.h |  8 +++
>  src/libcamera/global_configuration.cpp        | 53 +++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
> index f695498c4..78bebff9f 100644
> --- a/include/libcamera/internal/global_configuration.h
> +++ b/include/libcamera/internal/global_configuration.h
> @@ -8,6 +8,9 @@
>  #pragma once
>  
>  #include <filesystem>
> +#include <optional>
> +#include <string>
> +#include <string_view>
>  
>  #include "libcamera/internal/yaml_parser.h"
>  
> @@ -22,6 +25,11 @@ public:
>  
>         unsigned int version() const;
>         Configuration configuration() const;
> +       std::optional<std::string> option(
> +               const std::initializer_list<std::string_view> confPath) const;
> +       std::optional<std::string> envOption(
> +               const char *const envVariable,
> +               const std::initializer_list<std::string_view> confPath) const;
>  
>  private:
>         bool loadFile(const std::filesystem::path &fileName);
> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp
> index 50756b859..5be0f0701 100644
> --- a/src/libcamera/global_configuration.cpp
> +++ b/src/libcamera/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>
>  
> @@ -39,6 +41,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)
> @@ -106,6 +113,52 @@ 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
> + * \return A value if an item corresponding to \a confPath exists in the
> + * configuration file, no value otherwise

I think:

\return The value of the configuration item corresponding to \a confPath if it
exists in the configuration file, or no value otherwise

> + */
> +std::optional<std::string> GlobalConfiguration::option(
> +       const std::initializer_list<std::string_view> confPath) const
> +{
> +       const YamlObject *c = &configuration();
> +       for (auto part : confPath) {
> +               c = &(*c)[part];
> +               if (!*c)
> +                       return {};
> +       }
> +       return c->get<std::string>();
> +}
> +
> +/**
> + * \brief Return value of the configuration option from a file or environment

Maybe "Return value of environment variable with a fallback on the
configuration file" ?

Otherwise it sounds like you can actually get the file from either of your
choice.

> + * \param[in] envVariable Environment variable to get the value from
> + * \param[in] confPath The same as in GlobalConfiguration::option

Maybe "The sequence of YAML section names to fall back on when \a envVariable
is unavailable" ?

I personally think that falling back on the config file is an important concept
in this function that should be in the documentation.

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

I wonder if this (returning empty environment variable) will cause any
confusion for users? I did a quick test with setting an environment variable to
an empty string and the unset command gave me the same result when it was set
to an empty string as to when it wasn't set at all.

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

This is a bit convoluted... maybe "The value retrieved from the given
environment if it is set, otherwise the value from the configuration file if it
exists" ?

I like the idea though.


Thanks,

Paul

> + */
> +std::optional<std::string> GlobalConfiguration::envOption(
> +       const char *envVariable,
> +       const std::initializer_list<std::string_view> confPath) const
> +{
> +       const char *envValue = utils::secure_getenv(envVariable);
> +       if (envValue)
> +               return std::optional{ std::string{ envValue } };
> +       return option(confPath);
> +}
> +
>  /**
>   * \brief Return configuration version
>   *
> -- 
> 2.50.1
>
Barnabás Pőcze Sept. 8, 2025, 12:06 p.m. UTC | #2
Hi

2025. 09. 08. 12:25 keltezéssel, Paul Elder írta:
> Hi Milan,
> 
> Thanks for the patch.
> 
> Quoting Milan Zamazal (2025-07-29 16:31:51)
>> Let's add some helpers to make accessing simple configuration values
>> simpler.
>>
>> GlobalConfiguration::option ensures that no value is returned rather
>> than a value of YamlObject::empty.
>>
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>   .../libcamera/internal/global_configuration.h |  8 +++
>>   src/libcamera/global_configuration.cpp        | 53 +++++++++++++++++++
>>   2 files changed, 61 insertions(+)
>>
>> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
>> index f695498c4..78bebff9f 100644
>> --- a/include/libcamera/internal/global_configuration.h
>> +++ b/include/libcamera/internal/global_configuration.h
>> @@ -8,6 +8,9 @@
>>   #pragma once
>>   
>>   #include <filesystem>
>> +#include <optional>
>> +#include <string>
>> +#include <string_view>
>>   
>>   #include "libcamera/internal/yaml_parser.h"
>>   
>> @@ -22,6 +25,11 @@ public:
>>   
>>          unsigned int version() const;
>>          Configuration configuration() const;
>> +       std::optional<std::string> option(
>> +               const std::initializer_list<std::string_view> confPath) const;
>> +       std::optional<std::string> envOption(
>> +               const char *const envVariable,
>> +               const std::initializer_list<std::string_view> confPath) const;
>>   
>>   private:
>>          bool loadFile(const std::filesystem::path &fileName);
>> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp
>> index 50756b859..5be0f0701 100644
>> --- a/src/libcamera/global_configuration.cpp
>> +++ b/src/libcamera/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>
>>   
>> @@ -39,6 +41,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)
>> @@ -106,6 +113,52 @@ 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
>> + * \return A value if an item corresponding to \a confPath exists in the
>> + * configuration file, no value otherwise
> 
> I think:
> 
> \return The value of the configuration item corresponding to \a confPath if it
> exists in the configuration file, or no value otherwise
> 
>> + */
>> +std::optional<std::string> GlobalConfiguration::option(
>> +       const std::initializer_list<std::string_view> confPath) const
>> +{
>> +       const YamlObject *c = &configuration();
>> +       for (auto part : confPath) {
>> +               c = &(*c)[part];
>> +               if (!*c)
>> +                       return {};
>> +       }
>> +       return c->get<std::string>();
>> +}
>> +
>> +/**
>> + * \brief Return value of the configuration option from a file or environment
> 
> Maybe "Return value of environment variable with a fallback on the
> configuration file" ?
> 
> Otherwise it sounds like you can actually get the file from either of your
> choice.
> 
>> + * \param[in] envVariable Environment variable to get the value from
>> + * \param[in] confPath The same as in GlobalConfiguration::option
> 
> Maybe "The sequence of YAML section names to fall back on when \a envVariable
> is unavailable" ?
> 
> I personally think that falling back on the config file is an important concept
> in this function that should be in the documentation.
> 
>> + *
>> + * 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
> 
> I wonder if this (returning empty environment variable) will cause any
> confusion for users? I did a quick test with setting an environment variable to
> an empty string and the unset command gave me the same result when it was set
> to an empty string as to when it wasn't set at all.

So that behaviour was my suggestion. The idea was that sometimes you might want
to explicitly inhibit some kind of behaviour, and you would do that by setting the
env var to the empty string. (Also see one of the later patches that uses the same
logic to inhibit the loading of any configuration.) I actually had your "layers" code
in mind, e.g. using `LIBCAMERA_LAYERS= cam ...` to inhibit the loading of any
layers that would otherwise be loaded based on some configuration, etc. Of course
`env -u LIBCAMERA_LAYERS cam` would also work, but that seems more convoluted.


Regards,
Barnabás Pőcze



> 
>> + * 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
> 
> This is a bit convoluted... maybe "The value retrieved from the given
> environment if it is set, otherwise the value from the configuration file if it
> exists" ?
> 
> I like the idea though.
> 
> 
> Thanks,
> 
> Paul
> 
>> + */
>> +std::optional<std::string> GlobalConfiguration::envOption(
>> +       const char *envVariable,
>> +       const std::initializer_list<std::string_view> confPath) const
>> +{
>> +       const char *envValue = utils::secure_getenv(envVariable);
>> +       if (envValue)
>> +               return std::optional{ std::string{ envValue } };
>> +       return option(confPath);
>> +}
>> +
>>   /**
>>    * \brief Return configuration version
>>    *
>> -- 
>> 2.50.1
>>
Milan Zamazal Sept. 8, 2025, 2:46 p.m. UTC | #3
Hi Paul,

thank you for the wording improvement suggestions, I'll use them when
preparing v17.

Paul Elder <paul.elder@ideasonboard.com> writes:

> Hi Milan,
>
> Thanks for the patch.
>
> Quoting Milan Zamazal (2025-07-29 16:31:51)
>> Let's add some helpers to make accessing simple configuration values
>> simpler.
>> 
>> GlobalConfiguration::option ensures that no value is returned rather
>> than a value of YamlObject::empty.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  .../libcamera/internal/global_configuration.h |  8 +++
>>  src/libcamera/global_configuration.cpp        | 53 +++++++++++++++++++
>>  2 files changed, 61 insertions(+)
>> 
>> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
>> index f695498c4..78bebff9f 100644
>> --- a/include/libcamera/internal/global_configuration.h
>> +++ b/include/libcamera/internal/global_configuration.h
>> @@ -8,6 +8,9 @@
>>  #pragma once
>>  
>>  #include <filesystem>
>> +#include <optional>
>> +#include <string>
>> +#include <string_view>
>>  
>>  #include "libcamera/internal/yaml_parser.h"
>>  
>> @@ -22,6 +25,11 @@ public:
>>  
>>         unsigned int version() const;
>>         Configuration configuration() const;
>> +       std::optional<std::string> option(
>> +               const std::initializer_list<std::string_view> confPath) const;
>> +       std::optional<std::string> envOption(
>> +               const char *const envVariable,
>> +               const std::initializer_list<std::string_view> confPath) const;
>>  
>>  private:
>>         bool loadFile(const std::filesystem::path &fileName);
>> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp
>> index 50756b859..5be0f0701 100644
>> --- a/src/libcamera/global_configuration.cpp
>> +++ b/src/libcamera/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>
>>  
>> @@ -39,6 +41,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)
>> @@ -106,6 +113,52 @@ 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
>> + * \return A value if an item corresponding to \a confPath exists in the
>> + * configuration file, no value otherwise
>
> I think:
>
> \return The value of the configuration item corresponding to \a confPath if it
> exists in the configuration file, or no value otherwise
>
>> + */
>> +std::optional<std::string> GlobalConfiguration::option(
>> +       const std::initializer_list<std::string_view> confPath) const
>> +{
>> +       const YamlObject *c = &configuration();
>> +       for (auto part : confPath) {
>> +               c = &(*c)[part];
>> +               if (!*c)
>> +                       return {};
>> +       }
>> +       return c->get<std::string>();
>> +}
>> +
>> +/**
>> + * \brief Return value of the configuration option from a file or environment
>
> Maybe "Return value of environment variable with a fallback on the
> configuration file" ?
>
> Otherwise it sounds like you can actually get the file from either of your
> choice.
>
>> + * \param[in] envVariable Environment variable to get the value from
>> + * \param[in] confPath The same as in GlobalConfiguration::option
>
> Maybe "The sequence of YAML section names to fall back on when \a envVariable
> is unavailable" ?
>
> I personally think that falling back on the config file is an important concept
> in this function that should be in the documentation.
>
>> + *
>> + * 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
>
> I wonder if this (returning empty environment variable) will cause any
> confusion for users? 

Explained by Barnabás; I accepted his suggestion as it looked reasonable
to me.

> I did a quick test with setting an environment variable to an empty
> string and the unset command gave me the same result when it was set
> to an empty string as to when it wasn't set at all.

Do you mean it doesn't work as described or it differs from the current
behaviour?

>> + * 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
>
> This is a bit convoluted... maybe "The value retrieved from the given
> environment if it is set, otherwise the value from the configuration file if it
> exists" ?
>
> I like the idea though.
>
>
> Thanks,
>
> Paul
>
>> + */
>> +std::optional<std::string> GlobalConfiguration::envOption(
>> +       const char *envVariable,
>> +       const std::initializer_list<std::string_view> confPath) const
>> +{
>> +       const char *envValue = utils::secure_getenv(envVariable);
>> +       if (envValue)
>> +               return std::optional{ std::string{ envValue } };
>> +       return option(confPath);
>> +}
>> +
>>  /**
>>   * \brief Return configuration version
>>   *
>> -- 
>> 2.50.1
>>
Paul Elder Sept. 9, 2025, 2:06 p.m. UTC | #4
Hi Milan,

Quoting Milan Zamazal (2025-09-08 23:46:21)
> Hi Paul,
> 
> thank you for the wording improvement suggestions, I'll use them when
> preparing v17.
> 
> Paul Elder <paul.elder@ideasonboard.com> writes:
> 
> > Hi Milan,
> >
> > Thanks for the patch.
> >
> > Quoting Milan Zamazal (2025-07-29 16:31:51)
> >> Let's add some helpers to make accessing simple configuration values
> >> simpler.
> >> 
> >> GlobalConfiguration::option ensures that no value is returned rather
> >> than a value of YamlObject::empty.
> >> 
> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> >> ---
> >>  .../libcamera/internal/global_configuration.h |  8 +++
> >>  src/libcamera/global_configuration.cpp        | 53 +++++++++++++++++++
> >>  2 files changed, 61 insertions(+)
> >> 
> >> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
> >> index f695498c4..78bebff9f 100644
> >> --- a/include/libcamera/internal/global_configuration.h
> >> +++ b/include/libcamera/internal/global_configuration.h
> >> @@ -8,6 +8,9 @@
> >>  #pragma once
> >>  
> >>  #include <filesystem>
> >> +#include <optional>
> >> +#include <string>
> >> +#include <string_view>
> >>  
> >>  #include "libcamera/internal/yaml_parser.h"
> >>  
> >> @@ -22,6 +25,11 @@ public:
> >>  
> >>         unsigned int version() const;
> >>         Configuration configuration() const;
> >> +       std::optional<std::string> option(
> >> +               const std::initializer_list<std::string_view> confPath) const;
> >> +       std::optional<std::string> envOption(
> >> +               const char *const envVariable,
> >> +               const std::initializer_list<std::string_view> confPath) const;
> >>  
> >>  private:
> >>         bool loadFile(const std::filesystem::path &fileName);
> >> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp
> >> index 50756b859..5be0f0701 100644
> >> --- a/src/libcamera/global_configuration.cpp
> >> +++ b/src/libcamera/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>
> >>  
> >> @@ -39,6 +41,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)
> >> @@ -106,6 +113,52 @@ 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
> >> + * \return A value if an item corresponding to \a confPath exists in the
> >> + * configuration file, no value otherwise
> >
> > I think:
> >
> > \return The value of the configuration item corresponding to \a confPath if it
> > exists in the configuration file, or no value otherwise
> >
> >> + */
> >> +std::optional<std::string> GlobalConfiguration::option(
> >> +       const std::initializer_list<std::string_view> confPath) const
> >> +{
> >> +       const YamlObject *c = &configuration();
> >> +       for (auto part : confPath) {
> >> +               c = &(*c)[part];
> >> +               if (!*c)
> >> +                       return {};
> >> +       }
> >> +       return c->get<std::string>();
> >> +}
> >> +
> >> +/**
> >> + * \brief Return value of the configuration option from a file or environment
> >
> > Maybe "Return value of environment variable with a fallback on the
> > configuration file" ?
> >
> > Otherwise it sounds like you can actually get the file from either of your
> > choice.
> >
> >> + * \param[in] envVariable Environment variable to get the value from
> >> + * \param[in] confPath The same as in GlobalConfiguration::option
> >
> > Maybe "The sequence of YAML section names to fall back on when \a envVariable
> > is unavailable" ?
> >
> > I personally think that falling back on the config file is an important concept
> > in this function that should be in the documentation.
> >
> >> + *
> >> + * 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
> >
> > I wonder if this (returning empty environment variable) will cause any
> > confusion for users? 
> 
> Explained by Barnabás; I accepted his suggestion as it looked reasonable
> to me.

Ah ok I see.

> 
> > I did a quick test with setting an environment variable to an empty
> > string and the unset command gave me the same result when it was set
> > to an empty string as to when it wasn't set at all.
> 
> Do you mean it doesn't work as described or it differs from the current
> behaviour?

Maybe it's I don't know how to control my environment variables. Don't worry
about it :)


Paul

> 
> >> + * 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
> >
> > This is a bit convoluted... maybe "The value retrieved from the given
> > environment if it is set, otherwise the value from the configuration file if it
> > exists" ?
> >
> > I like the idea though.
> >
> >
> > Thanks,
> >
> > Paul
> >
> >> + */
> >> +std::optional<std::string> GlobalConfiguration::envOption(
> >> +       const char *envVariable,
> >> +       const std::initializer_list<std::string_view> confPath) const
> >> +{
> >> +       const char *envValue = utils::secure_getenv(envVariable);
> >> +       if (envValue)
> >> +               return std::optional{ std::string{ envValue } };
> >> +       return option(confPath);
> >> +}
> >> +
> >>  /**
> >>   * \brief Return configuration version
> >>   *
> >> -- 
> >> 2.50.1
> >>
>
Paul Elder Sept. 9, 2025, 2:07 p.m. UTC | #5
Hi Barnabás,

Quoting Barnabás Pőcze (2025-09-08 21:06:43)
> Hi
> 
> 2025. 09. 08. 12:25 keltezéssel, Paul Elder írta:
> > Hi Milan,
> > 
> > Thanks for the patch.
> > 
> > Quoting Milan Zamazal (2025-07-29 16:31:51)
> >> Let's add some helpers to make accessing simple configuration values
> >> simpler.
> >>
> >> GlobalConfiguration::option ensures that no value is returned rather
> >> than a value of YamlObject::empty.
> >>
> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> >> ---
> >>   .../libcamera/internal/global_configuration.h |  8 +++
> >>   src/libcamera/global_configuration.cpp        | 53 +++++++++++++++++++
> >>   2 files changed, 61 insertions(+)
> >>
> >> diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
> >> index f695498c4..78bebff9f 100644
> >> --- a/include/libcamera/internal/global_configuration.h
> >> +++ b/include/libcamera/internal/global_configuration.h
> >> @@ -8,6 +8,9 @@
> >>   #pragma once
> >>   
> >>   #include <filesystem>
> >> +#include <optional>
> >> +#include <string>
> >> +#include <string_view>
> >>   
> >>   #include "libcamera/internal/yaml_parser.h"
> >>   
> >> @@ -22,6 +25,11 @@ public:
> >>   
> >>          unsigned int version() const;
> >>          Configuration configuration() const;
> >> +       std::optional<std::string> option(
> >> +               const std::initializer_list<std::string_view> confPath) const;
> >> +       std::optional<std::string> envOption(
> >> +               const char *const envVariable,
> >> +               const std::initializer_list<std::string_view> confPath) const;
> >>   
> >>   private:
> >>          bool loadFile(const std::filesystem::path &fileName);
> >> diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp
> >> index 50756b859..5be0f0701 100644
> >> --- a/src/libcamera/global_configuration.cpp
> >> +++ b/src/libcamera/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>
> >>   
> >> @@ -39,6 +41,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)
> >> @@ -106,6 +113,52 @@ 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
> >> + * \return A value if an item corresponding to \a confPath exists in the
> >> + * configuration file, no value otherwise
> > 
> > I think:
> > 
> > \return The value of the configuration item corresponding to \a confPath if it
> > exists in the configuration file, or no value otherwise
> > 
> >> + */
> >> +std::optional<std::string> GlobalConfiguration::option(
> >> +       const std::initializer_list<std::string_view> confPath) const
> >> +{
> >> +       const YamlObject *c = &configuration();
> >> +       for (auto part : confPath) {
> >> +               c = &(*c)[part];
> >> +               if (!*c)
> >> +                       return {};
> >> +       }
> >> +       return c->get<std::string>();
> >> +}
> >> +
> >> +/**
> >> + * \brief Return value of the configuration option from a file or environment
> > 
> > Maybe "Return value of environment variable with a fallback on the
> > configuration file" ?
> > 
> > Otherwise it sounds like you can actually get the file from either of your
> > choice.
> > 
> >> + * \param[in] envVariable Environment variable to get the value from
> >> + * \param[in] confPath The same as in GlobalConfiguration::option
> > 
> > Maybe "The sequence of YAML section names to fall back on when \a envVariable
> > is unavailable" ?
> > 
> > I personally think that falling back on the config file is an important concept
> > in this function that should be in the documentation.
> > 
> >> + *
> >> + * 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
> > 
> > I wonder if this (returning empty environment variable) will cause any
> > confusion for users? I did a quick test with setting an environment variable to
> > an empty string and the unset command gave me the same result when it was set
> > to an empty string as to when it wasn't set at all.
> 
> So that behaviour was my suggestion. The idea was that sometimes you might want
> to explicitly inhibit some kind of behaviour, and you would do that by setting the
> env var to the empty string. (Also see one of the later patches that uses the same
> logic to inhibit the loading of any configuration.) I actually had your "layers" code
> in mind, e.g. using `LIBCAMERA_LAYERS= cam ...` to inhibit the loading of any
> layers that would otherwise be loaded based on some configuration, etc. Of course
> `env -u LIBCAMERA_LAYERS cam` would also work, but that seems more convoluted.

Ah I see. Hm, that sounds like a good idea. It's certainly better than
introducing a disable environment variable.

Ok looks good then.


Thanks,

Paul

> 
> 
> Regards,
> Barnabás Pőcze
> 
> 
> 
> > 
> >> + * 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
> > 
> > This is a bit convoluted... maybe "The value retrieved from the given
> > environment if it is set, otherwise the value from the configuration file if it
> > exists" ?
> > 
> > I like the idea though.
> > 
> > 
> > Thanks,
> > 
> > Paul
> > 
> >> + */
> >> +std::optional<std::string> GlobalConfiguration::envOption(
> >> +       const char *envVariable,
> >> +       const std::initializer_list<std::string_view> confPath) const
> >> +{
> >> +       const char *envValue = utils::secure_getenv(envVariable);
> >> +       if (envValue)
> >> +               return std::optional{ std::string{ envValue } };
> >> +       return option(confPath);
> >> +}
> >> +
> >>   /**
> >>    * \brief Return configuration version
> >>    *
> >> -- 
> >> 2.50.1
> >>
>

Patch
diff mbox series

diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h
index f695498c4..78bebff9f 100644
--- a/include/libcamera/internal/global_configuration.h
+++ b/include/libcamera/internal/global_configuration.h
@@ -8,6 +8,9 @@ 
 #pragma once
 
 #include <filesystem>
+#include <optional>
+#include <string>
+#include <string_view>
 
 #include "libcamera/internal/yaml_parser.h"
 
@@ -22,6 +25,11 @@  public:
 
 	unsigned int version() const;
 	Configuration configuration() const;
+	std::optional<std::string> option(
+		const std::initializer_list<std::string_view> confPath) const;
+	std::optional<std::string> envOption(
+		const char *const envVariable,
+		const std::initializer_list<std::string_view> confPath) const;
 
 private:
 	bool loadFile(const std::filesystem::path &fileName);
diff --git a/src/libcamera/global_configuration.cpp b/src/libcamera/global_configuration.cpp
index 50756b859..5be0f0701 100644
--- a/src/libcamera/global_configuration.cpp
+++ b/src/libcamera/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>
 
@@ -39,6 +41,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)
@@ -106,6 +113,52 @@  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
+ * \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::initializer_list<std::string_view> confPath) const
+{
+	const YamlObject *c = &configuration();
+	for (auto part : 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 *envVariable,
+	const std::initializer_list<std::string_view> confPath) const
+{
+	const char *envValue = utils::secure_getenv(envVariable);
+	if (envValue)
+		return std::optional{ std::string{ envValue } };
+	return option(confPath);
+}
+
 /**
  * \brief Return configuration version
  *