| Message ID | 20241001102810.479285-5-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal <mzamazal@redhat.com> írta: > In addition to LIBCAMERA_LOG_LEVELS environment variable, expose the > same configuration in the configuration file. > > Because logging asks for configuration and configuration loading uses > logging, extra care is needed. We must prevent calling any log function > before configuration is loaded. Otherwise the log function would ask > for configuration, it would load the configuration files and would try > to log the results. Which can lead to various breakages, even quite > subtle; this has been demonstrated in camera_reconfigure test when > trying to make the configuration initialization more transparent, as a > static variable instance with automatic initialization rather than a > pointer with manual initialization() invocation. > > But we must log at least errors when reading the configuration. > Especially, we cannot prevent logging in configuration initialization > entirely because the YAML parser may log errors. But in case of an > error during configuration initialization, the configuration cannot be > read anyway and the statically created initial global configuration > instance serves the purpose of providing some, empty configuration. > Then the errors from the YAML parser can be logged. > > We cannot use the same mechanism for other logging in configuration > initialization because then logging would read and use the empty > configuration, which is not a valid configuration this time, for the > whole run of libcamera. > > The configuration must be initialized, which is done by calling > initialize() method in IPAManager constructor. A more suitable place > would be CameraManager constructor but it would be too late there, the > IPAManager constructor is called earlier. > > The configuration snippet for logging looks as follows: > > configuration: > log: > levels: LIBCAMERA_LOG_LEVELS > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > [...] > diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp > index cfc24d389..33ae74e8f 100644 > --- a/src/libcamera/ipa_manager.cpp > +++ b/src/libcamera/ipa_manager.cpp > @@ -16,6 +16,7 @@ > #include <libcamera/base/log.h> > #include <libcamera/base/utils.h> > > +#include "libcamera/internal/global_configuration.h" > #include "libcamera/internal/ipa_module.h" > #include "libcamera/internal/ipa_proxy.h" > #include "libcamera/internal/pipeline_handler.h" > @@ -103,6 +104,8 @@ LOG_DEFINE_CATEGORY(IPAManager) > */ > IPAManager::IPAManager() > { > + GlobalConfiguration::initialize(); > [...] This looks very fragile. I think there is an appreciable risk of an unrelated change upsetting the order of construction so that this no longer works as expected. Regards, Barnabás Pőcze
Hi Barnabás, Barnabás Pőcze <pobrn@protonmail.com> writes: > Hi > > > 2024. október 1., kedd 12:27 keltezéssel, Milan Zamazal <mzamazal@redhat.com> írta: > >> In addition to LIBCAMERA_LOG_LEVELS environment variable, expose the >> same configuration in the configuration file. >> >> Because logging asks for configuration and configuration loading uses >> logging, extra care is needed. We must prevent calling any log function >> before configuration is loaded. Otherwise the log function would ask >> for configuration, it would load the configuration files and would try >> to log the results. Which can lead to various breakages, even quite >> subtle; this has been demonstrated in camera_reconfigure test when >> trying to make the configuration initialization more transparent, as a >> static variable instance with automatic initialization rather than a >> pointer with manual initialization() invocation. >> >> But we must log at least errors when reading the configuration. >> Especially, we cannot prevent logging in configuration initialization >> entirely because the YAML parser may log errors. But in case of an >> error during configuration initialization, the configuration cannot be >> read anyway and the statically created initial global configuration >> instance serves the purpose of providing some, empty configuration. >> Then the errors from the YAML parser can be logged. >> >> We cannot use the same mechanism for other logging in configuration >> initialization because then logging would read and use the empty >> configuration, which is not a valid configuration this time, for the >> whole run of libcamera. >> >> The configuration must be initialized, which is done by calling >> initialize() method in IPAManager constructor. A more suitable place >> would be CameraManager constructor but it would be too late there, the >> IPAManager constructor is called earlier. >> >> The configuration snippet for logging looks as follows: >> >> configuration: >> log: >> levels: LIBCAMERA_LOG_LEVELS >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> [...] >> diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp >> index cfc24d389..33ae74e8f 100644 >> --- a/src/libcamera/ipa_manager.cpp >> +++ b/src/libcamera/ipa_manager.cpp >> @@ -16,6 +16,7 @@ >> #include <libcamera/base/log.h> >> #include <libcamera/base/utils.h> >> >> +#include "libcamera/internal/global_configuration.h" >> #include "libcamera/internal/ipa_module.h" >> #include "libcamera/internal/ipa_proxy.h" >> #include "libcamera/internal/pipeline_handler.h" >> @@ -103,6 +104,8 @@ LOG_DEFINE_CATEGORY(IPAManager) >> */ >> IPAManager::IPAManager() >> { >> + GlobalConfiguration::initialize(); >> [...] > > This looks very fragile. I think there is an appreciable risk of an unrelated change > upsetting the order of construction so that this no longer works as expected. Yes, but does anybody have any idea how to do better? > Regards, > Barnabás Pőcze
diff --git a/include/libcamera/internal/global_configuration.h b/include/libcamera/internal/global_configuration.h index 628d85cb8..8058fd8e2 100644 --- a/include/libcamera/internal/global_configuration.h +++ b/include/libcamera/internal/global_configuration.h @@ -35,6 +35,7 @@ private: bool loadFile(const std::filesystem::path &fileName); void load(); + static Configuration get(); }; diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp index 7dcf97265..c6f4ebd7b 100644 --- a/src/libcamera/base/global_configuration.cpp +++ b/src/libcamera/base/global_configuration.cpp @@ -19,11 +19,19 @@ namespace libcamera { +LOG_DEFINE_CATEGORY(Configuration) + std::unique_ptr<GlobalConfiguration> GlobalConfiguration::instance_ = std::make_unique<GlobalConfiguration>(); /** - * \brief Do not create GlobalConfiguration instance directly, use initialize() + * \brief Initialize the global configuration + * + * This method is expected to be called only once, before the configuration is + * queried for the first time. + * + * \note Most notably, the method must be called before any logging, because + * logging queries the configuration. */ void GlobalConfiguration::initialize() { @@ -40,6 +48,13 @@ void GlobalConfiguration::initialize() * The configuration file is a YAML file and the configuration itself is stored * under `configuration' top-level item. * + * Example configuration file content: + * \code{.yaml} + * configuration: + * log: + * levels: 'IPAManager:DEBUG' + * \endcode + * * The configuration file is looked up in user's home directory first and if it * is not found then in system-wide configuration directories. If multiple * configuration files exist then only the first one found is used and no @@ -57,6 +72,9 @@ void GlobalConfiguration::initialize() * the underlying type. */ +/** + * \brief Do not create GlobalConfiguration instance directly, use initialize() + */ GlobalConfiguration::GlobalConfiguration() : configuration_(std::make_unique<YamlObject>()) { @@ -68,6 +86,11 @@ const std::vector<std::filesystem::path> std::filesystem::path("/etc/libcamera/configuration.yaml"), }; +/* + * Care is needed here to not log anything before the configuration is + * loaded otherwise the logger would be initialized with empty configuration. + */ + void GlobalConfiguration::load() { std::filesystem::path userConfigurationDirectory; @@ -100,12 +123,18 @@ bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName) return false; } - if (!file.open(File::OpenModeFlag::ReadOnly)) + if (!file.open(File::OpenModeFlag::ReadOnly)) { + LOG(Configuration, Warning) + << "Failed to open configuration file " << fileName; return true; + } auto root = YamlParser::parse(file); - if (!root) + if (!root) { + LOG(Configuration, Warning) + << "Failed to parse configuration file " << fileName; return true; + } configuration_ = std::move(root); return true; diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp index 3a656b8f0..0f6767107 100644 --- a/src/libcamera/base/log.cpp +++ b/src/libcamera/base/log.cpp @@ -14,6 +14,7 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <string> #include <syslog.h> #include <time.h> #include <unordered_set> @@ -25,6 +26,8 @@ #include <libcamera/base/thread.h> #include <libcamera/base/utils.h> +#include "libcamera/internal/global_configuration.h" + /** * \file base/log.h * \brief Logging infrastructure @@ -626,8 +629,13 @@ void Logger::parseLogFile() void Logger::parseLogLevels() { const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS"); - if (!debug) - return; + if (!debug) { + const std::optional<std::string> confDebug = + GlobalConfiguration::configuration()["log"]["levels"].get<std::string>(); + if (!confDebug.has_value()) + return; + debug = confDebug.value().c_str(); + } for (const char *pair = debug; *debug != '\0'; pair = debug) { const char *comma = strchrnul(debug, ','); diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp index cfc24d389..33ae74e8f 100644 --- a/src/libcamera/ipa_manager.cpp +++ b/src/libcamera/ipa_manager.cpp @@ -16,6 +16,7 @@ #include <libcamera/base/log.h> #include <libcamera/base/utils.h> +#include "libcamera/internal/global_configuration.h" #include "libcamera/internal/ipa_module.h" #include "libcamera/internal/ipa_proxy.h" #include "libcamera/internal/pipeline_handler.h" @@ -103,6 +104,8 @@ LOG_DEFINE_CATEGORY(IPAManager) */ IPAManager::IPAManager() { + GlobalConfiguration::initialize(); + #if HAVE_IPA_PUBKEY if (!pubKey_.isValid()) LOG(IPAManager, Warning) << "Public key not valid";
In addition to LIBCAMERA_LOG_LEVELS environment variable, expose the same configuration in the configuration file. Because logging asks for configuration and configuration loading uses logging, extra care is needed. We must prevent calling any log function before configuration is loaded. Otherwise the log function would ask for configuration, it would load the configuration files and would try to log the results. Which can lead to various breakages, even quite subtle; this has been demonstrated in camera_reconfigure test when trying to make the configuration initialization more transparent, as a static variable instance with automatic initialization rather than a pointer with manual initialization() invocation. But we must log at least errors when reading the configuration. Especially, we cannot prevent logging in configuration initialization entirely because the YAML parser may log errors. But in case of an error during configuration initialization, the configuration cannot be read anyway and the statically created initial global configuration instance serves the purpose of providing some, empty configuration. Then the errors from the YAML parser can be logged. We cannot use the same mechanism for other logging in configuration initialization because then logging would read and use the empty configuration, which is not a valid configuration this time, for the whole run of libcamera. The configuration must be initialized, which is done by calling initialize() method in IPAManager constructor. A more suitable place would be CameraManager constructor but it would be too late there, the IPAManager constructor is called earlier. The configuration snippet for logging looks as follows: configuration: log: levels: LIBCAMERA_LOG_LEVELS Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- .../libcamera/internal/global_configuration.h | 1 + src/libcamera/base/global_configuration.cpp | 35 +++++++++++++++++-- src/libcamera/base/log.cpp | 12 +++++-- src/libcamera/ipa_manager.cpp | 3 ++ 4 files changed, 46 insertions(+), 5 deletions(-)