From patchwork Tue Oct 1 10:27:56 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Milan Zamazal X-Patchwork-Id: 21451 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id D48DDBD80A for ; Tue, 1 Oct 2024 10:28:36 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 6A7086352B; Tue, 1 Oct 2024 12:28:36 +0200 (CEST) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.b="gEQQ9M17"; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id E278C6352C for ; Tue, 1 Oct 2024 12:28:29 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1727778509; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=sHNHnO/rAThzWzruTpKDdGLqUkZDPYTMt4wIO/bqGd0=; b=gEQQ9M17u8TN74My6UuVA5nl/aAhcDIZP1YhT2PGA5qzMNLIeIfD6uB29A+CiYWUNVkcQz arJvfF4e694DkgU9eYqtc+Xnv8Ro+DxelE5OaYNEZ9ILgK0udTAEZcjlk+Aqcwz3MQ6v4x MnlCrTaIYYRehbGO7xGkqlesg65sU3Q= Received: from mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-257-VW3pk_K-PDKJ8vrivBIMlQ-1; Tue, 01 Oct 2024 06:28:25 -0400 X-MC-Unique: VW3pk_K-PDKJ8vrivBIMlQ-1 Received: from mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (unknown [10.30.177.40]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-03.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id E442B192DE2C; Tue, 1 Oct 2024 10:28:24 +0000 (UTC) Received: from nuthatch.brq.redhat.com (unknown [10.43.17.37]) by mx-prod-int-04.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id A694B1956054; Tue, 1 Oct 2024 10:28:23 +0000 (UTC) From: Milan Zamazal To: libcamera-devel@lists.libcamera.org Cc: Milan Zamazal , Kieran Bingham , Naushir Patuck Subject: [PATCH v5 04/15] config: Look up logging levels in the configuration file Date: Tue, 1 Oct 2024 12:27:56 +0200 Message-ID: <20241001102810.479285-5-mzamazal@redhat.com> In-Reply-To: <20241001102810.479285-1-mzamazal@redhat.com> References: <20241001102810.479285-1-mzamazal@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.0 on 10.30.177.40 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" 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 --- .../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(-) 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::instance_ = std::make_unique(); /** - * \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()) { @@ -68,6 +86,11 @@ const std::vector 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 #include #include +#include #include #include #include @@ -25,6 +26,8 @@ #include #include +#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 confDebug = + GlobalConfiguration::configuration()["log"]["levels"].get(); + 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 #include +#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";