[RFC,v1,7/7] libcamera: base: log: Protect log categories with lock
diff mbox series

Message ID 20250121185554.301901-5-pobrn@protonmail.com
State Superseded
Headers show
Series
  • libcamera: base: log: Misc. changes
Related show

Commit Message

Barnabás Pőcze Jan. 21, 2025, 6:56 p.m. UTC
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@protonmail.com>
---
 include/libcamera/base/log.h |   2 +-
 src/libcamera/base/log.cpp   | 235 ++++++++++++++++-------------------
 2 files changed, 110 insertions(+), 127 deletions(-)

Comments

Laurent Pinchart Jan. 24, 2025, 7:39 p.m. UTC | #1
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@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);
>  }
>  
>  /**

Patch
diff mbox series

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);
 
 	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);
 }
 
 /**