[RFC,03/11] config: Look up logging levels in the configuration file
diff mbox series

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

Commit Message

Milan Zamazal March 26, 2024, 11:24 a.m. UTC
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 logged.  Otherwise the log function would ask for
configuration, it would load the configuration files and would try to log the
results, which would cause mutex deadlock due to calling logging inside logging.
Not logging in configuration is not an option, at least without introducing
complicated mechanism such as delayed logging, because we need to log at least
errors when reading the configuration.

We must also log in configuration loading only after the configuration is loaded
and available (or there is an error in loading the configuration).  Otherwise
logging would see an empty configuration and wouldn't be configured as asked.

Preventing logging before configuration is loaded is ensured by asking for
configuration in CameraManager constructor and in IPAManager constructor (which
is mysteriously loaded from CameraManager constructor before its body is
executed).  This seems to be sufficient at least for software ISP.

The configuration snippet for logging looks as follows:

  configuration:
    log:
      levels: LIBCAMERA_LOG_LEVELS

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/libcamera/base/global_configuration.cpp | 11 ++++++++++-
 src/libcamera/base/log.cpp                  | 12 ++++++++++--
 src/libcamera/camera_manager.cpp            |  9 +++++++++
 src/libcamera/ipa_manager.cpp               | 12 ++++++++++++
 4 files changed, 41 insertions(+), 3 deletions(-)

Patch
diff mbox series

diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp
index 72c3ce26..e9c44057 100644
--- a/src/libcamera/base/global_configuration.cpp
+++ b/src/libcamera/base/global_configuration.cpp
@@ -41,6 +41,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;
@@ -71,10 +76,15 @@  void GlobalConfiguration::load()
 
 bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName)
 {
+	/*
+	 * initialized_ must be set to true before any logging is called!
+	 */
+
 	File file(fileName);
 	if (!file.exists()) {
 		return false;
 	}
+	initialized_ = true;
 
 	if (!file.open(File::OpenModeFlag::ReadOnly)) {
 		LOG(Configuration, Warning)
@@ -89,7 +99,6 @@  bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName)
 		return true;
 	}
 	configuration_ = std::move(root);
-	initialized_ = true;
 	LOG(Configuration, Info) << "Configuration file " << fileName << "loaded";
 
 	return true;
diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
index c8045ef7..0564306d 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/camera_manager.cpp b/src/libcamera/camera_manager.cpp
index 355f3ada..cff8e80c 100644
--- a/src/libcamera/camera_manager.cpp
+++ b/src/libcamera/camera_manager.cpp
@@ -15,6 +15,7 @@ 
 
 #include "libcamera/internal/camera.h"
 #include "libcamera/internal/device_enumerator.h"
+#include "libcamera/internal/global_configuration.h"
 #include "libcamera/internal/pipeline_handler.h"
 
 /**
@@ -37,6 +38,14 @@  LOG_DEFINE_CATEGORY(Camera)
 CameraManager::Private::Private()
 	: initialized_(false)
 {
+	/*
+	 * This is needed to initialize global configuration
+	 * before any logging is called.  Actually, IPAManager
+	 * constructor is called before this, so the configuration
+	 * call is superfluous here, but the initialization belongs
+	 * here.
+	 */
+	GlobalConfiguration::configuration();
 }
 
 int CameraManager::Private::start()
diff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp
index 7a4515d9..5104b930 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"
@@ -105,6 +106,17 @@  IPAManager *IPAManager::self_ = nullptr;
  */
 IPAManager::IPAManager()
 {
+	/*
+	 * Make sure configuration is loaded early.
+	 * Otherwise there would be problems with mutual
+	 * configuration-logging dependencies.
+	 *
+	 * This is needed to be here in addition to CameraManager constructor
+	 * because logging is called before CameraManager constructor body is
+	 * executed.
+	 */
+	GlobalConfiguration::configuration();
+
 	if (self_)
 		LOG(IPAManager, Fatal)
 			<< "Multiple IPAManager objects are not allowed";