From patchwork Tue Feb 25 17:36:16 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= X-Patchwork-Id: 22877 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 4C65DBDB1C for ; Tue, 25 Feb 2025 17:36:25 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 9F4BD6873C; Tue, 25 Feb 2025 18:36:24 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=protonmail.com header.i=@protonmail.com header.b="vlD/o1pp"; dkim-atps=neutral Received: from mail-4316.protonmail.ch (mail-4316.protonmail.ch [185.70.43.16]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id BBBAB686D6 for ; Tue, 25 Feb 2025 18:36:22 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com; s=protonmail3; t=1740504982; x=1740764182; bh=3o/9e0VmUVhmnmrr6PgXseKVF982IpuzpXNd86t5QDw=; h=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References: Feedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID: Message-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post; b=vlD/o1pp+McEMLxQ5WuIl1otJLFrCBbrAArAY0s5NY3FXJs6PB/cyyqFEHdsI9IsS 2WlCwHRGe9DO/Rnmw4Nkvtu4uyKyXG9gPqQ1DxXvTmPysRMaeY052j3EIOMeXkEits PoFi9BPHWfvcHH2vODDozDA/FhbPjk6Yfair40qVntD0fwvCru6Ld//83UP+O6j+em 6f1YibYN51vyqfXLLUuckAcaUPkdK+76ZLytmQSyI/CJ8Ub1ST14EjAtL/zgdsjvuN sWCL2IKehZJjYOMGNYEZbxr6Tla4RtopYz0nPv6WV1g4jR43M1iQ3gDtywY7MxBF3+ SMLefFjV0ntHw== Date: Tue, 25 Feb 2025 17:36:16 +0000 To: libcamera-devel@lists.libcamera.org From: =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= Cc: Jacopo Mondi , Laurent Pinchart Subject: [PATCH v4 7/8] libcamera: base: log: Protect log categories with lock Message-ID: <20250225173531.2595922-8-pobrn@protonmail.com> In-Reply-To: <20250225173531.2595922-1-pobrn@protonmail.com> References: <20250225173531.2595922-1-pobrn@protonmail.com> Feedback-ID: 20568564:user:proton X-Pm-Message-ID: 6e7f7f3c8d198df5efde20fe48d732c3cb9c68aa MIME-Version: 1.0 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" Log categories may be added from any thread, so it is important to synchronize access to the `Logger::categories_` list between its two users: category creation (by LogCategory::create(), which calls Logger::findCategory() and Logger::registerCategory()); and log level setting (by Logger::logSetLevel()). The LogCategory::create() function uses a mutex to serialize category creation, but Logger::logSetLevel() can access `Logger::categories_` concurrently without any protection. To fix the issue, move the mutex to the Logger class, and use it to protect all accesses to the categories list. This requires moving all the logic of LogCategory::create() to a new Logger::findOrCreateCategory() function that combines both Logger::findCategory() and Logger::registerCategory() in order to make the two operations exacute atomically. Signed-off-by: Barnabás Pőcze Reviewed-by: Jacopo Mondi Reviewed-by: Laurent Pinchart --- include/libcamera/base/log.h | 1 + src/libcamera/base/log.cpp | 58 +++++++++++++----------------------- 2 files changed, 22 insertions(+), 37 deletions(-) diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h index 1fb92603f..8af74b59d 100644 --- a/include/libcamera/base/log.h +++ b/include/libcamera/base/log.h @@ -39,6 +39,7 @@ public: static const LogCategory &defaultCategory(); private: + friend class Logger; explicit LogCategory(std::string_view name); const std::string name_; diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp index 6687deb93..155355f0c 100644 --- a/src/libcamera/base/log.cpp +++ b/src/libcamera/base/log.cpp @@ -317,12 +317,12 @@ private: static LogSeverity parseLogLevel(std::string_view level); friend LogCategory; - void registerCategory(LogCategory *category); - LogCategory *findCategory(std::string_view name) const; + LogCategory *findOrCreateCategory(std::string_view name); static bool destroyed_; - std::vector categories_; + Mutex mutex_; + std::vector categories_ LIBCAMERA_TSA_GUARDED_BY(mutex_); std::list> levels_; std::shared_ptr output_; @@ -572,6 +572,8 @@ void Logger::logSetLevel(const char *category, const char *level) if (severity == LogInvalid) return; + MutexLocker locker(mutex_); + for (LogCategory *c : categories_) { if (c->name() == category) { c->setSeverity(severity); @@ -708,37 +710,28 @@ LogSeverity Logger::parseLogLevel(std::string_view level) } /** - * \brief Register a log category with the logger - * \param[in] category The log category - * - * Log categories must have unique names. It is invalid to call this function - * if a log category with the same name already exists. + * \brief Find an existing log category with the given name or create one + * \param[in] name Name of the log category + * \return The pointer to the log category found or created */ -void Logger::registerCategory(LogCategory *category) +LogCategory *Logger::findOrCreateCategory(std::string_view name) { - categories_.push_back(category); + MutexLocker locker(mutex_); - const std::string &name = category->name(); - for (const auto &[pattern, severity] : levels_) { - if (fnmatch(pattern.c_str(), name.c_str(), FNM_NOESCAPE) == 0) - category->setSeverity(severity); + for (LogCategory *category : categories_) { + if (category->name() == name) + return category; } -} -/** - * \brief Find an existing log category with the given name - * \param[in] name Name of the log category - * \return The pointer to the found log category or nullptr if not found - */ -LogCategory *Logger::findCategory(std::string_view name) const -{ - if (auto it = std::find_if(categories_.begin(), categories_.end(), - [name](auto c) { return c->name() == name; }); - it != categories_.end()) { - return *it; + LogCategory *category = categories_.emplace_back(new LogCategory(name)); + const char *categoryName = category->name().c_str(); + + for (const auto &[pattern, severity] : levels_) { + if (fnmatch(pattern.c_str(), categoryName, FNM_NOESCAPE) == 0) + category->setSeverity(severity); } - return nullptr; + return category; } /** @@ -776,16 +769,7 @@ LogCategory *Logger::findCategory(std::string_view name) const */ LogCategory *LogCategory::create(std::string_view name) { - static Mutex mutex_; - MutexLocker locker(mutex_); - LogCategory *category = Logger::instance()->findCategory(name); - - if (!category) { - category = new LogCategory(name); - Logger::instance()->registerCategory(category); - } - - return category; + return Logger::instance()->findOrCreateCategory(name); } /**