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