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

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

Commit Message

Milan Zamazal April 23, 2024, 10:30 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 loaded.  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 prevent logging in configuration loading before 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 | 18 +++++++++++++++++-
 src/libcamera/base/log.cpp                  | 12 ++++++++++--
 src/libcamera/camera_manager.cpp            |  9 +++++++++
 src/libcamera/ipa_manager.cpp               | 12 ++++++++++++
 4 files changed, 48 insertions(+), 3 deletions(-)

Patch
diff mbox series

diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp
index 96088b36..b45fd341 100644
--- a/src/libcamera/base/global_configuration.cpp
+++ b/src/libcamera/base/global_configuration.cpp
@@ -29,6 +29,13 @@  LOG_DEFINE_CATEGORY(Configuration)
  * 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 +64,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;
@@ -87,10 +99,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)
@@ -105,7 +122,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";