Patch Detail
Show a patch.
GET /api/patches/21357/?format=api
{ "id": 21357, "url": "https://patchwork.libcamera.org/api/patches/21357/?format=api", "web_url": "https://patchwork.libcamera.org/patch/21357/", "project": { "id": 1, "url": "https://patchwork.libcamera.org/api/projects/1/?format=api", "name": "libcamera", "link_name": "libcamera", "list_id": "libcamera_core", "list_email": "libcamera-devel@lists.libcamera.org", "web_url": "", "scm_url": "", "webscm_url": "" }, "msgid": "<20240925095850.348259-5-mzamazal@redhat.com>", "date": "2024-09-25T09:58:36", "name": "[v4,04/15] config: Look up logging levels in the configuration file", "commit_ref": null, "pull_url": null, "state": "superseded", "archived": false, "hash": "d8ee79fc1a3e9fa83eebc4d6309d96f77b191560", "submitter": { "id": 177, "url": "https://patchwork.libcamera.org/api/people/177/?format=api", "name": "Milan Zamazal", "email": "mzamazal@redhat.com" }, "delegate": null, "mbox": "https://patchwork.libcamera.org/patch/21357/mbox/", "series": [ { "id": 4623, "url": "https://patchwork.libcamera.org/api/series/4623/?format=api", "web_url": "https://patchwork.libcamera.org/project/libcamera/list/?series=4623", "date": "2024-09-25T09:58:32", "name": "Add global configuration file", "version": 4, "mbox": "https://patchwork.libcamera.org/series/4623/mbox/" } ], "comments": "https://patchwork.libcamera.org/api/patches/21357/comments/", "check": "pending", "checks": "https://patchwork.libcamera.org/api/patches/21357/checks/", "tags": {}, "headers": { "Return-Path": "<libcamera-devel-bounces@lists.libcamera.org>", "X-Original-To": "parsemail@patchwork.libcamera.org", "Delivered-To": "parsemail@patchwork.libcamera.org", "Received": [ "from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1A7D1C0F1B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed, 25 Sep 2024 09:59:25 +0000 (UTC)", "from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B08596351A;\n\tWed, 25 Sep 2024 11:59:24 +0200 (CEST)", "from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5B8BB6350F\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 25 Sep 2024 11:59:17 +0200 (CEST)", "from mx-prod-mc-02.mail-002.prod.us-west-2.aws.redhat.com\n\t(ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63])\n\tby relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3,\n\tcipher=TLS_AES_256_GCM_SHA384) id us-mta-615-PDR1vX4kN0q37U4XGklnYQ-1;\n\tWed, 25 Sep 2024 05:59:12 -0400", "from mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com (unknown\n\t[10.30.177.12])\n\t(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)\n\tkey-exchange X25519 server-signature RSA-PSS (2048 bits)\n\tserver-digest SHA256) (No client certificate requested)\n\tby mx-prod-mc-02.mail-002.prod.us-west-2.aws.redhat.com (Postfix)\n\twith ESMTPS id F3FA91944DE8; Wed, 25 Sep 2024 09:59:11 +0000 (UTC)", "from nuthatch.redhat.com (unknown [10.45.225.29])\n\tby mx-prod-int-03.mail-002.prod.us-west-2.aws.redhat.com (Postfix)\n\twith ESMTP id 4328A19560AA; Wed, 25 Sep 2024 09:59:10 +0000 (UTC)" ], "Authentication-Results": "lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"R2Yq9cQf\"; dkim-atps=neutral", "DKIM-Signature": "v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1727258356;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=wn83MlOCErh/QBfl++HFhNabY01UXCT1nzqZZzQVG5s=;\n\tb=R2Yq9cQfprnv8U4MTD3Ycgr/5+rAmVD3l1A/FC513XnvCOLSy7fLA9mdmVA2wtIZfQ+tsQ\n\tr0vl/Fy1f76NFDWiXKcoCvVcm+B9QKCtyY/Esyad7nHI2XiwpkgEDs2Jsc8g2NMb6Nwa7T\n\txIqOy1DoD7pBfJxuxA8GfMm1a4v2ndA=", "X-MC-Unique": "PDR1vX4kN0q37U4XGklnYQ-1", "From": "Milan Zamazal <mzamazal@redhat.com>", "To": "libcamera-devel@lists.libcamera.org", "Cc": "Milan Zamazal <mzamazal@redhat.com>,\n\tKieran Bingham <kieran.bingham@ideasonboard.com>,\n\tNaushir Patuck <naush@raspberrypi.com>", "Subject": "[PATCH v4 04/15] config: Look up logging levels in the configuration\n\tfile", "Date": "Wed, 25 Sep 2024 11:58:36 +0200", "Message-ID": "<20240925095850.348259-5-mzamazal@redhat.com>", "In-Reply-To": "<20240925095850.348259-1-mzamazal@redhat.com>", "References": "<20240925095850.348259-1-mzamazal@redhat.com>", "MIME-Version": "1.0", "X-Scanned-By": "MIMEDefang 3.0 on 10.30.177.12", "X-Mimecast-Spam-Score": "0", "X-Mimecast-Originator": "redhat.com", "Content-Transfer-Encoding": "8bit", "Content-Type": "text/plain; charset=\"US-ASCII\"; x-default=true", "X-BeenThere": "libcamera-devel@lists.libcamera.org", "X-Mailman-Version": "2.1.29", "Precedence": "list", "List-Id": "<libcamera-devel.lists.libcamera.org>", "List-Unsubscribe": "<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>", "List-Archive": "<https://lists.libcamera.org/pipermail/libcamera-devel/>", "List-Post": "<mailto:libcamera-devel@lists.libcamera.org>", "List-Help": "<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>", "List-Subscribe": "<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>", "Errors-To": "libcamera-devel-bounces@lists.libcamera.org", "Sender": "\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>" }, "content": "In addition to LIBCAMERA_LOG_LEVELS environment variable, expose the\nsame configuration in the configuration file.\n\nBecause logging asks for configuration and configuration loading uses\nlogging, extra care is needed. We must prevent calling any log function\nbefore configuration is loaded. Otherwise the log function would ask\nfor configuration, it would load the configuration files and would try\nto log the results, which would cause mutex deadlock due to calling\nlogging inside logging. Not logging in configuration is not an option,\nat least without introducing complicated mechanism such as delayed\nlogging, because we need to log at least errors when reading the\nconfiguration.\n\nWe must also prevent logging in configuration loading before the\nconfiguration is loaded and available (or there is an error in loading\nthe configuration). Otherwise logging would see an empty configuration\nand wouldn't be configured as asked.\n\nPreventing logging before configuration is loaded is ensured by asking\nfor configuration in CameraManager constructor and in IPAManager\nconstructor (which is mysteriously loaded from CameraManager constructor\nbefore its body is executed). This seems to be sufficient at least for\nsoftware ISP.\n\nThe configuration snippet for logging looks as follows:\n\n configuration:\n log:\n levels: LIBCAMERA_LOG_LEVELS\n\nSigned-off-by: Milan Zamazal <mzamazal@redhat.com>\n---\n src/libcamera/base/global_configuration.cpp | 18 +++++++++++++++++-\n src/libcamera/base/log.cpp | 12 ++++++++++--\n src/libcamera/camera_manager.cpp | 10 ++++++++++\n src/libcamera/ipa_manager.cpp | 12 ++++++++++++\n 4 files changed, 49 insertions(+), 3 deletions(-)", "diff": "diff --git a/src/libcamera/base/global_configuration.cpp b/src/libcamera/base/global_configuration.cpp\nindex 96088b36e..b45fd3419 100644\n--- a/src/libcamera/base/global_configuration.cpp\n+++ b/src/libcamera/base/global_configuration.cpp\n@@ -29,6 +29,13 @@ LOG_DEFINE_CATEGORY(Configuration)\n * The configuration file is a YAML file and the configuration itself is stored\n * under `configuration' top-level item.\n *\n+ * Example configuration file content:\n+ * \\code{.yaml}\n+ * configuration:\n+ * log:\n+ * levels: 'IPAManager:DEBUG'\n+ * \\endcode\n+ *\n * The configuration file is looked up in user's home directory first and if it\n * is not found then in system-wide configuration directories. If multiple\n * configuration files exist then only the first one found is used and no\n@@ -57,6 +64,11 @@ const std::vector<std::filesystem::path>\n \t\tstd::filesystem::path(\"/etc/libcamera/configuration.yaml\"),\n \t};\n \n+/*\n+ * Care is needed here to not log anything before the configuration is\n+ * loaded otherwise the logger would be initialized with empty configuration.\n+ */\n+\n void GlobalConfiguration::load()\n {\n \tstd::filesystem::path userConfigurationDirectory;\n@@ -87,10 +99,15 @@ void GlobalConfiguration::load()\n \n bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName)\n {\n+\t/*\n+\t * initialized_ must be set to true before any logging is called!\n+\t */\n+\n \tFile file(fileName);\n \tif (!file.exists()) {\n \t\treturn false;\n \t}\n+\tinitialized_ = true;\n \n \tif (!file.open(File::OpenModeFlag::ReadOnly)) {\n \t\tLOG(Configuration, Warning)\n@@ -105,7 +122,6 @@ bool GlobalConfiguration::loadFile(const std::filesystem::path &fileName)\n \t\treturn true;\n \t}\n \tconfiguration_ = std::move(root);\n-\tinitialized_ = true;\n \tLOG(Configuration, Info) << \"Configuration file \" << fileName << \"loaded\";\n \n \treturn true;\ndiff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\nindex 3a656b8f0..0f6767107 100644\n--- a/src/libcamera/base/log.cpp\n+++ b/src/libcamera/base/log.cpp\n@@ -14,6 +14,7 @@\n #include <stdio.h>\n #include <stdlib.h>\n #include <string.h>\n+#include <string>\n #include <syslog.h>\n #include <time.h>\n #include <unordered_set>\n@@ -25,6 +26,8 @@\n #include <libcamera/base/thread.h>\n #include <libcamera/base/utils.h>\n \n+#include \"libcamera/internal/global_configuration.h\"\n+\n /**\n * \\file base/log.h\n * \\brief Logging infrastructure\n@@ -626,8 +629,13 @@ void Logger::parseLogFile()\n void Logger::parseLogLevels()\n {\n \tconst char *debug = utils::secure_getenv(\"LIBCAMERA_LOG_LEVELS\");\n-\tif (!debug)\n-\t\treturn;\n+\tif (!debug) {\n+\t\tconst std::optional<std::string> confDebug =\n+\t\t\tGlobalConfiguration::configuration()[\"log\"][\"levels\"].get<std::string>();\n+\t\tif (!confDebug.has_value())\n+\t\t\treturn;\n+\t\tdebug = confDebug.value().c_str();\n+\t}\n \n \tfor (const char *pair = debug; *debug != '\\0'; pair = debug) {\n \t\tconst char *comma = strchrnul(debug, ',');\ndiff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp\nindex 09f587765..c1093320c 100644\n--- a/src/libcamera/camera_manager.cpp\n+++ b/src/libcamera/camera_manager.cpp\n@@ -15,6 +15,7 @@\n \n #include \"libcamera/internal/camera.h\"\n #include \"libcamera/internal/device_enumerator.h\"\n+#include \"libcamera/internal/global_configuration.h\"\n #include \"libcamera/internal/ipa_manager.h\"\n #include \"libcamera/internal/pipeline_handler.h\"\n \n@@ -41,6 +42,15 @@ CameraManager::Private::Private()\n \t: initialized_(false)\n {\n \tipaManager_ = std::make_unique<IPAManager>();\n+\n+\t/*\n+\t * This is needed to initialize global configuration\n+\t * before any logging is called. Actually, IPAManager\n+\t * constructor is called before this, so the configuration\n+\t * call is superfluous here, but the initialization belongs\n+\t * here.\n+\t */\n+\tGlobalConfiguration::configuration();\n }\n \n int CameraManager::Private::start()\ndiff --git a/src/libcamera/ipa_manager.cpp b/src/libcamera/ipa_manager.cpp\nindex cfc24d389..28384844a 100644\n--- a/src/libcamera/ipa_manager.cpp\n+++ b/src/libcamera/ipa_manager.cpp\n@@ -16,6 +16,7 @@\n #include <libcamera/base/log.h>\n #include <libcamera/base/utils.h>\n \n+#include \"libcamera/internal/global_configuration.h\"\n #include \"libcamera/internal/ipa_module.h\"\n #include \"libcamera/internal/ipa_proxy.h\"\n #include \"libcamera/internal/pipeline_handler.h\"\n@@ -103,6 +104,17 @@ LOG_DEFINE_CATEGORY(IPAManager)\n */\n IPAManager::IPAManager()\n {\n+\t/*\n+\t * Make sure configuration is loaded early.\n+\t * Otherwise there would be problems with mutual\n+\t * configuration-logging dependencies.\n+\t *\n+\t * This is needed to be here in addition to CameraManager constructor\n+\t * because logging is called before CameraManager constructor body is\n+\t * executed.\n+\t */\n+\tGlobalConfiguration::configuration();\n+\n #if HAVE_IPA_PUBKEY\n \tif (!pubKey_.isValid())\n \t\tLOG(IPAManager, Warning) << \"Public key not valid\";\n", "prefixes": [ "v4", "04/15" ] }