[RFC PATCH v1 7/7] libcamera: base: log: Protect log categories with lock
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jan 24 20:39:00 CET 2025
Hi Barnabás,
Thank you for the patch.
On Tue, Jan 21, 2025 at 06:56:16PM +0000, Barnabás Pőcze wrote:
> Log categories may be added from any thread, so it is important
> to synchronize access to the `Logger::categories_` list between
> its two users: `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`.
>
> To achieve that, `Logger::{find,register}Category()` are merged into
> a single function: `Logger::findOrCreateCategory()`; and the mutex
> from `LogCategory::create()` is moved into the `Logger` class.
>
> Furthermore, appropriate `MutexLocker`s are placed in
> `Logger::findOrCreateCategory()` and `Logger::logSetLevel()`.
>
> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> ---
> include/libcamera/base/log.h | 2 +-
> src/libcamera/base/log.cpp | 235 ++++++++++++++++-------------------
> 2 files changed, 110 insertions(+), 127 deletions(-)
>
> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
> index ef161bece..58e873fa9 100644
> --- a/include/libcamera/base/log.h
> +++ b/include/libcamera/base/log.h
> @@ -30,6 +30,7 @@ enum LogSeverity {
> class LogCategory
> {
> public:
> + explicit LogCategory(std::string_view name);
> static LogCategory *create(std::string_view name);
>
> const std::string &name() const { return name_; }
> @@ -39,7 +40,6 @@ public:
> static const LogCategory &defaultCategory();
>
> private:
> - 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 6430650ec..d2de5a80e 100644
> --- a/src/libcamera/base/log.cpp
> +++ b/src/libcamera/base/log.cpp
> @@ -66,7 +66,9 @@
>
> namespace libcamera {
>
> -static int log_severity_to_syslog(LogSeverity severity)
> +namespace {
> +
> +int log_severity_to_syslog(LogSeverity severity)
> {
> switch (severity) {
> case LogDebug:
> @@ -84,7 +86,7 @@ static int log_severity_to_syslog(LogSeverity severity)
> }
> }
>
> -static const char *log_severity_name(LogSeverity severity)
> +const char *log_severity_name(LogSeverity severity)
> {
> static const char *const names[] = {
> "DEBUG",
> @@ -100,6 +102,92 @@ static const char *log_severity_name(LogSeverity severity)
> return "UNKWN";
> }
>
> +/**
> + * \brief Parse a log level string into a LogSeverity
> + * \param[in] level The log level string
> + *
> + * Log levels can be specified as an integer value in the range from LogDebug to
> + * LogFatal, or as a string corresponding to the severity name in uppercase. Any
> + * other value is invalid.
> + *
> + * \return The log severity, or LogInvalid if the string is invalid
> + */
> +LogSeverity parseLogLevel(std::string_view level)
> +{
> + static const char *const names[] = {
> + "DEBUG",
> + "INFO",
> + "WARN",
> + "ERROR",
> + "FATAL",
> + };
> +
> + unsigned int severity;
> +
> + if (std::isdigit(level[0])) {
> + auto [end, ec] = std::from_chars(level.data(), level.data() + level.size(), severity, 10);
> + if (ec != std::errc() || *end != '\0' || severity > LogFatal)
> + severity = LogInvalid;
> + } else {
> + severity = LogInvalid;
> + for (unsigned int i = 0; i < std::size(names); ++i) {
> + if (names[i] == level) {
> + severity = i;
> + break;
> + }
> + }
> + }
> +
> + return static_cast<LogSeverity>(severity);
> +}
> +
> +/**
> + * \brief Parse the log levels from the environment
> + *
> + * The log levels are stored in the LIBCAMERA_LOG_LEVELS environment variable
> + * as a list of "category:level" pairs, separated by commas (','). Parse the
> + * variable and store the levels to configure all log categories.
> + */
> +void parseLogLevels(const char *line, std::list<std::pair<std::string, LogSeverity>> &levels)
> +{
> + for (const char *pair = line; *line != '\0'; pair = line) {
> + const char *comma = strchrnul(line, ',');
> + size_t len = comma - pair;
> +
> + /* Skip over the comma. */
> + line = *comma == ',' ? comma + 1 : comma;
> +
> + /* Skip to the next pair if the pair is empty. */
> + if (!len)
> + continue;
> +
> + std::string_view category;
> + std::string_view level;
> +
> + const char *colon = static_cast<const char *>(memchr(pair, ':', len));
> + if (!colon) {
> + /* 'x' is a shortcut for '*:x'. */
> + category = "*";
> + level = std::string_view(pair, len);
> + } else {
> + category = std::string_view(pair, colon - pair);
> + level = std::string_view(colon + 1, comma - colon - 1);
> + }
> +
> + /* Both the category and the level must be specified. */
> + if (category.empty() || level.empty())
> + continue;
> +
> + LogSeverity severity = parseLogLevel(level);
> + if (severity == LogInvalid)
> + continue;
> +
> + levels.emplace_back(category, severity);
> + }
> +}
> +
> +} /* namespace */
> +
> /**
> * \brief Log output
> *
> @@ -313,15 +401,13 @@ private:
> Logger();
>
> void parseLogFile();
> - void parseLogLevels();
> - static LogSeverity parseLogLevel(std::string_view level);
Why do you turn those functions into global functions ? That's not
explained in the commit message, and I don't see the need. It makes the
patch harder to review, and should be split to a separate patch if you
want to keep this.
>
> friend LogCategory;
> - void registerCategory(LogCategory *category);
> - LogCategory *findCategory(std::string_view name) const;
> + LogCategory *findOrCreateCategory(std::string_view name);
>
> static bool destroyed_;
>
> + Mutex mutex_;
> std::vector<LogCategory *> categories_;
> std::list<std::pair<std::string, LogSeverity>> levels_;
>
> @@ -572,6 +658,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);
> @@ -593,7 +681,9 @@ Logger::Logger()
> logSetStream(&std::cerr, color);
>
> parseLogFile();
> - parseLogLevels();
> +
> + if (const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS"))
> + parseLogLevels(debug, levels_);
> }
>
> /**
> @@ -620,105 +710,21 @@ void Logger::parseLogFile()
> }
>
> /**
> - * \brief Parse the log levels from the environment
> - *
> - * The log levels are stored in the LIBCAMERA_LOG_LEVELS environment variable
> - * as a list of "category:level" pairs, separated by commas (','). Parse the
> - * variable and store the levels to configure all log categories.
> - */
> -void Logger::parseLogLevels()
> -{
> - const char *debug = utils::secure_getenv("LIBCAMERA_LOG_LEVELS");
> - if (!debug)
> - return;
> -
> - for (const char *pair = debug; *debug != '\0'; pair = debug) {
> - const char *comma = strchrnul(debug, ',');
> - size_t len = comma - pair;
> -
> - /* Skip over the comma. */
> - debug = *comma == ',' ? comma + 1 : comma;
> -
> - /* Skip to the next pair if the pair is empty. */
> - if (!len)
> - continue;
> -
> - std::string_view category;
> - std::string_view level;
> -
> - const char *colon = static_cast<const char *>(memchr(pair, ':', len));
> - if (!colon) {
> - /* 'x' is a shortcut for '*:x'. */
> - category = "*";
> - level = std::string_view(pair, len);
> - } else {
> - category = std::string_view(pair, colon - pair);
> - level = std::string_view(colon + 1, comma - colon - 1);
> - }
> -
> - /* Both the category and the level must be specified. */
> - if (category.empty() || level.empty())
> - continue;
> -
> - LogSeverity severity = parseLogLevel(level);
> - if (severity == LogInvalid)
> - continue;
> -
> - levels_.emplace_back(category, severity);
> - }
> -}
> -
> -/**
> - * \brief Parse a log level string into a LogSeverity
> - * \param[in] level The log level string
> - *
> - * Log levels can be specified as an integer value in the range from LogDebug to
> - * LogFatal, or as a string corresponding to the severity name in uppercase. Any
> - * other value is invalid.
> - *
> - * \return The log severity, or LogInvalid if the string is invalid
> + * \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
> */
> -LogSeverity Logger::parseLogLevel(std::string_view level)
> +LogCategory *Logger::findOrCreateCategory(std::string_view name)
> {
> - static const char *const names[] = {
> - "DEBUG",
> - "INFO",
> - "WARN",
> - "ERROR",
> - "FATAL",
> - };
> -
> - unsigned int severity;
> + MutexLocker locker(mutex_);
>
> - if (std::isdigit(level[0])) {
> - auto [end, ec] = std::from_chars(level.data(), level.data() + level.size(), severity, 10);
> - if (ec != std::errc() || *end != '\0' || severity > LogFatal)
> - severity = LogInvalid;
> - } else {
> - severity = LogInvalid;
> - for (unsigned int i = 0; i < std::size(names); ++i) {
> - if (names[i] == level) {
> - severity = i;
> - break;
> - }
> - }
> + for (LogCategory *c : categories_) {
> + if (c->name() == name)
> + return c;
> }
>
> - return static_cast<LogSeverity>(severity);
> -}
> -
> -/**
> - * \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.
> - */
> -void Logger::registerCategory(LogCategory *category)
> -{
> - categories_.push_back(category);
> + LogCategory *c = categories_.emplace_back(new LogCategory(name));
>
> - const std::string &name = category->name();
> for (const std::pair<std::string, LogSeverity> &level : levels_) {
> bool match = true;
>
> @@ -734,26 +740,12 @@ void Logger::registerCategory(LogCategory *category)
> }
>
> if (match) {
> - category->setSeverity(level.second);
> + c->setSeverity(level.second);
> break;
> }
> }
> -}
>
> -/**
> - * \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;
> - }
> -
> - return nullptr;
> + return c;
> }
>
> /**
> @@ -791,16 +783,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);
> }
>
> /**
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list