[libcamera-devel] [PATCH 2/2] libcamera: base: log: Fix LogCategory creation issues

[Laurent Pinchart]

Hi Tomi,

Thank you for the patch.

On Fri, Aug 26, 2022 at 02:39:31PM +0300, Tomi Valkeinen via libcamera-devel wrote:
> Each declaration of a LogCategory will create a new LogCategory, and
> will be stored in an unordered_set Logger::categories_. This means that
> when a plugin .so is unloaded and loaded, as happens when destructing
> and creating a CamereManager, we'll get duplicate categories.
> 
> The Logger::registerCategory docs say "Log categories must have unique
> names. If a category with the same name already exists this function
> performs no operation.". The code does not comply with this.
> 
> We solve the issue with two changes:
> 
> Change the unordered_set to a vector for simplicity, as there's no need
> for an unordered_set.
> 
> Instead of using the LogCategory constructor to create new categories in
> _LOG_CATEGORY() macro, use a factory method. The factory method will
> return either an existing LogCategory if one exists with the given name,
> or a newly created one.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> ---
>  include/libcamera/base/log.h |  6 +++--
>  src/libcamera/base/log.cpp   | 51 +++++++++++++++++++++++++++++++-----
>  2 files changed, 48 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libcamera/base/log.h b/include/libcamera/base/log.h
> index 8b462767..76e01118 100644
> --- a/include/libcamera/base/log.h
> +++ b/include/libcamera/base/log.h
> @@ -29,7 +29,7 @@ enum LogSeverity {
>  class LogCategory
>  {
>  public:
> -	explicit LogCategory(const char *name);
> +	static LogCategory *create(const std::string &name);

Let's pass a const char * to this function, to have a single
construction of a std::string inside the function instead of in every
caller.

>  
>  	const std::string &name() const { return name_; }
>  	LogSeverity severity() const { return severity_; }
> @@ -38,6 +38,8 @@ public:
>  	static const LogCategory &defaultCategory();
>  
>  private:
> +	explicit LogCategory(const std::string &name);

Is the change from const char * to std::string needed here ? If so, does
it belong to 1/2 ?

> +
>  	const std::string name_;
>  	LogSeverity severity_;
>  };
> @@ -49,7 +51,7 @@ extern const LogCategory &_LOG_CATEGORY(name)();
>  const LogCategory &_LOG_CATEGORY(name)()				\
>  {									\
>  	/* The instance will be deleted by the Logger destructor. */	\
> -	static LogCategory *category = new LogCategory(#name);		\
> +	static LogCategory *category = LogCategory::create(#name);	\
>  	return *category;						\
>  }
>  
> diff --git a/src/libcamera/base/log.cpp b/src/libcamera/base/log.cpp
> index a4a5b452..c6500e96 100644
> --- a/src/libcamera/base/log.cpp
> +++ b/src/libcamera/base/log.cpp
> @@ -314,10 +314,11 @@ private:
>  
>  	friend LogCategory;
>  	void registerCategory(LogCategory *category);
> +	LogCategory *findCategory(const std::string &name) const;
>  
>  	static bool destroyed_;
>  
> -	std::unordered_set<LogCategory *> categories_;
> +	std::vector<LogCategory *> categories_;
>  	std::list<std::pair<std::string, LogSeverity>> levels_;
>  
>  	std::shared_ptr<LogOutput> output_;
> @@ -707,12 +708,14 @@ LogSeverity Logger::parseLogLevel(const std::string &level)
>   * \brief Register a log category with the logger
>   * \param[in] category The log category
>   *
> - * Log categories must have unique names. If a category with the same name
> - * already exists this function performs no operation.
> + * Log categories must have unique names. It is illegal to call this function

Nobody will arrest you for doing so, so you can write invalid instead of
illegal :-)

> + * if a log category with the same name already exists.
>   */
>  void Logger::registerCategory(LogCategory *category)
>  {
> -	categories_.insert(category);
> +	ASSERT(!findCategory(category->name()));

I wonder if this is overkill, as the caller checks that the category
isn't found first. I suppose the performance penalty of the lookup is
acceptable in debug mode, but maybe it's not needed ?

> +
> +	categories_.push_back(category);

This reminds me we need to add locking to the logger. That's out of
scope for this patch of course.

>  
>  	const std::string &name = category->name();
>  	for (const std::pair<std::string, LogSeverity> &level : levels_) {
> @@ -736,6 +739,21 @@ void Logger::registerCategory(LogCategory *category)
>  	}
>  }
>  
> +/**
> + * \brief Find an existing log category with the given name
> + * \param[in] name The name of the log category

 * \return ...

> + */
> +LogCategory *Logger::findCategory(const std::string &name) const
> +{
> +	if (auto it = std::find_if(categories_.begin(), categories_.end(),
> +				   [&name](auto c) { return name == c->name(); });

Would it be more efficient to store categories in an unordered_map
indexed by the category name ?

> +	    it != categories_.end()) {
> +		return *it;
> +	}
> +
> +	return nullptr;
> +}
> +
>  /**
>   * \enum LogSeverity
>   * Log message severity
> @@ -760,14 +778,33 @@ void Logger::registerCategory(LogCategory *category)
>   * and is used to control the log level per group.
>   */
>  
> +/**
> + * \brief Create a new LogCategory or return an existing one

 * \param[in] name ...

> + * \return The pointer to the LogCategory

The return documentation goes to the end of the comment.

> + *
> + * Create and return a new LogCategory with the given name if such a category
> + * does not yet exist, or return the existing one.
> + */
> +LogCategory *LogCategory::create(const std::string &name)
> +{
> +	LogCategory *category = Logger::instance()->findCategory(name);
> +
> +	if (!category) {
> +		category = new LogCategory(name);
> +

You could drop the blank line.

> +		Logger::instance()->registerCategory(category);
> +	}
> +
> +	return category;
> +}
> +
>  /**
>   * \brief Construct a log category
>   * \param[in] name The category name
>   */
> -LogCategory::LogCategory(const char *name)
> +LogCategory::LogCategory(const std::string &name)
>  	: name_(name), severity_(LogSeverity::LogInfo)
>  {
> -	Logger::instance()->registerCategory(this);
>  }
>  
>  /**
> @@ -804,7 +841,7 @@ void LogCategory::setSeverity(LogSeverity severity)
>   */
>  const LogCategory &LogCategory::defaultCategory()
>  {
> -	static const LogCategory *category = new LogCategory("default");
> +	static const LogCategory *category = LogCategory::create("default");
>  	return *category;
>  }
>  

-- 
Regards,

Laurent Pinchart