Message ID | 20250729073201.5369-4-mzamazal@redhat.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
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 >
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 >>
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 >>
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 > >> >
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 > >> >
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 *
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(+)