[{"id":33166,"web_url":"https://patchwork.libcamera.org/comment/33166/","msgid":"<20250124193900.GH13689@pendragon.ideasonboard.com>","date":"2025-01-24T19:39:00","subject":"Re: [RFC PATCH v1 7/7] libcamera: base: log: Protect log categories\n\twith lock","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Tue, Jan 21, 2025 at 06:56:16PM +0000, Barnabás Pőcze wrote:\n> Log categories may be added from any thread, so it is important\n> to synchronize access to the `Logger::categories_` list between\n> its two users: `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`.\n> \n> To achieve that, `Logger::{find,register}Category()` are merged into\n> a single function: `Logger::findOrCreateCategory()`; and the mutex\n> from `LogCategory::create()` is moved into the `Logger` class.\n> \n> Furthermore, appropriate `MutexLocker`s are placed in\n> `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`.\n> \n> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>\n> ---\n>  include/libcamera/base/log.h |   2 +-\n>  src/libcamera/base/log.cpp   | 235 ++++++++++++++++-------------------\n>  2 files changed, 110 insertions(+), 127 deletions(-)\n> \n> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h\n> index ef161bece..58e873fa9 100644\n> --- a/include/libcamera/base/log.h\n> +++ b/include/libcamera/base/log.h\n> @@ -30,6 +30,7 @@ enum LogSeverity {\n>  class LogCategory\n>  {\n>  public:\n> +\texplicit LogCategory(std::string_view name);\n>  \tstatic LogCategory *create(std::string_view name);\n>  \n>  \tconst std::string &name() const { return name_; }\n> @@ -39,7 +40,6 @@ public:\n>  \tstatic const LogCategory &defaultCategory();\n>  \n>  private:\n> -\texplicit LogCategory(std::string_view name);\n>  \n>  \tconst std::string name_;\n>  \n> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp\n> index 6430650ec..d2de5a80e 100644\n> --- a/src/libcamera/base/log.cpp\n> +++ b/src/libcamera/base/log.cpp\n> @@ -66,7 +66,9 @@\n>  \n>  namespace libcamera {\n>  \n> -static int log_severity_to_syslog(LogSeverity severity)\n> +namespace {\n> +\n> +int log_severity_to_syslog(LogSeverity severity)\n>  {\n>  \tswitch (severity) {\n>  \tcase LogDebug:\n> @@ -84,7 +86,7 @@ static int log_severity_to_syslog(LogSeverity severity)\n>  \t}\n>  }\n>  \n> -static const char *log_severity_name(LogSeverity severity)\n> +const char *log_severity_name(LogSeverity severity)\n>  {\n>  \tstatic const char *const names[] = {\n>  \t\t\"DEBUG\",\n> @@ -100,6 +102,92 @@ static const char *log_severity_name(LogSeverity severity)\n>  \t\treturn \"UNKWN\";\n>  }\n>  \n> +/**\n> + * \\brief Parse a log level string into a LogSeverity\n> + * \\param[in] level The log level string\n> + *\n> + * Log levels can be specified as an integer value in the range from LogDebug to\n> + * LogFatal, or as a string corresponding to the severity name in uppercase. Any\n> + * other value is invalid.\n> + *\n> + * \\return The log severity, or LogInvalid if the string is invalid\n> + */\n> +LogSeverity parseLogLevel(std::string_view level)\n> +{\n> +\tstatic const char *const names[] = {\n> +\t\t\"DEBUG\",\n> +\t\t\"INFO\",\n> +\t\t\"WARN\",\n> +\t\t\"ERROR\",\n> +\t\t\"FATAL\",\n> +\t};\n> +\n> +\tunsigned int severity;\n> +\n> +\tif (std::isdigit(level[0])) {\n> +\t\tauto [end, ec] = std::from_chars(level.data(), level.data() + level.size(), severity, 10);\n> +\t\tif (ec != std::errc() || *end != '\\0' || severity > LogFatal)\n> +\t\t\tseverity = LogInvalid;\n> +\t} else {\n> +\t\tseverity = LogInvalid;\n> +\t\tfor (unsigned int i = 0; i < std::size(names); ++i) {\n> +\t\t\tif (names[i] == level) {\n> +\t\t\t\tseverity = i;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t}\n> +\n> +\treturn static_cast<LogSeverity>(severity);\n> +}\n> +\n> +/**\n> + * \\brief Parse the log levels from the environment\n> + *\n> + * The log levels are stored in the LIBCAMERA_LOG_LEVELS environment variable\n> + * as a list of \"category:level\" pairs, separated by commas (','). Parse the\n> + * variable and store the levels to configure all log categories.\n> + */\n> +void parseLogLevels(const char *line, std::list<std::pair<std::string, LogSeverity>> &levels)\n> +{\n> +\tfor (const char *pair = line; *line != '\\0'; pair = line) {\n> +\t\tconst char *comma = strchrnul(line, ',');\n> +\t\tsize_t len = comma - pair;\n> +\n> +\t\t/* Skip over the comma. */\n> +\t\tline = *comma == ',' ? comma + 1 : comma;\n> +\n> +\t\t/* Skip to the next pair if the pair is empty. */\n> +\t\tif (!len)\n> +\t\t\tcontinue;\n> +\n> +\t\tstd::string_view category;\n> +\t\tstd::string_view level;\n> +\n> +\t\tconst char *colon = static_cast<const char *>(memchr(pair, ':', len));\n> +\t\tif (!colon) {\n> +\t\t\t/* 'x' is a shortcut for '*:x'. */\n> +\t\t\tcategory = \"*\";\n> +\t\t\tlevel = std::string_view(pair, len);\n> +\t\t} else {\n> +\t\t\tcategory = std::string_view(pair, colon - pair);\n> +\t\t\tlevel = std::string_view(colon + 1, comma - colon - 1);\n> +\t\t}\n> +\n> +\t\t/* Both the category and the level must be specified. */\n> +\t\tif (category.empty() || level.empty())\n> +\t\t\tcontinue;\n> +\n> +\t\tLogSeverity severity = parseLogLevel(level);\n> +\t\tif (severity == LogInvalid)\n> +\t\t\tcontinue;\n> +\n> +\t\tlevels.emplace_back(category, severity);\n> +\t}\n> +}\n> +\n> +} /* namespace */\n> +\n>  /**\n>   * \\brief Log output\n>   *\n> @@ -313,15 +401,13 @@ private:\n>  \tLogger();\n>  \n>  \tvoid parseLogFile();\n> -\tvoid parseLogLevels();\n> -\tstatic LogSeverity parseLogLevel(std::string_view level);\n\nWhy do you turn those functions into global functions ? That's not\nexplained in the commit message, and I don't see the need. It makes the\npatch harder to review, and should be split to a separate patch if you\nwant to keep this.\n\n>  \n>  \tfriend LogCategory;\n> -\tvoid registerCategory(LogCategory *category);\n> -\tLogCategory *findCategory(std::string_view name) const;\n> +\tLogCategory *findOrCreateCategory(std::string_view name);\n>  \n>  \tstatic bool destroyed_;\n>  \n> +\tMutex mutex_;\n>  \tstd::vector<LogCategory *> categories_;\n>  \tstd::list<std::pair<std::string, LogSeverity>> levels_;\n>  \n> @@ -572,6 +658,8 @@ void Logger::logSetLevel(const char *category, const char *level)\n>  \tif (severity == LogInvalid)\n>  \t\treturn;\n>  \n> +\tMutexLocker locker(mutex_);\n> +\n>  \tfor (LogCategory *c : categories_) {\n>  \t\tif (c->name() == category) {\n>  \t\t\tc->setSeverity(severity);\n> @@ -593,7 +681,9 @@ Logger::Logger()\n>  \tlogSetStream(&std::cerr, color);\n>  \n>  \tparseLogFile();\n> -\tparseLogLevels();\n> +\n> +\tif (const char *debug = utils::secure_getenv(\"LIBCAMERA_LOG_LEVELS\"))\n> +\t\tparseLogLevels(debug, levels_);\n>  }\n>  \n>  /**\n> @@ -620,105 +710,21 @@ void Logger::parseLogFile()\n>  }\n>  \n>  /**\n> - * \\brief Parse the log levels from the environment\n> - *\n> - * The log levels are stored in the LIBCAMERA_LOG_LEVELS environment variable\n> - * as a list of \"category:level\" pairs, separated by commas (','). Parse the\n> - * variable and store the levels to configure all log categories.\n> - */\n> -void Logger::parseLogLevels()\n> -{\n> -\tconst char *debug = utils::secure_getenv(\"LIBCAMERA_LOG_LEVELS\");\n> -\tif (!debug)\n> -\t\treturn;\n> -\n> -\tfor (const char *pair = debug; *debug != '\\0'; pair = debug) {\n> -\t\tconst char *comma = strchrnul(debug, ',');\n> -\t\tsize_t len = comma - pair;\n> -\n> -\t\t/* Skip over the comma. */\n> -\t\tdebug = *comma == ',' ? comma + 1 : comma;\n> -\n> -\t\t/* Skip to the next pair if the pair is empty. */\n> -\t\tif (!len)\n> -\t\t\tcontinue;\n> -\n> -\t\tstd::string_view category;\n> -\t\tstd::string_view level;\n> -\n> -\t\tconst char *colon = static_cast<const char *>(memchr(pair, ':', len));\n> -\t\tif (!colon) {\n> -\t\t\t/* 'x' is a shortcut for '*:x'. */\n> -\t\t\tcategory = \"*\";\n> -\t\t\tlevel = std::string_view(pair, len);\n> -\t\t} else {\n> -\t\t\tcategory = std::string_view(pair, colon - pair);\n> -\t\t\tlevel = std::string_view(colon + 1, comma - colon - 1);\n> -\t\t}\n> -\n> -\t\t/* Both the category and the level must be specified. */\n> -\t\tif (category.empty() || level.empty())\n> -\t\t\tcontinue;\n> -\n> -\t\tLogSeverity severity = parseLogLevel(level);\n> -\t\tif (severity == LogInvalid)\n> -\t\t\tcontinue;\n> -\n> -\t\tlevels_.emplace_back(category, severity);\n> -\t}\n> -}\n> -\n> -/**\n> - * \\brief Parse a log level string into a LogSeverity\n> - * \\param[in] level The log level string\n> - *\n> - * Log levels can be specified as an integer value in the range from LogDebug to\n> - * LogFatal, or as a string corresponding to the severity name in uppercase. Any\n> - * other value is invalid.\n> - *\n> - * \\return The log severity, or LogInvalid if the string is invalid\n> + * \\brief Find an existing log category with the given name or create one\n> + * \\param[in] name Name of the log category\n> + * \\return The pointer to the log category found or created\n>   */\n> -LogSeverity Logger::parseLogLevel(std::string_view level)\n> +LogCategory *Logger::findOrCreateCategory(std::string_view name)\n>  {\n> -\tstatic const char *const names[] = {\n> -\t\t\"DEBUG\",\n> -\t\t\"INFO\",\n> -\t\t\"WARN\",\n> -\t\t\"ERROR\",\n> -\t\t\"FATAL\",\n> -\t};\n> -\n> -\tunsigned int severity;\n> +\tMutexLocker locker(mutex_);\n>  \n> -\tif (std::isdigit(level[0])) {\n> -\t\tauto [end, ec] = std::from_chars(level.data(), level.data() + level.size(), severity, 10);\n> -\t\tif (ec != std::errc() || *end != '\\0' || severity > LogFatal)\n> -\t\t\tseverity = LogInvalid;\n> -\t} else {\n> -\t\tseverity = LogInvalid;\n> -\t\tfor (unsigned int i = 0; i < std::size(names); ++i) {\n> -\t\t\tif (names[i] == level) {\n> -\t\t\t\tseverity = i;\n> -\t\t\t\tbreak;\n> -\t\t\t}\n> -\t\t}\n> +\tfor (LogCategory *c : categories_) {\n> +\t\tif (c->name() == name)\n> +\t\t\treturn c;\n>  \t}\n>  \n> -\treturn static_cast<LogSeverity>(severity);\n> -}\n> -\n> -/**\n> - * \\brief Register a log category with the logger\n> - * \\param[in] category The log category\n> - *\n> - * Log categories must have unique names. It is invalid to call this function\n> - * if a log category with the same name already exists.\n> - */\n> -void Logger::registerCategory(LogCategory *category)\n> -{\n> -\tcategories_.push_back(category);\n> +\tLogCategory *c = categories_.emplace_back(new LogCategory(name));\n>  \n> -\tconst std::string &name = category->name();\n>  \tfor (const std::pair<std::string, LogSeverity> &level : levels_) {\n>  \t\tbool match = true;\n>  \n> @@ -734,26 +740,12 @@ void Logger::registerCategory(LogCategory *category)\n>  \t\t}\n>  \n>  \t\tif (match) {\n> -\t\t\tcategory->setSeverity(level.second);\n> +\t\t\tc->setSeverity(level.second);\n>  \t\t\tbreak;\n>  \t\t}\n>  \t}\n> -}\n>  \n> -/**\n> - * \\brief Find an existing log category with the given name\n> - * \\param[in] name Name of the log category\n> - * \\return The pointer to the found log category or nullptr if not found\n> - */\n> -LogCategory *Logger::findCategory(std::string_view name) const\n> -{\n> -\tif (auto it = std::find_if(categories_.begin(), categories_.end(),\n> -\t\t\t\t   [name](auto c) { return c->name() == name; });\n> -\t    it != categories_.end()) {\n> -\t\treturn *it;\n> -\t}\n> -\n> -\treturn nullptr;\n> +\treturn c;\n>  }\n>  \n>  /**\n> @@ -791,16 +783,7 @@ LogCategory *Logger::findCategory(std::string_view name) const\n>   */\n>  LogCategory *LogCategory::create(std::string_view name)\n>  {\n> -\tstatic Mutex mutex_;\n> -\tMutexLocker locker(mutex_);\n> -\tLogCategory *category = Logger::instance()->findCategory(name);\n> -\n> -\tif (!category) {\n> -\t\tcategory = new LogCategory(name);\n> -\t\tLogger::instance()->registerCategory(category);\n> -\t}\n> -\n> -\treturn category;\n> +\treturn Logger::instance()->findOrCreateCategory(name);\n>  }\n>  \n>  /**","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 E85C1C31E9\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 24 Jan 2025 19:39:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E03026855D;\n\tFri, 24 Jan 2025 20:39:12 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6AB7C68556\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 24 Jan 2025 20:39:11 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AD998564;\n\tFri, 24 Jan 2025 20:38:06 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IHSdyZJH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737747486;\n\tbh=LH4nPorzjlCy7uCO2zcflwp4jbZ89ugQyHBlMREbv3Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IHSdyZJHxbqp0fQpm0+AvGTE7ZWu1RoU6gdxjCozl0sTtI/A+OiHPWlOptcCcxwQ1\n\tD/3lgtv0lko5vIJqYZPkf9XpQ/PT+OwP2vPCQdcG091X7kSe/jltyufA8sAha4a32F\n\tPPr/M0iF0WF2F9Ldd31whVUqm8iERH6ZgeETQctk=","Date":"Fri, 24 Jan 2025 21:39:00 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= <pobrn@protonmail.com>","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 7/7] libcamera: base: log: Protect log categories\n\twith lock","Message-ID":"<20250124193900.GH13689@pendragon.ideasonboard.com>","References":"<20250121185554.301901-1-pobrn@protonmail.com>\n\t<20250121185554.301901-5-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250121185554.301901-5-pobrn@protonmail.com>","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>"}}]